shindig-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Stanton Sievers" <siever...@gmail.com>
Subject Re: Review Request: gadgets.metadata request fails for gadgets that don't have a "default" view
Date Sat, 28 Jan 2012 15:17:28 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message