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 On 2011/01/25 10:13:52, anupama.dutta wrote:
Space after #
Done. 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()) { On 2011/01/25 10:13:52, anupama.dutta wrote:
Can we add a tiny test that exercises this check?
Done. 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(); On 2011/01/25 10:13:52, anupama.dutta wrote:
Would concatBatchesWithinUrlLimit be more clear (though long) -
currently the
"limit" seems quite an ambiguous term to use.
Done. 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 On 2011/01/25 10:13:52, anupama.dutta wrote:
site -> page?
Done. 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. On 2011/01/25 10:13:52, anupama.dutta wrote:
element -> Element Element for generating a sample Concat Uri.
Done. 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. On 2011/01/25 10:13:52, anupama.dutta wrote:
elements list -> List of elements
Done. 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 On 2011/01/25 10:13:52, anupama.dutta wrote:
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)?
Done. 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; On 2011/01/25 10:13:52, anupama.dutta wrote:
Why do we need this extra padding? Is this to account for the &?
Yes..It includes '&', '=' and numerical values used with every resource uri. 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()) { On 2011/01/25 10:13:52, anupama.dutta wrote:
Since we don't expect this value to change during this method's
execution, can
we evaluate it outside the for loop?
Done. 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 > On 2011/01/25 10:13:52, anupama.dutta wrote:
Could you clarify what this check helps us in?
Comment added. "Skip resource from concat if concat url length of individual node exceeds the max allowed url length." 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) { On 2011/01/25 10:13:52, anupama.dutta wrote:
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?
No, this is not the main reason. index helps also in concat url length. 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. On 2011/01/25 10:13:52, anupama.dutta wrote:
For each batch of elements present in concatBatches, return the
respective Uri
batches.
Done. 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) { On 2011/01/25 10:13:52, anupama.dutta wrote:
geturiBatches -> getUriBatches
Done. 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; On 2011/01/25 10:13:52, anupama.dutta wrote:
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?
Done. 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) On 2011/01/25 10:13:52, anupama.dutta wrote:
Space before and after =
Done. http://codereview.appspot.com/3734041/
