Enabling single resource concatenation

Single JS/CSS resources should be able to concatenate with themselves. This behavior should be similar to ProxyingVisitor proxying single JS/CSS resources.

This will be useful in cases when ScriptConcatContentRewriter and/or StyleConcatContentRewriter are used along with ProxyingContentRewriter (without the ConcatVisitors and not proxying JS/CSS)

[gagan.goku, 2010-09-13 09:54:57]

Neat cl :)
Took a quick look at it and the code looks nice. Just wondering if we need to
add another param to Config, but from first impression its the right change to
make.

Will take another look later.

[gagan.goku, 2010-09-17 16:21:54]

You write awesome tests :) [ which basically means i have not gone through all
of the test cases :) ]
Neat cl. LGTM. Please address small comments and send out to dev@

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

http://codereview.appspot.com/2164045/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:61:
private final boolean singleResource;
singleResourceConcat sounds better ?

http://codereview.appspot.com/2164045/diff/1/java/gadgets/src/main/java/org/a...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ContentRewriterFeature.java
(right):

http://codereview.appspot.com/2164045/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ContentRewriterFeature.java:156:
private final boolean enableSingleResource;
enableSingleResourceConcatenation ? if you do change the variable name, do
update other usages in this file.

http://codereview.appspot.com/2164045/diff/1/java/gadgets/src/test/java/org/a...
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java
(right):

http://codereview.appspot.com/2164045/diff/1/java/gadgets/src/test/java/org/a...
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java:364:
Uri concatUri = Uri.parse(StringUtils.replace(concatNode.getAttribute("href"),
"&", "&"));
this might not be necessary anymore as Ziv submitted the & change

[Kuntal Loya, 2010-09-21 11:22:22]

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

http://codereview.appspot.com/2164045/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:61:
private final boolean singleResource;
On 2010/09/17 16:21:54, gagan.goku wrote:
> singleResourceConcat sounds better ?

Done.

http://codereview.appspot.com/2164045/diff/1/java/gadgets/src/main/java/org/a...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ContentRewriterFeature.java
(right):

http://codereview.appspot.com/2164045/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ContentRewriterFeature.java:156:
private final boolean enableSingleResource;
On 2010/09/17 16:21:54, gagan.goku wrote:
> enableSingleResourceConcatenation ? if you do change the variable name, do
> update other usages in this file.

Done.

http://codereview.appspot.com/2164045/diff/1/java/gadgets/src/test/java/org/a...
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java
(right):

http://codereview.appspot.com/2164045/diff/1/java/gadgets/src/test/java/org/a...
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java:364:
Uri concatUri = Uri.parse(StringUtils.replace(concatNode.getAttribute("href"),
"&", "&"));
On 2010/09/17 16:21:54, gagan.goku wrote:
> this might not be necessary anymore as Ziv submitted the & change

Done.

[Kuntal Loya, 2010-10-01 10:08:48]

Ping.

[gagan.goku, 2010-10-03 19:17:28]

small nitpicks related to tests.
Will commit once you address these

http://codereview.appspot.com/2164045/diff/7001/java/gadgets/src/main/java/or...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ContentRewriterFeature.java
(right):

http://codereview.appspot.com/2164045/diff/7001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ContentRewriterFeature.java:138:
@Named("shindig.content-rewrite.enable-single-resource-concat")boolean
enableSingleResourceConcatenation) {
line length

http://codereview.appspot.com/2164045/diff/7001/java/gadgets/src/test/java/or...
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java
(right):

http://codereview.appspot.com/2164045/diff/7001/java/gadgets/src/test/java/or...
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java:132:
assertEquals(VisitStatus.BYPASS, getVisitStatusJs(js1, null, false, false));
You could create another method which takes the singleResourceConcat parameter,
while keeping the current getVisitStatusJs() method signature as is [ implicit
singleResourceConcat = false ] That way the diffs would be fewer.

But your approach makes things clearer, so its okay if you feel against the 2
method approach.

http://codereview.appspot.com/2164045/diff/7001/java/gadgets/src/test/java/or...
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java:228:
assertEquals(VisitStatus.RESERVE_NODE, getVisitStatusCss(node, null, true));
this should ideally be moved out to another test case, something like:
visitCssSeperatedByNonEmptyTextNodeWhenSingleConcatEnabled

http://codereview.appspot.com/2164045/diff/7001/java/gadgets/src/test/java/or...
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java:232:
public void dontVisitRelFreeCss() throws Exception {
I know this is not related to this codereview, but could you also rename this
method to:
dontVisitCssWithoutRelAttribute

http://codereview.appspot.com/2164045/diff/7001/java/gadgets/src/test/java/or...
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java:240:
public void dontVisitTypeCssFreeCss() throws Exception {
I know this is not related to this codereview, but could you also rename this
method to
dontVisitCssWithoutTypeAttribute.

http://codereview.appspot.com/2164045/diff/7001/java/gadgets/src/test/java/or...
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java:265:
Node node = elem("link", "href", CSS1_URL_STR);
This test is kind of redundant as we have tested that css is ignored when either
the type or rel attribute is missing (we have covered both code paths so this
one is kind of implied)

http://codereview.appspot.com/2164045/diff/7001/java/gadgets/src/test/java/or...
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java:324:
assertEquals(CONCAT_BASE_URI.getScheme(), concatUri.getScheme());
you could check the entire thing in 1 shot, it would be more readable as well:
assertEquals("//blah/gadgets/concat?1= ..." , concatUri.toString())

http://codereview.appspot.com/2164045/diff/7001/java/gadgets/src/test/java/or...
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java:370:
Uri concatUri = Uri.parse(concatNode.getAttribute("href"));
same comment as above.

http://codereview.appspot.com/2164045/diff/7001/java/gadgets/src/test/java/or...
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java:495:
List<Node> fullListJs = Lists.newArrayList();
You can make this a 1 liner:
List<Node> jsList = ImmutableList.of(seqNodes(js1, js2), seqNodes(js3));

http://codereview.appspot.com/2164045/diff/7001/java/gadgets/src/test/java/or...
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java:512:
assertEquals(CONCAT_BASE_URI.getScheme(), concatUri1.getScheme());
something like:
assertEquals("//example.org/gadgets/concat?1=..&2=..&...", concatUri.toString())
might be more readable.

Btw, why is the &amp; to & conversion still required ? I though Ziv moved the
escaping code out of ConcatVisitor.

http://codereview.appspot.com/2164045/diff/7001/java/gadgets/src/test/java/or...
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java:824:
private VisitStatus getVisitStatusJs(Node node, String rewriteRegex, boolean
splitJs, boolean singleResouce)
line length

[Kuntal Loya, 2010-10-04 08:50:15]

http://codereview.appspot.com/2164045/diff/7001/java/gadgets/src/main/java/or...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ContentRewriterFeature.java
(right):

http://codereview.appspot.com/2164045/diff/7001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ContentRewriterFeature.java:138:
@Named("shindig.content-rewrite.enable-single-resource-concat")boolean
enableSingleResourceConcatenation) {
On 2010/10/03 19:17:28, gagan.goku wrote:
> line length

Done.

http://codereview.appspot.com/2164045/diff/7001/java/gadgets/src/test/java/or...
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java
(right):

http://codereview.appspot.com/2164045/diff/7001/java/gadgets/src/test/java/or...
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java:132:
assertEquals(VisitStatus.BYPASS, getVisitStatusJs(js1, null, false, false));
On 2010/10/03 19:17:28, gagan.goku wrote:
> You could create another method which takes the singleResourceConcat
parameter,
> while keeping the current getVisitStatusJs() method signature as is [ implicit
> singleResourceConcat = false ] That way the diffs would be fewer.
> 
> But your approach makes things clearer, so its okay if you feel against the 2
> method approach.

I can use the existing tests to verify all the scenarios if I use the same
method.

http://codereview.appspot.com/2164045/diff/7001/java/gadgets/src/test/java/or...
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java:228:
assertEquals(VisitStatus.RESERVE_NODE, getVisitStatusCss(node, null, true));
On 2010/10/03 19:17:28, gagan.goku wrote:
> this should ideally be moved out to another test case, something like:
> visitCssSeperatedByNonEmptyTextNodeWhenSingleConcatEnabled

Done.

http://codereview.appspot.com/2164045/diff/7001/java/gadgets/src/test/java/or...
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java:232:
public void dontVisitRelFreeCss() throws Exception {
On 2010/10/03 19:17:28, gagan.goku wrote:
> I know this is not related to this codereview, but could you also rename this
> method to:
> dontVisitCssWithoutRelAttribute

CssWithoutRelAttribute is same as RelFreeCss.

http://codereview.appspot.com/2164045/diff/7001/java/gadgets/src/test/java/or...
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java:240:
public void dontVisitTypeCssFreeCss() throws Exception {
On 2010/10/03 19:17:28, gagan.goku wrote:
> I know this is not related to this codereview, but could you also rename this
> method to
> dontVisitCssWithoutTypeAttribute.

CssWithoutTypeAttribute is same as TypeCssFreeCss

http://codereview.appspot.com/2164045/diff/7001/java/gadgets/src/test/java/or...
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java:265:
Node node = elem("link", "href", CSS1_URL_STR);
On 2010/10/03 19:17:28, gagan.goku wrote:
> This test is kind of redundant as we have tested that css is ignored when
either
> the type or rel attribute is missing (we have covered both code paths so this
> one is kind of implied)

Maybe to verify its not an XOR :)

http://codereview.appspot.com/2164045/diff/7001/java/gadgets/src/test/java/or...
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java:324:
assertEquals(CONCAT_BASE_URI.getScheme(), concatUri.getScheme());
On 2010/10/03 19:17:28, gagan.goku wrote:
> you could check the entire thing in 1 shot, it would be more readable as well:
> assertEquals("//blah/gadgets/concat?1= ..." , concatUri.toString())

It is good to split and check like they have done in existing tests, esp for
concat uris which are big and would have to be constructed the same way
CONCAT_BASE_URI.getScheme() + getAuthorigy() + ...
So, splitting it up is better as it saves all this computation, plus gives
exactly what is wrong.

http://codereview.appspot.com/2164045/diff/7001/java/gadgets/src/test/java/or...
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java:495:
List<Node> fullListJs = Lists.newArrayList();
On 2010/10/03 19:17:28, gagan.goku wrote:
> You can make this a 1 liner:
> List<Node> jsList = ImmutableList.of(seqNodes(js1, js2), seqNodes(js3));

Needs a list and not ImmutableList

http://codereview.appspot.com/2164045/diff/7001/java/gadgets/src/test/java/or...
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java:512:
assertEquals(CONCAT_BASE_URI.getScheme(), concatUri1.getScheme());
On 2010/10/03 19:17:28, gagan.goku wrote:
> something like:
> assertEquals("//example.org/gadgets/concat?1=..&2=..&...",
concatUri.toString())
> might be more readable.
> 
> Btw, why is the &amp; to & conversion still required ? I though Ziv moved the
> escaping code out of ConcatVisitor.

Removed the &amp; changed, I seem to have missed this one.

http://codereview.appspot.com/2164045/diff/7001/java/gadgets/src/test/java/or...
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java:824:
private VisitStatus getVisitStatusJs(Node node, String rewriteRegex, boolean
splitJs, boolean singleResouce)
On 2010/10/03 19:17:28, gagan.goku wrote:
> line length

Done.

[Kuntal Loya, 2010-10-08 14:22:48]

svn up + test renames.

[gagan.goku, 2010-10-08 16:53:34]

LGTM. Will commit shortly.

[gagan.goku, 2010-10-08 17:33:25]

On 2010/10/08 16:53:34, gagan.goku wrote:
> LGTM. Will commit shortly.

Too many merge failures :( Please svn up and reupload patch.

[Kuntal Loya, 2010-10-08 18:29:01]

Done.


On Fri, Oct 8, 2010 at 11:03 PM, <gagan.goku@gmail.com> wrote:

> On 2010/10/08 16:53:34, gagan.goku wrote:
>
>> LGTM. Will commit shortly.
>>
>
> Too many merge failures :( Please svn up and reupload patch.
>
>
> http://codereview.appspot.com/2164045/
>

[gagan.goku, 2010-10-08 18:53:49]

Build successful. Committed as r1005954.