Some comments on the alternate approach.
http://codereview.appspot.com/3734041/diff/108001/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/108001/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java#newcode187 java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:187: // Regardless what happens, inject a copy of the first node, Update the comment to indicat that we inject as many copies of the first node as needed, with the new (concat) URIs. http://codereview.appspot.com/3734041/diff/108001/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java (right): http://codereview.appspot.com/3734041/diff/108001/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java#newcode40 java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java:40: @Inject(optional = true) Fix indentation on this line and the next. http://codereview.appspot.com/3734041/diff/108001/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java#newcode127 java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java:127: /* Remove commented block. http://codereview.appspot.com/3734041/diff/108001/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/108001/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java#newcode132 java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:132: uriBuilders.add(uriBuilder); Most of the code here (from line 132 - 139) are repititions from the block before the for loop. Could we remove the code from outside the for loop and bring the first iteration in here itself? http://codereview.appspot.com/3734041/diff/108001/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java#newcode153 java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:153: if(batchUris != null && uriBuilder != ctx.makeQueryParams(null, null)) { Space before ( http://codereview.appspot.com/3734041/diff/108001/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java#newcode166 java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:166: for(UriBuilder cocnatUriBuilder : uriBuilders) { cocnatUriBuilder -> concatUriBuilder http://codereview.appspot.com/3734041/diff/108001/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java#newcode168 java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:168: if(version != null) { Indentation off and space needed before ( http://codereview.appspot.com/3734041/diff/108001/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/108001/java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java#newcode140 java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java:140: assertNull(uri.getUris().get(0).getScheme()); I think you should also assert that the size of getUris() is exactly 1 before each of these blocks? Similar asserts for size of getUris() might be needed in other tests as well. http://codereview.appspot.com/3734041/
