Simpler taming of opensocial, gadgets and related apis

[Jasvir, 2009-10-19 22:28:15]

The original taming of opensocial was very verbose because any callbacks (which
occur commonly in opensocial) had to be hand tamed.  The new taming api makes it
easier to tame new features by declaring just the methods, functions and
constructors that your feature makes available to a cajoled gadget

See http://code.google.com/p/google-caja/wiki/LibraryTaming for further
instructions

This change:
* refactors the taming out of caja/taming.js into individual features (which get
minified along with the rest of the feature)
* plugs two obvious xss errors in opensocial

[Jasvir, 2009-10-19 22:35:45]

http://codereview.appspot.com/135051/diff/1027/48
File features/src/main/javascript/features/caja/taming.js (right):

http://codereview.appspot.com/135051/diff/1027/48#newcode105
Line 105: var tamings___ = tamings___ || [];
Changed from register function to an array of hooks because the caja feature is
not guaranteed to be loaded before other features.  This is similar to how
gadgets.* and opensocial.* get initialized.

http://codereview.appspot.com/135051/diff/1027/54
File
features/src/main/javascript/features/opensocial-jsonrpc/jsonrpccontainer.js
(left):

http://codereview.appspot.com/135051/diff/1027/54#oldcode502
Line 502: 
Removed because it unnecessarily shadowed an identical JsonRpcRequestItem
defined above which made it impossible to whitelist.

[johnfargo, 2009-10-20 21:53:57]

Aside from the rather minor comments below, my primary concern is that this adds
bulk to every gadget render, not just those that are cajoled.

This said, I agree it's the right move to include tamings with features
themselves. It appears that tamings for gadget and container context are
equivalent - if so, a new RenderingContext.CAJA might be in order:
<tamings>
  <script src="..."/>
</tamings>

Or we could try to genericize this by adding extensible syntax:
<ext:caja>
  <script src="..."/'>
</ext:caja>
...the content of which could be read by a Rewriter (CajaContentRewriter) and
injected as needed.

...or something similar. Thoughts?

http://codereview.appspot.com/135051/diff/1027/48
File features/src/main/javascript/features/caja/taming.js (right):

http://codereview.appspot.com/135051/diff/1027/48#newcode105
Line 105: var tamings___ = tamings___ || [];
Not that it's a big deal in this case, but maybe it should be. This is one of a
few use cases I've seen arise that call for a clearer representation of the
feature dependency tree.

http://codereview.appspot.com/135051/diff/1027/48#newcode115
Line 115: ___.grantFunc(imports.outers.console, 'log');
nit: I wonder if this is necessary, vs. just encouraging people to use
gadgets.log.

http://codereview.appspot.com/135051/diff/1027/46
File features/src/main/javascript/features/flash/taming.js (right):

http://codereview.appspot.com/135051/diff/1027/46#newcode1
Line 1: /*
Missing a corresponding feature.xml update for flash.

http://codereview.appspot.com/135051/diff/1027/46#newcode36
Line 36: var O = ifr.contentWindow.Object;
possible mini-optimization: pull A and O above var cleanse to avoid having to
recreate the frame.

http://codereview.appspot.com/135051/diff/1027/54
File
features/src/main/javascript/features/opensocial-jsonrpc/jsonrpccontainer.js
(right):

http://codereview.appspot.com/135051/diff/1027/54#newcode59
Line 59: var JsonRpcRequestItem = function(rpc, opt_processData) {
nit: spacing 2 to the left.

[Jasvir, 2009-10-21 00:06:58]

On 2009/10/20 21:53:57, johnfargo wrote:
> Aside from the rather minor comments below, my primary concern is that this
adds
> bulk to every gadget render, not just those that are cajoled.
> 
> This said, I agree it's the right move to include tamings with features
> themselves. It appears that tamings for gadget and container context are
> equivalent - if so, a new RenderingContext.CAJA might be in order:
> <tamings>
>   <script src="..."/>
> </tamings>
> 
> Or we could try to genericize this by adding extensible syntax:
> <ext:caja>
>   <script src="..."/'>
> </ext:caja>
> ...the content of which could be read by a Rewriter (CajaContentRewriter) and
> injected as needed.
> 
> ...or something similar. Thoughts?

jasvir@rorohiko:~/src/svn-changes/shindig-tame/features/src/main/javascript/features:0$
for i in */taming.js ; do x=$(du -b `dirname $i` --exclude=.svn); echo `du -b $i
| cut -f1` $x; done       
3676 10135 caja
1096 22998 core.io
2006 60921 core
1087 12984 dynamic-height
5413 17379 flash
1273 14395 minimessage
6389 151840 opensocial-reference
1105 12448 pubsub
1052 7408 settitle
1034 9058 skins
1578 27632 tabs
1381 17326 views

Do you think the gains are significant in comparison to the size of each feature
and the complexity of maintaining another renderingcontext?

> 
> http://codereview.appspot.com/135051/diff/1027/48
> File features/src/main/javascript/features/caja/taming.js (right):
> 
> http://codereview.appspot.com/135051/diff/1027/48#newcode105
> Line 105: var tamings___ = tamings___ || [];
> Not that it's a big deal in this case, but maybe it should be. This is one of
a
> few use cases I've seen arise that call for a clearer representation of the
> feature dependency tree.
> 
> http://codereview.appspot.com/135051/diff/1027/48#newcode115
> Line 115: ___.grantFunc(imports.outers.console, 'log');
> nit: I wonder if this is necessary, vs. just encouraging people to use
> gadgets.log.
> 
> http://codereview.appspot.com/135051/diff/1027/46
> File features/src/main/javascript/features/flash/taming.js (right):
> 
> http://codereview.appspot.com/135051/diff/1027/46#newcode1
> Line 1: /*
> Missing a corresponding feature.xml update for flash.
> 
> http://codereview.appspot.com/135051/diff/1027/46#newcode36
> Line 36: var O = ifr.contentWindow.Object;
> possible mini-optimization: pull A and O above var cleanse to avoid having to
> recreate the frame.
> 
> http://codereview.appspot.com/135051/diff/1027/54
> File
> features/src/main/javascript/features/opensocial-jsonrpc/jsonrpccontainer.js
> (right):
> 
> http://codereview.appspot.com/135051/diff/1027/54#newcode59
> Line 59: var JsonRpcRequestItem = function(rpc, opt_processData) {
> nit: spacing 2 to the left.

[Jasvir, 2009-10-21 00:07:24]

http://codereview.appspot.com/135051/diff/1027/48
File features/src/main/javascript/features/caja/taming.js (right):

http://codereview.appspot.com/135051/diff/1027/48#newcode115
Line 115: ___.grantFunc(imports.outers.console, 'log');
On 2009/10/20 21:53:57, johnfargo wrote:
> nit: I wonder if this is necessary, vs. just encouraging people to use
> gadgets.log.

Done.

http://codereview.appspot.com/135051/diff/1027/54
File
features/src/main/javascript/features/opensocial-jsonrpc/jsonrpccontainer.js
(right):

http://codereview.appspot.com/135051/diff/1027/54#newcode59
Line 59: var JsonRpcRequestItem = function(rpc, opt_processData) {
On 2009/10/20 21:53:57, johnfargo wrote:
> nit: spacing 2 to the left.

Done.

[Jasvir, 2009-10-21 19:03:23]

http://codereview.appspot.com/135051/diff/1027/48
File features/src/main/javascript/features/caja/taming.js (right):

http://codereview.appspot.com/135051/diff/1027/48#newcode105
Line 105: var tamings___ = tamings___ || [];
This works for now.  Its vulnerable to a feature you don't trust resetting this
array entirely to prevent it from getting exposed to a gadget but if you have a
feature you don't trust, it can do anything anyways.

On 2009/10/20 21:53:57, johnfargo wrote:
> Not that it's a big deal in this case, but maybe it should be. This is one of
a
> few use cases I've seen arise that call for a clearer representation of the
> feature dependency tree.

http://codereview.appspot.com/135051/diff/1027/46
File features/src/main/javascript/features/flash/taming.js (right):

http://codereview.appspot.com/135051/diff/1027/46#newcode1
Line 1: /*
On 2009/10/20 21:53:57, johnfargo wrote:
> Missing a corresponding feature.xml update for flash.

Done.

[Jasvir, 2009-10-26 06:18:01]

Snapshot.

On 2009/10/21 19:03:23, jasvir wrote:
> http://codereview.appspot.com/135051/diff/1027/48
> File features/src/main/javascript/features/caja/taming.js (right):
> 
> http://codereview.appspot.com/135051/diff/1027/48#newcode105
> Line 105: var tamings___ = tamings___ || [];
> This works for now.  Its vulnerable to a feature you don't trust resetting
this
> array entirely to prevent it from getting exposed to a gadget but if you have
a
> feature you don't trust, it can do anything anyways.
> 
> On 2009/10/20 21:53:57, johnfargo wrote:
> > Not that it's a big deal in this case, but maybe it should be. This is one
of
> a
> > few use cases I've seen arise that call for a clearer representation of the
> > feature dependency tree.
> 
> http://codereview.appspot.com/135051/diff/1027/46
> File features/src/main/javascript/features/flash/taming.js (right):
> 
> http://codereview.appspot.com/135051/diff/1027/46#newcode1
> Line 1: /*
> On 2009/10/20 21:53:57, johnfargo wrote:
> > Missing a corresponding feature.xml update for flash.
> 
> Done.