----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3670/#review4672 -----------------------------------------------------------
My biggest concern with this change is that it breaks backwards compatibility. There is no longer "iframeurl" on the metadata responses. This isn't a big deal if everyone is using the common container, since you've updated that portion, but if they aren't, then this will cause problems. Maybe not a big deal, as the metadata request/response is not spec'd. Something to consider and get more feedback on from the rest of the dev list. Also, have you tried this out with requestNavigateTo functionality or the view switching functionality (as in the sample common container)? With this patch the CC shouldn't have to re-issue any metadata requests because it has everything it needs. I want to make sure that is indeed the case. http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.site.gadget/gadget_holder.js <https://reviews.apache.org/r/3670/#comment10391> Can we strip out the gmodules language? I've never noticed it being here before. http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.site.gadget/gadget_holder.js <https://reviews.apache.org/r/3670/#comment10392> With your changes, uri is now the uri with render params and other query params set on it. Is that necessary for the patch? I'd like to not change this behavior if you don't have to. http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java <https://reviews.apache.org/r/3670/#comment10393> Trailing whitespace http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultIframeUriManager.java <https://reviews.apache.org/r/3670/#comment10394> Make this protected so that anyone can override this functionality. Prior to your changes they would have been able to do that by overriding makeRenderingUri(). - Stanton On 2012-01-28 01:17:22, Ryan Baxter wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/3670/ > ----------------------------------------------------------- > > (Updated 2012-01-28 01:17:22) > > > Review request for shindig. > > > Summary > ------- > > The request for gadgets.metadata fails for a gadget spec that doesn't specify > a "default" view. > > > This addresses bug SHINDIG-1549. > https://issues.apache.org/jira/browse/SHINDIG-1549 > > > Diffs > ----- > > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerTest.java > 1236884 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultIframeUriManagerTest.java > 1236884 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerServiceTest.java > 1236884 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/FakeIframeUriManager.java > 1236884 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/IframeUriManager.java > 1236884 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultIframeUriManager.java > 1236884 > > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.site.gadget/gadget_holder.js > 1236884 > > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/constant.js > 1236884 > > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/util.js > 1236884 > > http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/container/gadget_holder_test.js > 1236884 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java > 1236884 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java > 1236884 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java > 1236884 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/UriManagerTestBase.java > 1236884 > > Diff: https://reviews.apache.org/r/3670/diff > > > Testing > ------- > > Updated unit tests and tested in the various containers > > > Thanks, > > Ryan > >
