Incorporate OpenAjax Hub as Pub-Sub Mechanism for Shindig

Preliminary support for using OpenAjax Hub as a pubsub mechanism for OpenSocial/Shindig.  See http://wiki.opensocial.org/index.php?title=PubSub.next_Proposals for proposal.

New feature is simply named "pubsub-2".

This patch contains several files (OpenAjax-mashup.js, iframe.js, crypto.js) from an OpenAjax Hub branch, which uses Shindig RPC as the underlying transport layer for the Hub pubsub features.

Due to the way OpenAjax Hub works, I needed to know in shindig-container.js whether a gadget requires the "pubsub-2" feature and if so use the Hub code to create the iframe.  I wasn't sure what the best way to integrate that into shindig-container.js, so I just did it the easy way.  This definitely needs work.

Also, as per the discussion in the proposal linked above, the gadget side pubsub-2 API is accessible from a gadget instance object.  For lack of a better place, I just put it in util.js.

[Javier Pedemonte, 2010-06-21 03:31:18]

wiring metadata

[Javier Pedemonte, 2010-06-21 03:32:59]

Last patch:
Have metadata call return feature parameters in JSON response.  Used for pubsub
wiring metadata
(http://wiki.opensocial.org/index.php?title=PubSub.next_Proposals#Proposed_syn...).
Updated pubsub-2 samples to show wiring metadata.

[Javier Pedemonte, 2010-06-21 14:26:16]

As mentioned in the description at the top, the code in shindig-container.js is
not ideal:  it does a metadata call to the server so that it can see whether the
gadget requires the "pubsub-2" feature.  But it looks like there is some work on
a new container that will make it easy to acquire and cache the gadget metadata
on the container side.  I'll change the code to use that when it becomes the
default, but not sure when that is planned.

[Javier Pedemonte, 2010-07-14 19:03:18]

Last patch: I found that some places within the code (in shindig-container.js
and rpc.js) assume that the relayUrl is going to be absolute.  But when running
from localhost, serverBase_ is set merely as "/gadgets/", causing some things to
fail (the 'pubsub' sample [not 'pubsub-2'] failed and callSameDomain() did not
function correctly).  Updated the code to have
shindig.IfrGadget.prototype.setServerBase() make the URL absolute if necessary.

[Javier Pedemonte, 2010-07-29 06:09:52]

The latest patch adds security features to the RPC layer, as described here:
http://wiki.opensocial.org/index.php?title=PubSub.next_Proposals#Secure_rpc.

[johnfargo, 2010-08-12 06:39:54]

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?

[Javier Pedemonte, 2010-08-13 03:01:52]

On 2010/08/12 06:39:54, johnfargo wrote:
> 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.

Will do.

> 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?

The pubsub-2 APIs are made accessible from a gadget instance object, in order to
make this code compatible with the use of inline gadgets.  This was discussed in
the past:
http://wiki.opensocial.org/index.php?title=PubSub.next_Proposals#Static-based...

> 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?

Well, I'm using "shindig-container" feature -- the new common container
conflicts, since both define "shindig.container" object.  When the common
container is ready, I'll move over to that, since this getting the gadget
metadata will be very easy.

> 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.

The commented-out line of code caused issues in my testing, so I need to remove
it.  The "parameters" variable is already initialized higher up, anyway.

> 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)

This is the gadget instance object, as mentioned above.

> RE: Style issues with OpenAjax files

Since these are external files, developed for another project, they were written
using a different style.  In general, comments follow the JSDoc style and
optional params are specified there.

> 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)?

Yes.

> 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:

There was a lengthy discussion on this topic.  In the end, it was decided to add
these two optional params for those developers that wish to use them.  Use of
closures works as well.

> 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?

Hehe, I remember having discussions on the above two topics, but I don't
remember the reasoning.

> 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.

The Hub spec defines that if scope isn't specified as an argument, it defaults
to "window".

> 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?

This bit of code is there to handle a very specific situation: A user creates an
application on top of OpenAjax Hub.  The Hub code itself uses Shindig RPC under
the covers, but puts the "gadgets" namespace under OpenAjax.gadgets.  Later, the
app dynamically loads an OpenSocial gadget, which requires loading the Shindig
libs.  But the Shindig libs will see that 'window.gadgets' doesn't exist and
create an object in that namespace.  There are now two different RPC related
objects ('OpenAjax.gadgets' and 'window.gadgets') and stuff breaks.

This code allows the user to state that they may dynamically load the Shindig
libs and therefore also creates the 'window.gadgets' namespace.  When Shindig is
loaded later, it sees the existing 'window.gadgets' and both the Hub and Shindig
use the same object for communication.

I can't use gadgets.config in the Hub code, since it can run without the rest of
Shindig and therefore won't have access to gadgets.config.

> 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?

Basically.  I decided to follow the lead of the "pubsub" feature, which exposed
a 'gadgets.pubsubrouter.init()' function.

> 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

The "shindig.random" feature is only available on the 'container' side, while
this code runs on the 'gadget' side.  Is it OK to also expose this API on
'gadget' side?

> 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.

I'm confused -- which method are you talking about?


I'll post an updated patch shortly...

[Paul Lindner, 2010-08-13 06:58:26]

Patch partially applied.

thanks!

Will go over the overlapping changes more closely tomorrow.

http://codereview.appspot.com/1247044/diff/55001/56014
File features/src/main/javascript/features/core.util/util.js (left):

http://codereview.appspot.com/1247044/diff/55001/56014#oldcode127
features/src/main/javascript/features/core.util/util.js:127: parameters = {};
lgtm, will commit with some mods

http://codereview.appspot.com/1247044/diff/55001/56014
File features/src/main/javascript/features/core.util/util.js (right):

http://codereview.appspot.com/1247044/diff/55001/56014#newcode369
features/src/main/javascript/features/core.util/util.js:369: 'byId' :
function(id) {
This is only used in pubsub-2, move to that namespace?

http://codereview.appspot.com/1247044/diff/55001/56015
File
features/src/main/javascript/features/shindig.container/shindig-container.js
(right):

http://codereview.appspot.com/1247044/diff/55001/56015#newcode805
features/src/main/javascript/features/shindig.container/shindig-container.js:805:
// OAAIfrGadget
remove commented code?

[Javier Pedemonte, 2010-08-13 18:15:06]

On 2010/08/13 06:58:26, Paul Lindner wrote:
> http://codereview.appspot.com/1247044/diff/55001/56014#newcode369
> features/src/main/javascript/features/core.util/util.js:369: 'byId' :
> function(id) {
> This is only used in pubsub-2, move to that namespace?

So, as previously mentioned, this was an attempt to lay the groundwork for
inline gadgets -- the idea being that an inline gadget would call
"gadgets.byId(__MODULE_ID__)" (where the string __MODULE_ID__ is replaced by the
appropriate ID on the server) and then use the gadget instance returned by that
function to make all the Shindig API calls.

But since inline gadgets is being handled in another bug, and since pubsub-2
would be the only one using this now, I think it'll be best to just switch
pubsub-2 from instance-based to static-based APIs.  Probably something off of
"gadgets.pubsub2" in order to avoid conflict with the existing "gadgets.pubsub".
 This'll make it easier to get this feature in for Shindig 2.0.0, and worry
about inline gadgets in the other bug, for a later release.  

I'll make those changes and try to have a patch up later today.

[Javier Pedemonte, 2010-08-17 21:09:05]

Re: Patch Set 7

I quickly redid the shindig-container.js patch, to refactor the new code and
make it look cleaner, more logical. This patch adds a dependency on
shindig.uri.ext, but that could easily be removed.

Also, the OAHub version should actually be 2.0.4.  Didn't want to upload a whole
new patch set just for that...

(Aside: Is there a way to add a detailed description when attaching a patch
using upload.py?  I know I can set the short description (title) using
"--message"; but "--description" only seems to work during issue create.)

[Javier Pedemonte, 2010-08-18 02:49:41]

"gadgets.pubsub2" isn't a good namespace.

It's tempting to re-use "gadgets.pubsub" from the original "pubsub" feature. 
The APIs are similar, but unfortunately, there are subtle differences --
"pubsub-2" isn't backwards compatible with "pubsub".

I propose using "gadgets.Hub".

[Han Nguyen, 2010-08-26 17:25:45]

On 2010/08/18 02:49:41, Javier Pedemonte wrote:
> "gadgets.pubsub2" isn't a good namespace.
> 
> It's tempting to re-use "gadgets.pubsub" from the original "pubsub" feature. 
> The APIs are similar, but unfortunately, there are subtle differences --
> "pubsub-2" isn't backwards compatible with "pubsub".
> 
> I propose using "gadgets.Hub".

"gadgets.Hub" sounds good, if there's no other suggestions by end of tomorrow, I
think we should go with it. 
Thanks,
Han

[Paul Lindner, 2010-08-27 12:08:03]

It's a no-go on re-using pubsub, a few people have already shipped and will
continue to support that namespace for the foreseeable future (eXo, others..)

gadgets.Hub seems fine to me.

[Javier Pedemonte, 2010-08-27 21:23:37]

Re: Patch Set 8 -- Patch for remaining files (not including RPC changes, which
are in different bug).  This patch is based on trunk, not on 2.0.x branch.

[Paul Lindner, 2010-08-27 21:26:17]

thanks, will get to this asap.


On Fri, Aug 27, 2010 at 2:23 PM, <jhpedemonte@gmail.com> wrote:

> Re: Patch Set 8 -- Patch for remaining files (not including RPC changes,
> which are in different bug).  This patch is based on trunk, not on 2.0.x
> branch.
>
>
>
> http://codereview.appspot.com/1247044/
>



-- 
Paul Lindner -- lindner@inuus.com -- linkedin.com/in/plindner

[WoodstaE, 2010-10-18 14:45:20]

Minor typo under #Topics
(http://opensocial-resources.googlecode.com/svn/spec/proposals/1.1/inter-gadge...):
speicificaiton should be specification.