http://codereview.appspot.com/3734041/diff/88001/java/common/conf/shindig.properties File java/common/conf/shindig.properties (right):
http://codereview.appspot.com/3734041/diff/88001/java/common/conf/shindig.properties#newcode162 java/common/conf/shindig.properties:162: #Maximum Get Url size limit Space after # http://codereview.appspot.com/3734041/diff/88001/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/88001/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java#newcode135 java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:135: if (nodes.isEmpty()) { Can we add a tiny test that exercises this check? http://codereview.appspot.com/3734041/diff/88001/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java#newcode169 java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:169: List<List<Element>> concatBatchesWithinLimit = Lists.newLinkedList(); Would concatBatchesWithinUrlLimit be more clear (though long) - currently the "limit" seems quite an ambiguous term to use. http://codereview.appspot.com/3734041/diff/88001/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java#newcode229 java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:229: * @param gadget Gadget object for the site under consideration site -> page? http://codereview.appspot.com/3734041/diff/88001/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java#newcode230 java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:230: * @param elem element for generating any random url. element -> Element Element for generating a sample Concat Uri. http://codereview.appspot.com/3734041/diff/88001/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java#newcode249 java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:249: * @param output elements list which are grouped together based on Url Max limit check. elements list -> List of elements http://codereview.appspot.com/3734041/diff/88001/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java#newcode250 java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:250: * @param urlOffset offset in Concat Uri apart from nodes uri's offset -> Offset (Always start comments with caps.) The definition of url offset (and even the term itself) is very vague in all your comments and variable/method names. Do you want to consider something along the lines of baseConcatUrlLength (because this is the length of the concat url leaving aside the lengths of the Uris of the concatenated nodes)? http://codereview.appspot.com/3734041/diff/88001/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java#newcode259 java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:259: int extraPadding = Integer.toString(index).length() + 2; Why do we need this extra padding? Is this to account for the &? http://codereview.appspot.com/3734041/diff/88001/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java#newcode264 java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:264: URL_LENGTH_BUFFER_MARGIN * ConcatUriManager.ConcatData.getMaxUrlLength()) { Since we don't expect this value to change during this method's execution, can we evaluate it outside the for loop? http://codereview.appspot.com/3734041/diff/88001/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java#newcode271 java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:271: if (urlOffset + url.length() + extraPadding > Could you clarify what this check helps us in? http://codereview.appspot.com/3734041/diff/88001/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java#newcode279 java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:279: if (index > 1) { Is the index mainly for knowing whether there are remaining nodes to be added to the output? If so, could we not check the size of batchWithMaximumLimit and decide whether to add to output or not? http://codereview.appspot.com/3734041/diff/88001/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java#newcode286 java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:286: * For each batch present in concatBatches return the respective Uri batches. For each batch of elements present in concatBatches, return the respective Uri batches. http://codereview.appspot.com/3734041/diff/88001/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java#newcode291 java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:291: private List<List<Uri>> geturiBatches(List<List<Element>> concatBatches) { geturiBatches -> getUriBatches http://codereview.appspot.com/3734041/diff/88001/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/88001/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java#newcode38 java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java:38: static final int URL_MAX_LENGTH = 2048; From your offline explanation, it seems that this is the DEFAULT_URL_MAX_LENGTH used if the shindig property for urlMaxLength is not defined. Can we name it as DEFAULT_URL_MAX_LENGTH for clarity? http://codereview.appspot.com/3734041/diff/88001/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) Space before and after = http://codereview.appspot.com/3734041/
