osapi rewrite to support gadgets.rpc

Rewrite of osapi to factor out the RPC transport mechanism and support a gadgets.rpc based transport in addition to the json-rpc one. This is a pretty extensive rewrite but hopefully the code is a little simpler and more extensible now. It should be fairly trivial to add a REST transport instead of the json-rpc one for instance.

There is now a container-side osapi.js which container can use as a basis for exposing additional osapi services using the new gadgets.rpc transport. See /javascript/container/osapi.js. 

To enable this feature in your container config you can either have the set of services introspected at gadget render-time using
{ "gadget.features" ...
 { "osapi.services" : 
   {"gadgets.rpc" : ["container.listMethods"]}
 }
}

or if you want to avoid introspection and just list out the container methods to expose and exclude "container.listMethods". E.g.

{ "gadget.features" ...
 { "osapi.services" : 
   {"gadgets.rpc" : 
     ["requestShareApp", "requestPermission"]}
 }
}

In your container JS to add new methods simply bind them to the osapi namespace. E.g.

osapi.newservice = {};
osapi.newservice.do = function (request, callback) {
  callback(<data>);
}

will expose osapi.newservice.do inside any gadget on the page if introspection is enabled, if introspection is disabled it needs to be added to the container config.

The samplecontainer is updated to support introspection

In addition the container config now supports declaring the set of available methods for a json-rpc endpoint directly too so that Shindig doesnt have to call system.listMethods against each endpoint. Instead you can directly add

{ "gadget.features" ...
 { "osapi.services" : 
   {"http://%host%/social/rpc" : 
     ["people.get", "activities.get", ...]}
 }
}

Other minor changes..
 - makeNonProxiedRequest now returns error "rc" and "text" fields
 - osapi introspection over gadgets.rpc can delay the gadget onload callback by a max of 300ms.
- osapi.ui is removed. The sample container exposes "activities.requestCreate", "messages.requestSend", "requestShareApp", "requestPermission" as examples only. This is subject to change as these are not standardized
- osapi batching supports request chaining. If the response to an osapi api call is a JSON-RPC method call, it is executed and made available under the original key

Thanks to Sachin Senoy for his initial work on the gadgets.rpc transport.

[awiner, 2009-06-08 17:12:43]

Generally looks good.  A few nits, a couple points in the code where I'm
confused.  The only question of general interest is whether we should use
"requestXyz()" for method names - I'd vote no, drop the "request" convention.

http://codereview.appspot.com/67159/diff/1/20
File ../trunk/features/src/main/javascript/features/osapi/batch.js (right):

http://codereview.appspot.com/67159/diff/1/20#newcode129
Line 129: current(batchResult[key].params);
control flow isn't obvious here - does this execute the chained call directly? 
If so, why are we also dispatching the RPC below?

http://codereview.appspot.com/67159/diff/1/20#newcode160
Line 160: window.setTimeout(function(){transportCallback({})}, 0);
what's the advantage of going through the transport callback instead of directly
to the user callback?

http://codereview.appspot.com/67159/diff/1/23
File ../trunk/features/src/main/javascript/features/osapi/gadgetsrpctransport.js
(right):

http://codereview.appspot.com/67159/diff/1/23#newcode32
Line 32: *   <gadget frame>, 'osapi.handleGadgetRpcCallback', null, callbackId,
<gadget frame id>?

http://codereview.appspot.com/67159/diff/1/23#newcode65
Line 65: transport.execute = execute;
might as well assign immediately on line 58

http://codereview.appspot.com/67159/diff/1/23#newcode75
Line 75: // introspect the container services for availabe methods and bind them
available

http://codereview.appspot.com/67159/diff/1/23#newcode87
Line 87: console.trace();
delete

http://codereview.appspot.com/67159/diff/1/23#newcode107
Line 107: window.setTimeout("gadgets.util.runOnLoadHandlers()", 300);
i think of the string form as deprecated

Also, why do we need to wait both for the listMethods() callback to run and
300ms to elapse?

http://codereview.appspot.com/67159/diff/1/24
File ../trunk/features/src/main/javascript/features/osapi/jsonrpctransport.js
(right):

http://codereview.appspot.com/67159/diff/1/24#newcode76
Line 76: var transport = { name : endpointUrl };
= {name : endpointUrl, execute : execute};

http://codereview.appspot.com/67159/diff/1/16
File ../trunk/features/src/main/javascript/features/osapi/osapi.js (right):

http://codereview.appspot.com/67159/diff/1/16#newcode51
Line 51: // defaults that we dont have to populate this.
what would break if we stopped doing this in the client?  Even if the spec is
unclear, our handlers shouldn't be.  (Super-ugly too to assume that all RPCs
have userId and groupId.)

http://codereview.appspot.com/67159/diff/1/16#newcode64
Line 64: last[parts[parts.length - 1]] = apiMethod;
consider throwing an error if the same method is registered twice

http://codereview.appspot.com/67159/diff/1/22
File ../trunk/features/src/main/javascript/features/osapi/peoplehelpers.js
(right):

http://codereview.appspot.com/67159/diff/1/22#newcode22
Line 22: gadgets.util.registerOnLoadHandler(function() {
which registerOnLoadHandler() is this?  The "real" one, or the wrapped one that
won't execute 'til later.  Looks like we have strong ordering requirements.

http://codereview.appspot.com/67159/diff/1/2
File ../trunk/javascript/samplecontainer/samplecontainer.js (right):

http://codereview.appspot.com/67159/diff/1/2#newcode276
Line 276: osapi.activities.requestCreate = function(request, callback) {
why requestCreate(), etc.?  Spec conversations were that these would simply be
"create()", etc.

[Sachin Shenoy, 2009-06-08 20:15:33]

Nice work! Looks good.

Will have another look later tomorrow.

http://codereview.appspot.com/67159/diff/1/20
File ../trunk/features/src/main/javascript/features/osapi/batch.js (right):

http://codereview.appspot.com/67159/diff/1/20#newcode130
Line 130: 
On the same note it is not clear how UI + JsonRPC call gets chained. Can both UI
and JsonRPC register for same method, say "activities.create". When the method
is invoked UI gets to handle it first and after getting users consent the method
gets sent via JsonRPC.

http://codereview.appspot.com/67159/diff/1/6
File ../trunk/javascript/container/osapi.js (right):

http://codereview.appspot.com/67159/diff/1/6#newcode87
Line 87: recurseNames(osapi, "", 5, names)
Is it better to do this over osapi.ui?

[etnu00, 2009-06-08 20:23:46]

http://codereview.appspot.com/67159/diff/1/23
File ../trunk/features/src/main/javascript/features/osapi/gadgetsrpctransport.js
(right):

http://codereview.appspot.com/67159/diff/1/23#newcode43
Line 43: callback({ code : 500, message : 'Container refused the request' });
Didn't we just recently modify opensocial to require that calbacks happen
asynchronously? This seems like a regression.

http://codereview.appspot.com/67159/diff/1/23#newcode84
Line 84: gadgets.util._runOnLoadHandlers = gadgets.util.runOnLoadHandlers;
You shouldn't set a placeholder on gadgets.util. Just declare a local variable.

http://codereview.appspot.com/67159/diff/1/23#newcode102
Line 102: gadgets.util.runOnLoadHandlers();
The overall structure here is pretty strange. Instead of calling onLoadHandler,
why not create a new method that deals with the delegation logic, and override
onLoadHandler to call that instead.

This code will fail if somebody else modifies runOnLoadHandler (say, to insert
something else 'before' handlers are called), which strikes me as being quite
fragile.

http://codereview.appspot.com/67159/diff/1/23#newcode107
Line 107: window.setTimeout("gadgets.util.runOnLoadHandlers()", 300);
There's no reason to ever use the string form (which is just another variant of
eval). Just setTimeout(gadgets.util.runOnLoadHandlers).

This also doesn't actually 'wait' 300ms. If I have code that depends on the
results of listMethods, I can NOT call it until I am sure that this code has
completed. That means that I can not write any code that runs outside of an
onload handler.

[louiscryan, 2009-06-08 21:52:07]

After discussion I've removed the call chaining as it doesnt add a lot of value.
Containers can always implement a custom transport if they need to do something
quirky like this but in general it should just be simpler for them to have the
container fulfill the entire request directly.

http://codereview.appspot.com/67159/diff/1/20
File ../trunk/features/src/main/javascript/features/osapi/batch.js (right):

http://codereview.appspot.com/67159/diff/1/20#newcode129
Line 129: current(batchResult[key].params);
On 2009/06/08 17:12:43, awiner wrote:
> control flow isn't obvious here - does this execute the chained call directly?

> If so, why are we also dispatching the RPC below?

Executing the call here will cause the RPC to be added to the batch under the
original key, it still needs to be passed to the transport for execution. Ill
add a more thorough unit test for this area as its a bit lacking currently.

http://codereview.appspot.com/67159/diff/1/20#newcode130
Line 130: 
On 2009/06/08 20:15:33, Sachin Shenoy wrote:
> On the same note it is not clear how UI + JsonRPC call gets chained. Can both
UI
> and JsonRPC register for same method, say "activities.create". When the method
> is invoked UI gets to handle it first and after getting users consent the
method
> gets sent via JsonRPC.

We dont allow method overloading. The container would register activities.create
which would actual cause the creation and return a JSON-RPC method
activities.get which is used to fetch the result. The container could of course
just fully implement activities.create and return the actual result

http://codereview.appspot.com/67159/diff/1/20#newcode160
Line 160: window.setTimeout(function(){transportCallback({})}, 0);
On 2009/06/08 17:12:43, awiner wrote:
> what's the advantage of going through the transport callback instead of
directly
> to the user callback?

None, removed

http://codereview.appspot.com/67159/diff/1/23
File ../trunk/features/src/main/javascript/features/osapi/gadgetsrpctransport.js
(right):

http://codereview.appspot.com/67159/diff/1/23#newcode32
Line 32: *   <gadget frame>, 'osapi.handleGadgetRpcCallback', null, callbackId,
On 2009/06/08 17:12:43, awiner wrote:
> <gadget frame id>?

Cleaned up comment

http://codereview.appspot.com/67159/diff/1/23#newcode43
Line 43: callback({ code : 500, message : 'Container refused the request' });
On 2009/06/08 20:23:46, etnu00 wrote:
> Didn't we just recently modify opensocial to require that calbacks happen
> asynchronously? This seems like a regression.

This is dispatched inside an asynchronous callback

http://codereview.appspot.com/67159/diff/1/23#newcode65
Line 65: transport.execute = execute;
On 2009/06/08 17:12:43, awiner wrote:
> might as well assign immediately on line 58

Done.

http://codereview.appspot.com/67159/diff/1/23#newcode75
Line 75: // introspect the container services for availabe methods and bind them
On 2009/06/08 17:12:43, awiner wrote:
> available

Done.

http://codereview.appspot.com/67159/diff/1/23#newcode84
Line 84: gadgets.util._runOnLoadHandlers = gadgets.util.runOnLoadHandlers;
On 2009/06/08 20:23:46, etnu00 wrote:
> You shouldn't set a placeholder on gadgets.util. Just declare a local
variable.

Done.

http://codereview.appspot.com/67159/diff/1/23#newcode87
Line 87: console.trace();
On 2009/06/08 17:12:43, awiner wrote:
> delete

Done.

http://codereview.appspot.com/67159/diff/1/23#newcode102
Line 102: gadgets.util.runOnLoadHandlers();
On 2009/06/08 20:23:46, etnu00 wrote:
> The overall structure here is pretty strange. Instead of calling
onLoadHandler,
> why not create a new method that deals with the delegation logic, and override
> onLoadHandler to call that instead.

Isnt that exactly what is done above?

> 
> This code will fail if somebody else modifies runOnLoadHandler (say, to insert
> something else 'before' handlers are called), which strikes me as being quite
> fragile.

Currently the only way the could do that would be to chain the call exactly as I
have done here which should still work

http://codereview.appspot.com/67159/diff/1/23#newcode107
Line 107: window.setTimeout("gadgets.util.runOnLoadHandlers()", 300);
On 2009/06/08 17:12:43, awiner wrote:
> i think of the string form as deprecated
> 

Done

> Also, why do we need to wait both for the listMethods() callback to run and
> 300ms to elapse?

You wait for 2 of 3 things to occur. 
- Call of runOnLoad at end of rendered gadget
- Callback from container.listMethods
- timeout

http://codereview.appspot.com/67159/diff/1/24
File ../trunk/features/src/main/javascript/features/osapi/jsonrpctransport.js
(right):

http://codereview.appspot.com/67159/diff/1/24#newcode76
Line 76: var transport = { name : endpointUrl };
On 2009/06/08 17:12:43, awiner wrote:
> = {name : endpointUrl, execute : execute};

Done.

http://codereview.appspot.com/67159/diff/1/16
File ../trunk/features/src/main/javascript/features/osapi/osapi.js (right):

http://codereview.appspot.com/67159/diff/1/16#newcode51
Line 51: // defaults that we dont have to populate this.
On 2009/06/08 17:12:43, awiner wrote:
> what would break if we stopped doing this in the client?  Even if the spec is
> unclear, our handlers shouldn't be.  (Super-ugly too to assume that all RPCs
> have userId and groupId.)

Agreed, Im just bringing over what Bob had because I dont want to gate this on
making sure all our handlers work properly. I can track this as a JIRA however.

http://codereview.appspot.com/67159/diff/1/16#newcode64
Line 64: last[parts[parts.length - 1]] = apiMethod;
On 2009/06/08 17:12:43, awiner wrote:
> consider throwing an error if the same method is registered twice

added gadgets.warn call.

http://codereview.appspot.com/67159/diff/1/6
File ../trunk/javascript/container/osapi.js (right):

http://codereview.appspot.com/67159/diff/1/6#newcode87
Line 87: recurseNames(osapi, "", 5, names)
On 2009/06/08 20:15:33, Sachin Shenoy wrote:
> Is it better to do this over osapi.ui?

No, osapi.ui is completely removed.

http://codereview.appspot.com/67159/diff/1/2
File ../trunk/javascript/samplecontainer/samplecontainer.js (right):

http://codereview.appspot.com/67159/diff/1/2#newcode276
Line 276: osapi.activities.requestCreate = function(request, callback) {
On 2009/06/08 17:12:43, awiner wrote:
> why requestCreate(), etc.?  Spec conversations were that these would simply be
> "create()", etc.

Agreed, removing.

[louiscryan, 2009-06-08 21:54:22]

Updated patch