> On 2011-12-22 20:34:52, Dan Dumont wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.gadget/gadget_site.js, > > line 419 > > <https://reviews.apache.org/r/1632/diff/4/?file=64701#file64701line419> > > > > Do you know why this is done in the loadingGadgetHolder?
Yeah -- it ends up getting used further down the line during osapi.container.GadgetHolder.prototype.render -- specifically in osapi.container.GadgetHolder.prototype.getIframeUrl_ (line 346 -- 348). Although I just noticed something else interesting that I hadn't noticed before -- it looks like the original authors of CC are actually already using the siteId as the moduleId... They just (apparently) forgot to also use it in the security tokens. If you look at lines 350 and 351 in gadget_holder.js you'll see that the siteId is used as the 'mid' parameter in the iframe url -- which means that the gadgets JS API code that deals with moduleId's will end up using the siteId as the moduleId (because the gadgets JS API parses the moduleId from the 'mid' parameter in the iframe url). However as we've been discussing the moduleId in the security token gets set to 0 -- so anything on the server side that pulls the moduleId from the security token will get the wrong value. You can actually see all this play out right now in shindig head by messing around with the common container sample page and adding a few gadgets. To see what I'm taking about -- using shindig head -- browse to the CC sample container, add the todo and horoscope gadgets (to use up a couple of siteId's) and then add the activities stream sample (to get a gadget that needs a security token) -- then using firefox right click the whitespace in the activities gadget and load it into its own tab (so you can easily see the requests it makes in firebug and execute code against the gadgets API in the context of that gadget). Then -- if you reload that tab, watch in firebug and look at the security token passed with the soacial API calls -- you'll see the moduleId in it is 0 -- but if you execute this code in the console: var prefs = new gadgets.Prefs(); console.log(prefs.getModuleId()); You'll get back 'gadget-site-2' So I guess this gives some precedent for using the siteId as the moduleId. As I've said before I personally think this would work out fine (using siteId as moduleId) and might be worth trying to start with and then changing later if we run into issues -- but in the end I dont think it makes much difference to me. There's also precedent for something similar elsewhere too (in both igoogle and rave and likely other places as well). If you look at igoogle for example you'll see that they wrap all their gadgets in a div which includes their moduleId (and in CC we use that wrapper div ID as the siteId). Ryan and I had a discussion on this a while back which might be worth looking over again -- here is the relevant piece: http://markmail.org/message/6fqyufoynv2q5q6a - Jesse ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1632/#review4085 ----------------------------------------------------------- On 2011-12-14 21:13:40, Jesse Ciancetta wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/1632/ > ----------------------------------------------------------- > > (Updated 2011-12-14 21:13:40) > > > Review request for shindig. > > > Summary > ------- > > Common container currently doesn't include the siteId (moduleId) in any of > it's security token processing/handling (all security tokens in common > container currently get minted with a moduleId of 0). > > This patch is a first (rough) cut at getting moduleId's into security tokens. > I am posting it somewhat prematurely to solicit feedback before I invest any > more time in finishing up the last bits. Here are the things that I know are > still left to do: > > -- Update unit tests on both the JS and Java side -- currently I've been > building and deploying with skipTests=true... > -- Figure out a strategy for dealing with preloaded gadgets. The current > auth-refresh process maintains tokens for preloaded gadgets, however the > preoad JS functions just take a gadgetUrl so there is no concept of a siteId > (moduleId) for them at this time. > -- Figure out how to get the token that is included in the original iframe to > include a moduleId. I think the token in the iframe likely comes back in the > metadata request (although I haven't looked yet to verify), which means that > the call to the metadata service would likely need to include the moduleId as > well. > > I'd greatly appreciate any comments people have on the patch and strategies > for dealing with the outstanding issues noted above. > > Thanks! > > > This addresses bugs RAVE-198 and SHINDIG-1611. > https://issues.apache.org/jira/browse/RAVE-198 > https://issues.apache.org/jira/browse/SHINDIG-1611 > > > Diffs > ----- > > > http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/commoncontainer/assembler.js > 1214246 > > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.gadget/gadget_site.js > 1214246 > > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/util.js > 1214246 > > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/container.js > 1214246 > > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/service.js > 1214246 > > http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/container/gadget_site_test.js > 1214246 > > http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/container/service_test.js > 1214246 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java > 1214246 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java > 1214246 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java > 1214246 > > Diff: https://reviews.apache.org/r/1632/diff > > > Testing > ------- > > > Thanks, > > Jesse > >
