> On 2012-01-28 15:17:28, Stanton Sievers wrote:
> > 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.
RE backwards compatibility
I realize this. Since this is the first release of Shindig with the common container I think
it is fine to break the "API". However if someone have a need for that functionality I can
add it back in.
RE requestNavigateTo
Even before this change we only ever made 1 request to get the metadata for the gadget. When
switching views we used the cached metadata. This was part of the problem. The cache is
indexed by gadget URL so switching views and / or subsequent calls to requestNavigateTo always
resulted in using the cached metadata.
> On 2012-01-28 15:17:28, Stanton Sievers wrote:
> > http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.site.gadget/gadget_holder.js,
line 169
> > <https://reviews.apache.org/r/3670/diff/3/?file=71190#file71190line169>
> >
> > 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.
It was not necessary, but it seemed odd to me that we were not using the actual URL in the
iFrame when setting up relay uri. If getIframeUrl_ changed the URL in some way that would
effect setting up RPC then we might be in trouble. I figured it would be best to use the
same URL that the iFrame will contain.
- Ryan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3670/#review4672
-----------------------------------------------------------
On 2012-01-30 15:29:59, Ryan Baxter wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3670/
> -----------------------------------------------------------
>
> (Updated 2012-01-30 15:29:59)
>
>
> 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/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/main/java/org/apache/shindig/gadgets/uri/DefaultIframeUriManager.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/test/java/org/apache/shindig/gadgets/servlet/FakeIframeUriManager.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/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/uri/UriManagerTestBase.java
1236884
>
> Diff: https://reviews.apache.org/r/3670/diff
>
>
> Testing
> -------
>
> Updated unit tests and tested in the various containers
>
>
> Thanks,
>
> Ryan
>
>