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.
http://codereview.appspot.com/67159
