This CL is huge, so I've cut my comments somewhere around half-way. I'm focusing on the pubsub pieces here moreso than the rpc.js changes.
In fact, on first glance it looks like the rpc changes could be separated. True? If so, let's create a new CL for them. These changes are definitely extensive, and there are strategic places I'd like to minimize the impact. rpc is one of them: a lot of new bytes are added here which in many circumstances are not required for existing users, but which would increase code size noticeably. Where possible, we should selectively inject the needed code. One technique for doing so might mirror what I've done for shindig.uri (creating shindig.uri.ext alongside it). More to come. http://codereview.appspot.com/1247044/diff/33001/34002 File content/container/sample-pubsub-2-publisher.xml (right): http://codereview.appspot.com/1247044/diff/33001/34002#newcode37 content/container/sample-pubsub-2-publisher.xml:37: var g = gadgets.byId(__MODULE_ID__); Hm, __MODULE_ID__ is pretty much deprecated at this point, as its purpose was mostly historical (to support insecure inlined gadgets). Is it required here? http://codereview.appspot.com/1247044/diff/33001/34003 File content/container/sample-pubsub-2-subscriber.xml (right): http://codereview.appspot.com/1247044/diff/33001/34003#newcode48 content/container/sample-pubsub-2-subscriber.xml:48: "message : " + gadgets.util.escapeString(data + "") + "<br/>"; nit: print out the new Date().toString() received http://codereview.appspot.com/1247044/diff/33001/34004 File content/container/sample-pubsub-2.html (right): http://codereview.appspot.com/1247044/diff/33001/34004#newcode43 content/container/sample-pubsub-2.html:43: return chromeId ? document.getElementById(chromeId) : null; nit: chromeId will always be defined here http://codereview.appspot.com/1247044/diff/33001/34004#newcode104 content/container/sample-pubsub-2.html:104: gadgets.io.makeNonProxiedRequest(url, this all feels like it should be pulled from common container. Why not use shindig.container.Service.getGadgetMetadata? http://codereview.appspot.com/1247044/diff/33001/34005 File features/src/main/javascript/features/core.util/util.js (right): http://codereview.appspot.com/1247044/diff/33001/34005#newcode130 features/src/main/javascript/features/core.util/util.js:130: // set, then 'parameters' will get overwritten as "{}". It's not a bug, just not the most cleanly-designed library, in that opt_url does allow parsing of "ad hoc" query strings, but as you note overwrites parameters. For the time being, we should keep parameters present here, and probably just gret rid of the opt_url feature slowly. It's replaced by the shindig.uri library. http://codereview.appspot.com/1247044/diff/33001/34005#newcode343 features/src/main/javascript/features/core.util/util.js:343: return _gadgets[id]; what is this for? storing arbitrary N/V pair data about a gadget, it appears? (I may discover as the review proceeds) http://codereview.appspot.com/1247044/diff/33001/34007 File features/src/main/javascript/features/pubsub-2/OpenAjax-mashup.js (right): http://codereview.appspot.com/1247044/diff/33001/34007#newcode26 features/src/main/javascript/features/pubsub-2/OpenAjax-mashup.js:26: var libs = {}; styling nit: 2-space indents http://codereview.appspot.com/1247044/diff/33001/34007#newcode36 features/src/main/javascript/features/pubsub-2/OpenAjax-mashup.js:36: registerLibrary: function(prefix, nsURL, version, extra){ nit: space before { http://codereview.appspot.com/1247044/diff/33001/34007#newcode56 features/src/main/javascript/features/pubsub-2/OpenAjax-mashup.js:56: * Standard Error names used when the standard functions need to throw Errors. I assume these are all standardized as part of OpenAjax. Wherever this is the case, please include a reference to the RFC/spec. http://codereview.appspot.com/1247044/diff/33001/34007#newcode63 features/src/main/javascript/features/pubsub-2/OpenAjax-mashup.js:63: Disconnected: "OpenAjax.hub.Error.Disconnected", hard to read -- perhaps a space before each. http://codereview.appspot.com/1247044/diff/33001/34007#newcode114 features/src/main/javascript/features/pubsub-2/OpenAjax-mashup.js:114: //OpenAjax.hub.Hub = function() {} Is this purely here for documentation purposes (ergo commented out)? http://codereview.appspot.com/1247044/diff/33001/34007#newcode134 features/src/main/javascript/features/pubsub-2/OpenAjax-mashup.js:134: * parameter of the onData callback function. which, if any, of these params are optional? If none, then this is fine. Those that are by convention are prefixed opt_. http://codereview.appspot.com/1247044/diff/33001/34007#newcode144 features/src/main/javascript/features/pubsub-2/OpenAjax-mashup.js:144: //OpenAjax.hub.Hub.prototype.subscribe = function( topic, onData, scope, onComplete, subscriberData ) {} local style nits: no space before first arg; where possible, fit to 100 chars per line. http://codereview.appspot.com/1247044/diff/33001/34007#newcode144 features/src/main/javascript/features/pubsub-2/OpenAjax-mashup.js:144: //OpenAjax.hub.Hub.prototype.subscribe = function( topic, onData, scope, onComplete, subscriberData ) {} I probably missed the opportunity to comment on this in the spec, but it seems that both subscriberData and scope could be removed simply by using a closure in onData: var subscriberData = {}; var self = this; var onData = function(topic, data) { onDataHandler.call(self, topic, data, subscriberData); }; myHubInstance.subscribe("topic", onData, ...); http://codereview.appspot.com/1247044/diff/33001/34007#newcode229 features/src/main/javascript/features/pubsub-2/OpenAjax-mashup.js:229: //OpenAjax.hub.Hub.prototype.getSubscriberScope = function(subscriberID) {} FMI, why not just let the caller manage this if (s)he wants? http://codereview.appspot.com/1247044/diff/33001/34007#newcode390 features/src/main/javascript/features/pubsub-2/OpenAjax-mashup.js:390: * the ManagedHub. The caller MUST not modify it. why not just copy it and spare yourself the trouble? http://codereview.appspot.com/1247044/diff/33001/34007#newcode430 features/src/main/javascript/features/pubsub-2/OpenAjax-mashup.js:430: this._scope = params.scope || window; window is always accessible; IMO null is the better default. http://codereview.appspot.com/1247044/diff/33001/34008 File features/src/main/javascript/features/pubsub-2/crypto.js (right): http://codereview.appspot.com/1247044/diff/33001/34008#newcode16 features/src/main/javascript/features/pubsub-2/crypto.js:16: */ if I'm not mistaken, we typically import w/ Apache copyright; and occasionally switch namespacing and such -- all while providing source attribution of course. That's what we did w/ the shindig.random sha1 impl anyway. http://codereview.appspot.com/1247044/diff/33001/34008#newcode21 features/src/main/javascript/features/pubsub-2/crypto.js:21: // BigEndianWord[5] <- smash.crypto.sha1( BigEndianWord[*] dataWA, int lenInBits) feature shindig.random includes a sha1 implementation; we should reuse that. http://codereview.appspot.com/1247044/diff/33001/34010 File features/src/main/javascript/features/pubsub-2/iframe.js (right): http://codereview.appspot.com/1247044/diff/33001/34010#newcode35 features/src/main/javascript/features/pubsub-2/iframe.js:35: if (!window.gadgets) { A. you can ensure window.gadgets exists by including globals.js in feature.xml B. Why not use gadgets.config for all this configuration? http://codereview.appspot.com/1247044/diff/33001/34010#newcode145 features/src/main/javascript/features/pubsub-2/iframe.js:145: ! params.IframeContainer.uri || ! params.IframeContainer.tunnelURI ) { seems a bit verbose. Perhaps break into a validation fn of some kind. http://codereview.appspot.com/1247044/diff/33001/34011 File features/src/main/javascript/features/pubsub-2/pubsub-2-router.js (right): http://codereview.appspot.com/1247044/diff/33001/34011#newcode32 features/src/main/javascript/features/pubsub-2/pubsub-2-router.js:32: gadgets.pubsub2router = function() { So this is just a shortcut to a singleton Hub? http://codereview.appspot.com/1247044/diff/33001/34014 File features/src/main/javascript/features/rpc/nix.transport.js (right): http://codereview.appspot.com/1247044/diff/33001/34014#newcode145 features/src/main/javascript/features/rpc/nix.transport.js:145: function setupRelay(rpctoken) { s/setupRelay/setupSecureRelayToParent/ or similar. http://codereview.appspot.com/1247044/diff/33001/34014#newcode148 features/src/main/javascript/features/rpc/nix.transport.js:148: var childToken = (0x7FFFFFFF * Math.random()) | 0; // XXX expose way to have child set this value for better security, use shindig.random http://codereview.appspot.com/1247044/diff/33001/34014#newcode150 features/src/main/javascript/features/rpc/nix.transport.js:150: window.location.href.substring(0, window.location.href.indexOf('#')), Should break out the self-URI parsing code (accommodating conditions where # is not present in the url etc) http://codereview.appspot.com/1247044/diff/33001/34014#newcode173 features/src/main/javascript/features/rpc/nix.transport.js:173: } this whole method should probably be broken into a helper in gadgets.rpc, with an onSuccess continuation. http://codereview.appspot.com/1247044/diff/33001/34015 File features/src/main/javascript/features/rpc/rpc.js (right): http://codereview.appspot.com/1247044/diff/33001/34015#newcode221 features/src/main/javascript/features/rpc/rpc.js:221: } This pattern is used in a few places. Perhaps it should be broken into a gadgets.util method (attachBrowserEvent). http://codereview.appspot.com/1247044/diff/33001/34015#newcode494 features/src/main/javascript/features/rpc/rpc.js:494: gadgets.error("URL passed to setRelayUrl must be absolute."); we could just absolutify the URL no? http://codereview.appspot.com/1247044/show
