http://codereview.appspot.com/3734041/diff/127001/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java (right):
http://codereview.appspot.com/3734041/diff/127001/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java#newcode190 java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:190: for(Uri uri : concatUri.getUris()) { On 2011/02/28 08:35:06, anupama.dutta wrote:
Space before (
Done. http://codereview.appspot.com/3734041/diff/127001/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java (right): http://codereview.appspot.com/3734041/diff/127001/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java#newcode130 java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:130: uriBuilder = makeUriBuilder(ctx, concatHost, concatPath); On 2011/02/28 08:35:06, anupama.dutta wrote:
Move the newline on line 131 to before line 130, since we are trying
to
reinitialize everything starting from line 130.
Done. http://codereview.appspot.com/3734041/diff/127001/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java#newcode167 java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:167: if (versions != null && versions.size() > 0) { On 2011/02/28 08:35:06, anupama.dutta wrote:
Will versions.size() ever be greater than 1 here?
For correct versioner implementation, it can never be greater than 1. I changed this comparison from > 0 to == 1. http://codereview.appspot.com/3734041/diff/127001/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java#newcode182 java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:182: while((resourceUri = uri.getQueryParameter(i.toString())) != null) { On 2011/02/28 08:35:06, anupama.dutta wrote:
Space after while
Done. http://codereview.appspot.com/3734041/diff/127001/java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java File java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java (right): http://codereview.appspot.com/3734041/diff/127001/java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java#newcode309 java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java:309: public void dontConcatMultipleResourceBeyoundUrlLimit() throws Exception { On 2011/02/28 08:35:06, anupama.dutta wrote:
Yet to go through this test in detail, but does it assume that
splitJsEnabled is
true or false? Can we add tests for both scenarios because even in the
non-json
case, if there are enough adjacent js/css files, we can easily exceed
the GET
URL limit and ideally, these cases should be fixed by this patch.
Add another Test for null case. I also check the snippets value in case of json. http://codereview.appspot.com/3734041/
