Add support for &jsload=1 to JsServlet to load versioned JS.

This is augmented/completed from http://codereview.appspot.com/1687043/

[mhermanto, 2010-10-11 22:16:30]

Update patch

[johnfargo, 2010-10-11 22:48:35]

Looks great. Several suggestions on the core impl but nothing too huge. Thanks
for all the tests!

[johnfargo, 2010-10-11 22:48:55]

http://codereview.appspot.com/2370041/diff/3001/java/gadgets/src/main/java/or...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/JsServlet.java
(right):

http://codereview.appspot.com/2370041/diff/3001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/JsServlet.java:54:
static final String ONLOAD_ERROR = "jsload must require onload";
recommend variable name JSLOAD_ONLOAD_ERROR, with text "jsload requires onload
parameter"

http://codereview.appspot.com/2370041/diff/3001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/JsServlet.java:57:
static final String NOCACHE_ERROR = "jsload must not require nocache";
similar.

http://codereview.appspot.com/2370041/diff/3001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/JsServlet.java:69:
"document.write('<script type=\"text/javascript\" src=\"%s\"></script>');" +
split up <script and </script> just to be sure:
"document.write('<scr'+'ipt...
and
'</scr'+'ipt>'

http://codereview.appspot.com/2370041/diff/3001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/JsServlet.java:80:
static class HttpUtilDelegator {
this pattern calls into question whether we should consider removing the "static
cling" from HttpUtil (and perhaps ServletUtil et al).

But for now, it just sets caching headers. Recommend naming it "CachingSetter"
or similar.

http://codereview.appspot.com/2370041/diff/3001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/JsServlet.java:105:
public void setJsloadTimeoutSecs(@Named("shindig.jsload.timeout-secs") int
jsloadTimeoutSecs) {
I think I named this originally, so maybe I can change it :) let's call this
shindig.jsload.ttl-secs

http://codereview.appspot.com/2370041/diff/3001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/JsServlet.java:171:
// Require users to specify &onload=. This is a reliable way to ensuer
synchronous
nit: s/enseur/ensure

http://codereview.appspot.com/2370041/diff/3001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/JsServlet.java:172:
// execution. IE asynchronously loads script, before it loads source-scripted
and
pedantic note: this ensures reliable continuation moreso than synchronous
execution

http://codereview.appspot.com/2370041/diff/3001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/JsServlet.java:179:
// Also, require to not specify &nocache=. This is merely invalidated here.
a little confused about this -- why's that? we can just tack on nocache to the
downstream URL, and emit the loader JS with no-cache headers as well.

http://codereview.appspot.com/2370041/diff/3001/java/gadgets/src/main/java/or...
File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ProxyUriBase.java
(right):

http://codereview.appspot.com/2370041/diff/3001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ProxyUriBase.java:27:
import org.apache.shindig.config.ContainerConfig;
chunk mismatch on this file -- could you resync and merge?

http://codereview.appspot.com/2370041/diff/3001/java/gadgets/src/test/java/or...
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/JsServletTest.java
(right):

http://codereview.appspot.com/2370041/diff/3001/java/gadgets/src/test/java/or...
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/JsServletTest.java:65:
expect(request.getQueryString()).andReturn(null);
we might consider adding a TODO to abstract this out into a helper function
associated with the Uri class.

[mhermanto, 2010-10-12 00:42:04]

Update patch

[mhermanto, 2010-10-12 00:49:37]

On Mon, Oct 11, 2010 at 3:48 PM, <johnfargo@gmail.com> wrote:

>
>
>
http://codereview.appspot.com/2370041/diff/3001/java/gadgets/src/main/java/or...
> File
>
>
> java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/JsServlet.java
> (right):
>
>
>
http://codereview.appspot.com/2370041/diff/3001/java/gadgets/src/main/java/or...
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/JsServlet.java:54:
> static final String ONLOAD_ERROR = "jsload must require onload";
> recommend variable name JSLOAD_ONLOAD_ERROR, with text "jsload requires
> onload parameter"
>

Done.


>
>
http://codereview.appspot.com/2370041/diff/3001/java/gadgets/src/main/java/or...
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/JsServlet.java:57:
> static final String NOCACHE_ERROR = "jsload must not require nocache";
> similar.
>

Done.


>
>
>
http://codereview.appspot.com/2370041/diff/3001/java/gadgets/src/main/java/or...
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/JsServlet.java:69:
> "document.write('<script type=\"text/javascript\"
> src=\"%s\"></script>');" +
> split up <script and </script> just to be sure:
> "document.write('<scr'+'ipt...
> and
> '</scr'+'ipt>'
>

Done.  Also commented for this not-so-obvious reason.

>
>
>
http://codereview.appspot.com/2370041/diff/3001/java/gadgets/src/main/java/or...
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/JsServlet.java:80:
> static class HttpUtilDelegator {
> this pattern calls into question whether we should consider removing the
> "static cling" from HttpUtil (and perhaps ServletUtil et al).
>
> But for now, it just sets caching headers. Recommend naming it
> "CachingSetter" or similar.
>

Done.


>
>
http://codereview.appspot.com/2370041/diff/3001/java/gadgets/src/main/java/or...
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/JsServlet.java:105:
> public void setJsloadTimeoutSecs(@Named("shindig.jsload.timeout-secs")
> int jsloadTimeoutSecs) {
> I think I named this originally, so maybe I can change it :) let's call
> this shindig.jsload.ttl-secs
>

Done. Renamed other things (methods, members) correspondingly.


>
>
http://codereview.appspot.com/2370041/diff/3001/java/gadgets/src/main/java/or...
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/JsServlet.java:171:
> // Require users to specify &onload=. This is a reliable way to ensuer
> synchronous
> nit: s/enseur/ensure
>

Done.


>
>
http://codereview.appspot.com/2370041/diff/3001/java/gadgets/src/main/java/or...
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/JsServlet.java:172:
> // execution. IE asynchronously loads script, before it loads
> source-scripted and
> pedantic note: this ensures reliable continuation moreso than
> synchronous execution
>
>
Done. Comment adjusted.


>
>
http://codereview.appspot.com/2370041/diff/3001/java/gadgets/src/main/java/or...
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/JsServlet.java:179:
> // Also, require to not specify &nocache=. This is merely invalidated
> here.
> a little confused about this -- why's that? we can just tack on nocache
> to the downstream URL, and emit the loader JS with no-cache headers as
> well.
>

Done. Upon presence of &nocache= --
- loader will have ttl=0.
- downstream URL will have &nocache=1.

All reflected in tests.


>
>
http://codereview.appspot.com/2370041/diff/3001/java/gadgets/src/main/java/or...
> File
>
> java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ProxyUriBase.java
> (right):
>
>
>
http://codereview.appspot.com/2370041/diff/3001/java/gadgets/src/main/java/or...
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ProxyUriBase.java:27:
> import org.apache.shindig.config.ContainerConfig;
> chunk mismatch on this file -- could you resync and merge?
>

Done.


>
>
>
http://codereview.appspot.com/2370041/diff/3001/java/gadgets/src/test/java/or...
> File
>
>
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/JsServletTest.java
> (right):
>
>
>
http://codereview.appspot.com/2370041/diff/3001/java/gadgets/src/test/java/or...
>
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/JsServletTest.java:65:
> expect(request.getQueryString()).andReturn(null);
> we might consider adding a TODO to abstract this out into a helper
> function associated with the Uri class.
>
> Done. TODO'ed.


>
> http://codereview.appspot.com/2370041/
>

Re-upload patch for further review.

[johnfargo, 2010-10-12 19:35:09]

LGTM++

On 2010/10/12 00:49:37, mhermanto wrote:
> On Mon, Oct 11, 2010 at 3:48 PM, <mailto:johnfargo@gmail.com> wrote:
> 
> >
> >
> >
>
http://codereview.appspot.com/2370041/diff/3001/java/gadgets/src/main/java/or...
> > File
> >
> >
> > java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/JsServlet.java
> > (right):
> >
> >
> >
>
http://codereview.appspot.com/2370041/diff/3001/java/gadgets/src/main/java/or...
> >
> >
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/JsServlet.java:54:
> > static final String ONLOAD_ERROR = "jsload must require onload";
> > recommend variable name JSLOAD_ONLOAD_ERROR, with text "jsload requires
> > onload parameter"
> >
> 
> Done.
> 
> 
> >
> >
>
http://codereview.appspot.com/2370041/diff/3001/java/gadgets/src/main/java/or...
> >
> >
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/JsServlet.java:57:
> > static final String NOCACHE_ERROR = "jsload must not require nocache";
> > similar.
> >
> 
> Done.
> 
> 
> >
> >
> >
>
http://codereview.appspot.com/2370041/diff/3001/java/gadgets/src/main/java/or...
> >
> >
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/JsServlet.java:69:
> > "document.write('<script type=\"text/javascript\"
> > src=\"%s\"></script>');" +
> > split up <script and </script> just to be sure:
> > "document.write('<scr'+'ipt...
> > and
> > '</scr'+'ipt>'
> >
> 
> Done.  Also commented for this not-so-obvious reason.
> 
> >
> >
> >
>
http://codereview.appspot.com/2370041/diff/3001/java/gadgets/src/main/java/or...
> >
> >
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/JsServlet.java:80:
> > static class HttpUtilDelegator {
> > this pattern calls into question whether we should consider removing the
> > "static cling" from HttpUtil (and perhaps ServletUtil et al).
> >
> > But for now, it just sets caching headers. Recommend naming it
> > "CachingSetter" or similar.
> >
> 
> Done.
> 
> 
> >
> >
>
http://codereview.appspot.com/2370041/diff/3001/java/gadgets/src/main/java/or...
> >
> >
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/JsServlet.java:105:
> > public void setJsloadTimeoutSecs(@Named("shindig.jsload.timeout-secs")
> > int jsloadTimeoutSecs) {
> > I think I named this originally, so maybe I can change it :) let's call
> > this shindig.jsload.ttl-secs
> >
> 
> Done. Renamed other things (methods, members) correspondingly.
> 
> 
> >
> >
>
http://codereview.appspot.com/2370041/diff/3001/java/gadgets/src/main/java/or...
> >
> >
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/JsServlet.java:171:
> > // Require users to specify &onload=. This is a reliable way to ensuer
> > synchronous
> > nit: s/enseur/ensure
> >
> 
> Done.
> 
> 
> >
> >
>
http://codereview.appspot.com/2370041/diff/3001/java/gadgets/src/main/java/or...
> >
> >
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/JsServlet.java:172:
> > // execution. IE asynchronously loads script, before it loads
> > source-scripted and
> > pedantic note: this ensures reliable continuation moreso than
> > synchronous execution
> >
> >
> Done. Comment adjusted.
> 
> 
> >
> >
>
http://codereview.appspot.com/2370041/diff/3001/java/gadgets/src/main/java/or...
> >
> >
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/JsServlet.java:179:
> > // Also, require to not specify &nocache=. This is merely invalidated
> > here.
> > a little confused about this -- why's that? we can just tack on nocache
> > to the downstream URL, and emit the loader JS with no-cache headers as
> > well.
> >
> 
> Done. Upon presence of &nocache= --
> - loader will have ttl=0.
> - downstream URL will have &nocache=1.
> 
> All reflected in tests.
> 
> 
> >
> >
>
http://codereview.appspot.com/2370041/diff/3001/java/gadgets/src/main/java/or...
> > File
> >
> > java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ProxyUriBase.java
> > (right):
> >
> >
> >
>
http://codereview.appspot.com/2370041/diff/3001/java/gadgets/src/main/java/or...
> >
> >
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ProxyUriBase.java:27:
> > import org.apache.shindig.config.ContainerConfig;
> > chunk mismatch on this file -- could you resync and merge?
> >
> 
> Done.
> 
> 
> >
> >
> >
>
http://codereview.appspot.com/2370041/diff/3001/java/gadgets/src/test/java/or...
> > File
> >
> >
> >
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/JsServletTest.java
> > (right):
> >
> >
> >
>
http://codereview.appspot.com/2370041/diff/3001/java/gadgets/src/test/java/or...
> >
> >
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/JsServletTest.java:65:
> > expect(request.getQueryString()).andReturn(null);
> > we might consider adding a TODO to abstract this out into a helper
> > function associated with the Uri class.
> >
> > Done. TODO'ed.
> 
> 
> >
> > http://codereview.appspot.com/2370041/
> >
> 
> Re-upload patch for further review.