Update JsUriManager interface to use JsUri

Make JsUriManager interface symmetric - add convertion from JsUri to Uri

[zhoresh, 2010-11-04 21:34:26]

Syncing with head

[johnfargo, 2010-11-05 20:46:37]

http://codereview.appspot.com/2911041/diff/11001/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/JsServlet.java
(right):

http://codereview.appspot.com/2911041/diff/11001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/JsServlet.java:175:
uriBuilder.removeQueryParameter(Param.JSLOAD.getKey());
why remove this param?

http://codereview.appspot.com/2911041/diff/11001/java/gadgets/src/main/java/o...
File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/JsUriManager.java
(right):

http://codereview.appspot.com/2911041/diff/11001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/JsUriManager.java:80:
this.context = context;
nanonit: put libs init at the end for easier scanning of ctor differences (all
init'ed in the same order)

http://codereview.appspot.com/2911041/diff/11001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/JsUriManager.java:127:
String version(String gadgetUri, String container, Collection<String> extern);
I'm confused; these should always be Uris, with Uri validation occurring at a
higher level for closer tying of Uri handling to error handling

[zhoresh, 2010-11-05 21:56:05]

http://codereview.appspot.com/2911041/diff/11001/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/JsServlet.java
(right):

http://codereview.appspot.com/2911041/diff/11001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/JsServlet.java:175:
uriBuilder.removeQueryParameter(Param.JSLOAD.getKey());
On 2010/11/05 20:46:38, johnfargo wrote:
> why remove this param?

You get ito this function only when original request have jsload=1, but you want
result url to not have it (otherwise you will have infinite loop...)

http://codereview.appspot.com/2911041/diff/11001/java/gadgets/src/main/java/o...
File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/JsUriManager.java
(right):

http://codereview.appspot.com/2911041/diff/11001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/JsUriManager.java:127:
String version(String gadgetUri, String container, Collection<String> extern);
On 2010/11/05 20:46:38, johnfargo wrote:
> I'm confused; these should always be Uris, with Uri validation occurring at a
> higher level for closer tying of Uri handling to error handling

Two points, first why do we even need gadget for js load, especially for
container js?
Second, the reason I changed it to string is so it will match the type of gadget
used by the JsUri (Which is from ProxyUriBase)

[jtarrio, 2010-11-05 22:30:36]

lgtm

[johnfargo, 2010-11-08 23:20:09]

Previous nits generally still stand. LGTM other than this.

http://codereview.appspot.com/2911041/diff/11001/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/JsServlet.java
(right):

http://codereview.appspot.com/2911041/diff/11001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/JsServlet.java:175:
uriBuilder.removeQueryParameter(Param.JSLOAD.getKey());
Ah, duh. I'd propose a micronit to sanitize jsload in the "createJsloadScript"
method rather than here, but it really doesn't matter much.

On 2010/11/05 21:56:05, zhoresh wrote:
> On 2010/11/05 20:46:38, johnfargo wrote:
> > why remove this param?
> 
> You get ito this function only when original request have jsload=1, but you
want
> result url to not have it (otherwise you will have infinite loop...)

http://codereview.appspot.com/2911041/diff/11001/java/gadgets/src/main/java/o...
File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/JsUriManager.java
(right):

http://codereview.appspot.com/2911041/diff/11001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/JsUriManager.java:127:
String version(String gadgetUri, String container, Collection<String> extern);
We don't; it's mostly legacy for analytics/request context, which AFAIK is not
used server-side.

I'd still very minorly prefer Uri over String simply to prevent any existing
users from having to change their bindings/impls.

On 2010/11/05 21:56:05, zhoresh wrote:
> On 2010/11/05 20:46:38, johnfargo wrote:
> > I'm confused; these should always be Uris, with Uri validation occurring at
a
> > higher level for closer tying of Uri handling to error handling
> 
> Two points, first why do we even need gadget for js load, especially for
> container js?
> Second, the reason I changed it to string is so it will match the type of
gadget
> used by the JsUri (Which is from ProxyUriBase)