Re: Incorporate OpenAjax Hub as Pub-Sub Mechanism for Shindig (issue1247044)

[jo...@gmail.com]

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