----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3180/#review4291 -----------------------------------------------------------
Overall this looks good to me. I have some questions about setting the module id as the fragment, since I think there are cases where the fragment gets used for an rpctoken or an ifpctok... you can see it in the gadget_holder feature code. I'm not sure how/why this is used but it's certainly worth looking into, as I think your code right now would kill off that piece of the fragment. http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/container.js <https://reviews.apache.org/r/3180/#comment9667> I'm having doubts about setting the fragment to be the module id. I thought we already used the fragment for the rpctoken in some cases (for like IFPC??). Will this overwrite that piece if it's set on the url? Can you use the "setFP" API instead of "setFragment"? http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/constant.js <https://reviews.apache.org/r/3180/#comment9668> Whether for this review or not, I think the ServiceConfig constants, like API_HOST and API_PATH should be added here as well. - Stanton On 2012-01-09 21:18:06, Dan Dumont wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/3180/ > ----------------------------------------------------------- > > (Updated 2012-01-09 21:18:06) > > > Review request for shindig, Henry Saputra, Ryan Baxter, li xu, Jesse > Ciancetta, and Stanton Sievers. > > > Summary > ------- > > Initial review of 1st change. Allowing common container to manage container > token refreshes. Also, refresh of gadget security tokens will now wait for > valid container security token before trying to refresh. > > > Diffs > ----- > > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerTest.java > 1222407 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerServiceTest.java > 1222407 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ModuleIdManager.java > PRE-CREATION > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ModuleIdManagerImpl.java > PRE-CREATION > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java > 1222407 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java > 1222407 > > http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/container/gadget_holder_test.js > 1222407 > > http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/container/service_test.js > 1222407 > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/AbstractSecurityToken.java > 1222407 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java > 1222407 > > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/feature.xml > 1222407 > > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/service.js > 1222407 > > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/util.js > 1222407 > > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/container.js > 1222407 > > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/constant.js > 1222407 > > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/feature.xml > 1222407 > > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.gadget/gadget_site.js > 1222407 > > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.gadget/gadget_holder.js > 1222407 > > http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/commoncontainer/assembler.js > 1222407 > > Diff: https://reviews.apache.org/r/3180/diff > > > Testing > ------- > > Tested code in a private container with some examples of setting no refresh > (ttl = 0) and setting an initial token (if it was written by jsp page to > avoid transaction) etc.. > > > Thanks, > > Dan > >
