Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(251)

Issue 2370041: Add support for &jsload=1 to JsServlet to load versioned JS. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
15 years, 5 months ago by mhermanto
Modified:
15 years, 5 months ago
Reviewers:
johnfargo, dev-remailer
Base URL:
http://svn.apache.org/repos/asf/shindig/trunk/
Visibility:
Public.

Description

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

Patch Set 1 : Update patch #

Total comments: 10

Patch Set 2 : Update patch #

Unified diffs Side-by-side diffs Delta from patch set Stats (+511 lines, -105 lines) Patch
java/common/src/test/java/org/apache/shindig/common/servlet/HttpServletResponseRecorder.java View 1 2 chunks +2 lines, -2 lines 0 comments Download
java/gadgets/src/main/java/org/apache/shindig/gadgets/DefaultGuiceModule.java View 1 chunk +3 lines, -0 lines 0 comments Download
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingGadgetRewriter.java View 2 chunks +4 lines, -2 lines 0 comments Download
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/JsServlet.java View 1 7 chunks +143 lines, -16 lines 0 comments Download
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultJsUriManager.java View 1 5 chunks +14 lines, -15 lines 0 comments Download
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/JsUriManager.java View 1 3 chunks +37 lines, -10 lines 0 comments Download
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ProxyUriBase.java View 1 3 chunks +9 lines, -4 lines 0 comments Download
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriCommon.java View 1 2 chunks +8 lines, -4 lines 0 comments Download
java/gadgets/src/test/java/org/apache/shindig/gadgets/render/RenderingGadgetRewriterTest.java View 1 21 chunks +30 lines, -30 lines 0 comments Download
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/JsServletTest.java View 1 1 chunk +167 lines, -0 lines 0 comments Download
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultJsUriManagerTest.java View 1 8 chunks +22 lines, -22 lines 0 comments Download
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/JsUriManagerTest.java View 1 chunk +72 lines, -0 lines 0 comments Download

Messages

Total messages: 7
mhermanto
15 years, 5 months ago (2010-10-05 04:22:21 UTC) #1
mhermanto
Update patch
15 years, 5 months ago (2010-10-11 22:16:30 UTC) #2
johnfargo
Looks great. Several suggestions on the core impl but nothing too huge. Thanks for all ...
15 years, 5 months ago (2010-10-11 22:48:35 UTC) #3
johnfargo
http://codereview.appspot.com/2370041/diff/3001/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/JsServlet.java 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/org/apache/shindig/gadgets/servlet/JsServlet.java#newcode54 java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/JsServlet.java:54: static final String ONLOAD_ERROR = "jsload must require onload"; ...
15 years, 5 months ago (2010-10-11 22:48:55 UTC) #4
mhermanto
Update patch
15 years, 5 months ago (2010-10-12 00:42:04 UTC) #5
mhermanto
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/org/apache/shindig/gadgets/servlet/JsServlet.java > ...
15 years, 5 months ago (2010-10-12 00:49:37 UTC) #6
johnfargo
15 years, 5 months ago (2010-10-12 19:35:09 UTC) #7
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b