Skins Implementation

[Paul Lindner <pl...@hi5.com>]

i, 
 
I committed a skins implmentation. I'd appreciate it if the javascript 
experts could review it.  Some notes: 
 
* The skins code uses idioms from views.js to interact with 
  gadgets.config.  This resulted in converting the object style 
  declarations with a function closure.  Is this okay?  I didn't see 
  anything in the style guide about this 
 
* I declared a new skins entry in syndicator.js that uses a properties 
  declaration with an ExistsValidator 
  - should we add validity checks for each and every field? 
 
* I added gadgets.Skins.init(properties), which allows the set of 
  properties to be set.  This is needed so the container can 
  dynamically set the skins values on a user-by-user basis. 
 
* Do we want to automatically set up the skins styles for the 
  gadget when rendered in an iframe?  I'm thinking that we could add 
  code like this: 
 
   document.bgColor = gadgets.skins.getProperty(gadgets.skins.Property.BG_COLOR); 
   document.body.style.color = gadgets.skins.getProperty(gadgets.skins.Property.FONT_COLOR); 
   ... 
 
  In the inlined-caja context this will already be true, might as well 
  make it work in a consistent way in the iframes. 
 
Thanks 

--  
Paul Lindner 
hi5 Architect 
plindner@hi5.com 
 

Re: Skins Implementation

[Kevin Brown <et...@google.com>]

On Sat, Mar 1, 2008 at 7:23 PM, Brian Eaton <be...@google.com> wrote:

> On Sat, Mar 1, 2008 at 2:38 PM, Kevin Brown <et...@google.com> wrote:
> >  Currently this isn't possible, since the skin attributes are defined in
> the
> >  rendering server, not by any outside source.
>
> Rendering container, or rendering gadget server?  I trust the gadget
> server for this, at least for now.  The container less so.


Yes, the gadget server defines these values. They never come from user or
container input at this point in time. If / when we support something like

/ifr?url=...&skin=http://...

we'll of course need to sanitize the values from the skin configuration. For
now, any per-user skin support would require custom code beyond what's in
shindig already.

Re: Skins Implementation

[Brian Eaton <be...@google.com>]

On Sat, Mar 1, 2008 at 2:38 PM, Kevin Brown <et...@google.com> wrote:
>  Currently this isn't possible, since the skin attributes are defined in the
>  rendering server, not by any outside source.

Rendering container, or rendering gadget server?  I trust the gadget
server for this, at least for now.  The container less so.

Your hangman proposal sounds fine to me.

Re: Skins Implementation

[Kevin Brown <et...@google.com>]

On Sat, Mar 1, 2008 at 10:23 AM, Brian Eaton <be...@google.com> wrote:

> On Sat, Mar 1, 2008 at 7:30 AM, Paul Lindner <pl...@hi5.com> wrote:
> >  * Do we want to automatically set up the skins styles for the
> >   gadget when rendered in an iframe?
>
> Can you add in-gadget code to sanitize the styles before they are
> applied to the gadget?  I'd like to avoid a situation where someone
> can impersonate a container and inject script via the skins feature.


Currently this isn't possible, since the skin attributes are defined in the
rendering server, not by any outside source.


>
> Cheers,
> Brian
>



-- 
~Kevin

Re: Skins Implementation

[Brian Eaton <be...@google.com>]

On Sat, Mar 1, 2008 at 7:30 AM, Paul Lindner <pl...@hi5.com> wrote:
>  * Do we want to automatically set up the skins styles for the
>   gadget when rendered in an iframe?

Can you add in-gadget code to sanitize the styles before they are
applied to the gadget?  I'd like to avoid a situation where someone
can impersonate a container and inject script via the skins feature.

Cheers,
Brian

Re: Skins Implementation

[Kevin Brown <et...@google.com>]

On Sat, Mar 1, 2008 at 7:30 AM, Paul Lindner <pl...@hi5.com> wrote:

>
> i,
>
> I committed a skins implmentation. I'd appreciate it if the javascript
> experts could review it.  Some notes:
>
> * The skins code uses idioms from views.js to interact with
>  gadgets.config.  This resulted in converting the object style
>  declarations with a function closure.  Is this okay?  I didn't see
>  anything in the style guide about this


http://cwiki.apache.org/confluence/display/SHINDIGxSITE/Javascript+Style#JavascriptStyle-Closures

Use them.

* I declared a new skins entry in syndicator.js that uses a properties
>  declaration with an ExistsValidator
>  - should we add validity checks for each and every field?


 Go ahead if you think it's useful -- now that we have the 'debug' flag, we
can easily disable the validation checks in production environments to avoid
the performance penalty.


>
> * I added gadgets.Skins.init(properties), which allows the set of
>  properties to be set.  This is needed so the container can
>  dynamically set the skins values on a user-by-user basis.


If we go down this route (and I think we should), we should probably come up
with a way for containers to define skins using some really basic markup
(key value pairs) so that the parent page can pass something like
&skin=<skin id>. Otherwise we'll get into patch maintenance hell. I think
John H. had some ideas on this.


>
> * Do we want to automatically set up the skins styles for the
>  gadget when rendered in an iframe?  I'm thinking that we could add
>  code like this:
>
>   document.bgColor = gadgets.skins.getProperty(
> gadgets.skins.Property.BG_COLOR);
>   document.body.style.color = gadgets.skins.getProperty(
> gadgets.skins.Property.FONT_COLOR);


This is a rather significant rendering performance hit. We should probably
automatically generate this in the output CSS.

I'm leaning heavily towards proposing __SKIN_ hangman variable substitution
in the spec as well -- otherwise you'll have gadgets with enormous blocks of
javascript like this, and terrible performance. Even if you're careful and
do all the property assignment in a single function so as to delay
reapplication of styles, you'd still wind up at least one extra
reapplication, which is slow.


>   ...
>
>  In the inlined-caja context this will already be true, might as well
>  make it work in a consistent way in the iframes.
>
> Thanks
>
> --
> Paul Lindner
> hi5 Architect
> plindner@hi5.com
>
>


-- 
~Kevin