Fixing ConcatVisitor for cases where media type is "all", no media type and having title attribute.

ConcatVisitor Fixes for CSS.
  CSS Styles getting reordered due to following issues.
  1) specifying no media type is equivalent to media type "screen" and so they should not be batched into different batch.
  2) css link tags should not be batched across media type 'all'.
  3) css link tags should not be batched across link's with title attribute. (as browsers support choice to select among preferred css stylesheets)

[anupama.dutta, 2010-08-13 05:17:06]

Some very initial comments below.

http://codereview.appspot.com/1997041/diff/1/3
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java
(right):

http://codereview.appspot.com/1997041/diff/1/3#newcode148
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:148:
elemConcat.setAttribute(type.getSrcAttrib(), concatUri.getUri().toString());
Please either remove this change or add a test for it (after discussing with
Kuntal).

http://codereview.appspot.com/1997041/diff/1/3#newcode210
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:210:
if (cur.getNodeType() == Node.TEXT_NODE &&
StringUtils.isEmpty(cur.getTextContent().trim())) {
Add a test case for this.

[Kuntal Loya, 2010-08-13 06:01:09]

http://codereview.appspot.com/1997041/diff/1/3
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java
(right):

http://codereview.appspot.com/1997041/diff/1/3#newcode148
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:148:
elemConcat.setAttribute(type.getSrcAttrib(), concatUri.getUri().toString());
On 2010/08/13 05:17:07, anupama.dutta wrote:
> Please either remove this change or add a test for it (after discussing with
> Kuntal).

This change is required - tests will be needed for it though. Similarly for
trimming the textNode.value.

http://codereview.appspot.com/1997041/diff/1/3#newcode202
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:202:
elem.getAttribute("type").contains("text/css"));
"text/css".equalsIgnoreCase(elem.getAttribute("type"))

[satya3656, 2010-08-13 08:55:24]

http://codereview.appspot.com/1997041/diff/1/3
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java
(right):

http://codereview.appspot.com/1997041/diff/1/3#newcode148
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:148:
elemConcat.setAttribute(type.getSrcAttrib(), concatUri.getUri().toString());
Changed a url to have '&' in the test.

http://codereview.appspot.com/1997041/diff/1/3#newcode202
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:202:
elem.getAttribute("type").contains("text/css"));
On 2010/08/13 06:01:09, Kuntal Loya wrote:
> "text/css".equalsIgnoreCase(elem.getAttribute("type"))

Done.

http://codereview.appspot.com/1997041/diff/1/3#newcode210
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:210:
if (cur.getNodeType() == Node.TEXT_NODE &&
StringUtils.isEmpty(cur.getTextContent().trim())) {
On 2010/08/13 05:17:07, anupama.dutta wrote:
> Add a test case for this.

Added

[satya3656, 2010-08-13 08:56:01]

Made the changes

[satya3656, 2010-08-16 12:39:28]

Add one more small fix if there is a title attribute set.

[Kuntal Loya, 2010-08-17 08:28:59]

Good fix.
LGTM++

http://codereview.appspot.com/1997041/diff/16001/17001
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java
(right):

http://codereview.appspot.com/1997041/diff/16001/17001#newcode45
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java:45:
private static final String JS1_URL_STR = "http://one.com/foo.js";
I can't see the diffs properly, so not sure, but are you testing that the
rewritten URL for this case contains escaped '&' values.

http://codereview.appspot.com/1997041/diff/16001/17001#newcode122
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java:122:

Could you add one small test for 'title' attribute as well.

[satya3656, 2010-08-17 10:08:38]

Sync to head(r 986246) .

[satya3656, 2010-08-17 10:08:47]

http://codereview.appspot.com/1997041/diff/16001/17001
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java
(right):

http://codereview.appspot.com/1997041/diff/16001/17001#newcode45
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java:45:
private static final String JS1_URL_STR = "http://one.com/foo.js";
On 2010/08/17 08:28:59, Kuntal Loya wrote:
> I can't see the diffs properly, so not sure, but are you testing that the
> rewritten URL for this case contains escaped '&' values.

We are using the Uri Class in both places(which encodes the query params), so it
should work fine.

http://codereview.appspot.com/1997041/diff/16001/17001#newcode122
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java:122:

On 2010/08/17 08:28:59, Kuntal Loya wrote:
> Could you add one small test for 'title' attribute as well.

I am adding the title attribute in the test directly, so that i don't mess up
with other tests.

[Kuntal Loya, 2010-08-18 06:11:58]

LGTM

[Paul Lindner, 2010-08-18 19:23:43]

does this handle the case where one link specifies the default media (screen)
and the adjacent tag has no media attribute?

<link media="screen" ....>
<link ....>

[zhoresh, 2010-08-18 20:33:22]

http://codereview.appspot.com/1997041/diff/27001/28002
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java
(left):

http://codereview.appspot.com/1997041/diff/27001/28002#oldcode147
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:147:
elemConcat.setAttribute(type.getSrcAttrib(),
concatUri.getUri().toString().replace("&", "&"));
I think there was a discussion about this before. I think the url need to be
escaped here.

http://codereview.appspot.com/1997041/diff/27001/28002
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java
(right):

http://codereview.appspot.com/1997041/diff/27001/28002#newcode182
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:182:
Element next = element;
Not related to your fix, but why do even need this local variable?

http://codereview.appspot.com/1997041/diff/27001/28002#newcode201
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:201:
return ("stylesheet".equalsIgnoreCase(elem.getAttribute("rel")) &&
Interesting find, please add a test (one with no stylesheet, and one with wrong
type)

http://codereview.appspot.com/1997041/diff/27001/28002#newcode246
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:246:
if (currMediaType == nextMediaType || (currMediaType != "all" && nextMediaType
!= "all")) {
If I understand your comment, shouldn't it be:
(curr == next && curr != "all")
Wouldn't the test catch it? maybe I am missing something

[gagan.goku, 2010-08-19 13:52:34]

http://codereview.appspot.com/1997041/diff/27001/28002
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java
(right):

http://codereview.appspot.com/1997041/diff/27001/28002#newcode88
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:88:
List<List<Element>> concatBatches = Lists.newLinkedList();
This function is pretty contrived. Could we add some comments outlining how it
works and maybe a TODO to clean it up later.
Some of the confusion points are:
1) What does batch mean ? Is it an intermediate list of uri's or the final list
of uris meant to be concatenated.

http://codereview.appspot.com/1997041/diff/27001/28002#newcode241
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:241:
private boolean areSiblingsCompatible(Element current, Element next) {
plz rename to something like areSiblingsIndependent ?

http://codereview.appspot.com/1997041/diff/27001/28002#newcode246
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:246:
if (currMediaType == nextMediaType || (currMediaType != "all" && nextMediaType
!= "all")) {
use String.equals rather than ==, like: currMediaType.equals(nextMediaType)

[satya3656, 2010-08-19 19:31:21]

Hi,
Added more comments and changed few variables name to make the code more
readable.

http://codereview.appspot.com/1997041/diff/27001/28002
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java
(left):

http://codereview.appspot.com/1997041/diff/27001/28002#oldcode147
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:147:
elemConcat.setAttribute(type.getSrcAttrib(),
concatUri.getUri().toString().replace("&", "&"));
On 2010/08/18 20:33:23, zhoresh wrote:
> I think there was a discussion about this before. I think the url need to be
> escaped here.

Reverted it, but is there any reason why gadgets/proxy?.. urls are not have this
escape, and only the gadgets/concat?... urls are having it?

http://codereview.appspot.com/1997041/diff/27001/28002
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java
(right):

http://codereview.appspot.com/1997041/diff/27001/28002#newcode88
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:88:
List<List<Element>> concatBatches = Lists.newLinkedList();
On 2010/08/19 13:52:35, gagan.goku wrote:
> This function is pretty contrived. Could we add some comments outlining how it
> works and maybe a TODO to clean it up later.
> Some of the confusion points are:
> 1) What does batch mean ? Is it an intermediate list of uri's or the final
list
> of uris meant to be concatenated.
> 

Done.

http://codereview.appspot.com/1997041/diff/27001/28002#newcode182
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:182:
Element next = element;
On 2010/08/18 20:33:23, zhoresh wrote:
> Not related to your fix, but why do even need this local variable?

Removed.

http://codereview.appspot.com/1997041/diff/27001/28002#newcode201
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:201:
return ("stylesheet".equalsIgnoreCase(elem.getAttribute("rel")) &&
On 2010/08/18 20:33:23, zhoresh wrote:
> Interesting find, please add a test (one with no stylesheet, and one with
wrong
> type)

Done.

http://codereview.appspot.com/1997041/diff/27001/28002#newcode241
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:241:
private boolean areSiblingsCompatible(Element current, Element next) {
On 2010/08/19 13:52:35, gagan.goku wrote:
> plz rename to something like areSiblingsIndependent ?

I guess it is fine now with the extra comments.

http://codereview.appspot.com/1997041/diff/27001/28002#newcode246
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:246:
if (currMediaType == nextMediaType || (currMediaType != "all" && nextMediaType
!= "all")) {
On 2010/08/18 20:33:23, zhoresh wrote:
> If I understand your comment, shouldn't it be:
> (curr == next && curr != "all")
> Wouldn't the test catch it? maybe I am missing something

We also want to concat adjacent links with media="all".
Comment is bit misleading, updated it.

http://codereview.appspot.com/1997041/diff/27001/28002#newcode246
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:246:
if (currMediaType == nextMediaType || (currMediaType != "all" && nextMediaType
!= "all")) {
On 2010/08/19 13:52:35, gagan.goku wrote:
> use String.equals rather than ==, like: currMediaType.equals(nextMediaType)

Done.

[zhoresh, 2010-08-20 18:41:03]

lgtm, Thanks!
Some minor comments, change and I will commit.

http://codereview.appspot.com/1997041/diff/38001/32003
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java
(right):

http://codereview.appspot.com/1997041/diff/38001/32003#newcode110
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:110:
*    <link href="7.css" rel="stylesheet" type="text/css" media="screen">      
-- Node12
s/Node12/Node7/

http://codereview.appspot.com/1997041/diff/38001/32003#newcode113
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:113:
*    buckets - [ [ Node1, Node2, Node3 ], [ Node4, Node 5 ], [ Node6 ], [ Node7
]
Node6 and Node7 should be same group

http://codereview.appspot.com/1997041/diff/38001/32003#newcode273
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:273:
* Checks if the adjacent siblings can be put in the same bucket.
The name and comment got me confused.
What this function really do is to check both are "all" or not "all" and have
same title.
The fact that they are not "all" doesn't mean they are in compatible yet.
Maybe change "areMediaAllCompatible"

[gagan.goku, 2010-08-20 19:39:54]

http://codereview.appspot.com/1997041/diff/38001/32003
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java
(right):

http://codereview.appspot.com/1997041/diff/38001/32003#newcode110
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:110:
*    <link href="7.css" rel="stylesheet" type="text/css" media="screen">      
-- Node12
On 2010/08/20 18:41:03, zhoresh wrote:
> s/Node12/Node7/ 

What he means is that link nodes being processed by revisit need not be adjacent
to each other.

http://codereview.appspot.com/1997041/diff/38001/32003#newcode113
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:113:
*    buckets - [ [ Node1, Node2, Node3 ], [ Node4, Node 5 ], [ Node6 ], [ Node7
]
On 2010/08/20 18:41:03, zhoresh wrote:
> Node6 and Node7 should be same group

It should be Node12 here, and they should be in different buckets because they
are not adjacent and are not separated by just text nodes.

http://codereview.appspot.com/1997041/diff/38001/32003#newcode273
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:273:
* Checks if the adjacent siblings can be put in the same bucket.
On 2010/08/20 18:41:03, zhoresh wrote:
> The name and comment got me confused.
> What this function really do is to check both are "all" or not "all" and have
> same title.
> The fact that they are not "all" doesn't mean they are in compatible yet.
> Maybe change "areMediaAllCompatible"

Yeah function name is a bit confusing. 2 link elements are compatible if they
can be placed into the same bucket (1st level of splits when going over link
nodes).

[satya3656, 2010-08-24 09:47:13]

Addressing the comments

[satya3656, 2010-08-24 09:50:13]

http://codereview.appspot.com/1997041/diff/38001/32003
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java
(right):

http://codereview.appspot.com/1997041/diff/38001/32003#newcode273
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:273:
* Checks if the adjacent siblings can be put in the same bucket.
On 2010/08/20 18:41:03, zhoresh wrote:
> The name and comment got me confused.
> What this function really do is to check both are "all" or not "all" and have
> same title.
> The fact that they are not "all" doesn't mean they are in compatible yet.
> Maybe change "areMediaAllCompatible"

Changing it to "areSiblingsBucketable", and updated the comments accordingly.

[satya3656, 2010-08-24 11:51:34]

Few more comment chnages to make it more readable.

[satya3656, 2010-08-24 11:55:39]

Small change in comment.

[gagan.goku, 2010-08-24 12:13:08]

LGTM.

Very small nitpick:

http://codereview.appspot.com/1997041/diff/51001/52002
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java
(right):

http://codereview.appspot.com/1997041/diff/51001/52002#newcode102
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:102:
* Example: Assume we have the following node list. (all have same parent)
Could you also add that "Node6 and Node12 have non "link" nodes in between)
Or better yet, rename Node12 to Node8 and say that Node7 is an empty text node
and hence did not come to revisit()

[satya3656, 2010-08-24 12:39:41]

Addressed the comment.

[satya3656, 2010-08-24 12:40:25]

http://codereview.appspot.com/1997041/diff/51001/52002
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java
(right):

http://codereview.appspot.com/1997041/diff/51001/52002#newcode102
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:102:
* Example: Assume we have the following node list. (all have same parent)
On 2010/08/24 12:13:08, gagan.goku wrote:
> Could you also add that "Node6 and Node12 have non "link" nodes in between)
> Or better yet, rename Node12 to Node8 and say that Node7 is an empty text node
> and hence did not come to revisit()
> 

Done.

[satya3656, 2010-08-24 12:41:52]

Sma

[satya3656, 2010-08-24 12:43:13]

Addressed the comment.

[Kuntal Loya, 2010-08-25 08:50:52]

minor documentation comments -

http://codereview.appspot.com/1997041/diff/59001/35003
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java
(right):

http://codereview.appspot.com/1997041/diff/59001/35003#newcode123
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:123:
*/
Good job with the documentation, really explains the whole idea.

But now it looks like revisit is just revisiting links :)

http://codereview.appspot.com/1997041/diff/59001/35003#newcode125
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:125:
// Collate Elements into batches to be concatenated.
Collate Elements into buckets to be concatenated

http://codereview.appspot.com/1997041/diff/59001/35003#newcode146
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:146:
// Split the existing batches based on media types.
Split the buckets into concat batches based on media types.

http://codereview.appspot.com/1997041/diff/59001/35003#newcode205
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:205:
return true;
Remove extra space.

http://codereview.appspot.com/1997041/diff/59001/35003#newcode236
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:236:
// rel="stylesheet" and type="css" also required.
and type="text/css"

[satya3656, 2010-08-25 09:35:07]

Done

http://codereview.appspot.com/1997041/diff/59001/35003
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java
(right):

http://codereview.appspot.com/1997041/diff/59001/35003#newcode123
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:123:
*/
On 2010/08/25 08:50:52, Kuntal Loya wrote:
> Good job with the documentation, really explains the whole idea.
> 
> But now it looks like revisit is just revisiting links :)
I guess it is ok, as it is mentioned above that link tags with same media type
within a bucket will be concatenated.

http://codereview.appspot.com/1997041/diff/59001/35003#newcode125
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:125:
// Collate Elements into batches to be concatenated.
On 2010/08/25 08:50:52, Kuntal Loya wrote:
> Collate Elements into buckets to be concatenated

Done.

http://codereview.appspot.com/1997041/diff/59001/35003#newcode146
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:146:
// Split the existing batches based on media types.
On 2010/08/25 08:50:52, Kuntal Loya wrote:
> Split the buckets into concat batches based on media types.

Done.

http://codereview.appspot.com/1997041/diff/59001/35003#newcode205
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:205:
return true;
On 2010/08/25 08:50:52, Kuntal Loya wrote:
> Remove extra space.

Done.

http://codereview.appspot.com/1997041/diff/59001/35003#newcode236
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:236:
// rel="stylesheet" and type="css" also required.
On 2010/08/25 08:50:52, Kuntal Loya wrote:
> and type="text/css"

Done.

[satya3656, 2010-08-25 09:35:46]

Addressing the comments

[zhoresh, 2010-08-25 17:48:24]

lgtm, committed at r989253

[gagan.goku, 2010-08-27 15:43:45]

You should close this issue now that it has been submitted.