----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1632/#review4074 -----------------------------------------------------------
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.gadget/gadget_site.js <https://reviews.apache.org/r/1632/#comment9173> Hmm. Ok our conversation at panera is staring to come back to me. My understanding of the code is that moduleid != siteid I could be wrong here, but everything in the code down to the expected data types for each within the token tell me they are different. I'm not sure who the original author of this stuff is, so we may never know for sure. That said. I do think that siteid is a more... container related bit of information, and moduleid is more of a backend-data bit of information. It would make sense to me that a site's id is the site id, but that a site *could* be associated with a module id (if persisted) but it's not necessary. I don't think I would want to enforce that the transport for a module id is in the id of the site. Since I do have plans to realign GadgetSite and UrlSite down the road. Perhaps we can tweak the constructor of a site or something to make this more straight-forward. At the very least, I would suggest that we not pass in the id to the siteIdExtractor() but instead just pass the currentGadgetEl element. Perhaps also rename this function to moduleIdExtractor. That way when creating the site, a container could append a data-module-id attribute to the element for specific use as the moduleid - Dan 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 > >
