Refactor ConcatLinkRewriter as a Visitor

Removes LinkRewriter references and dependencies from ConcatLinkRewriter, and re-implements ConcatLinkRewriter in the DomWalker.Visitor paradigm.

The operation implemented by this Visitor is limited in scope: it only performs concat (and in the case of JS, split-concat aka. "JS spriting") rewriting, but does *not* rearrange <style> nodes to be adjacent to one another. That will be done by another Visitor, with the previous operation provided by running both passes in order.

[johnfargo, 2010-03-03 09:15:03]

Implemented, but haven't yet verified, tests.

[johnfargo, 2010-03-03 09:16:23]

Spacing fix.

[johnfargo, 2010-03-03 09:22:17]

Test tweaks.

[johnfargo, 2010-03-03 18:23:24]

Completed tests, with corresponding bugfixes.

[johnfargo, 2010-03-03 18:24:39]

Import fix.

[johnfargo, 2010-03-03 19:50:56]

Synced to head.

[zhoresh, 2010-03-06 01:41:00]

http://codereview.appspot.com/224097/diff/2001/2003
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatLinkRewriter.java
(right):

http://codereview.appspot.com/224097/diff/2001/2003#newcode122
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatLinkRewriter.java:122:
ConcatUriManager.ConcatUri.fromList(gadget, uriBatches, type), !split);
It would be nice to actually figure out if split JS is needed. Meaning if split
js support is enabled, but there is really just 2 scripts that are sibling,
split js is not needed.

http://codereview.appspot.com/224097/diff/2001/2003#newcode166
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatLinkRewriter.java:166:
elem.getAttribute("type").contains("css"));
Check for null from elem.getAttribute

http://codereview.appspot.com/224097/diff/2001/2005
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatLinkRewriterTest.java
(right):

http://codereview.appspot.com/224097/diff/2001/2005#newcode88
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatLinkRewriterTest.java:88:
css4 = elem("link", "rel", "stylesheet", "type", "text/css", "href",
CSS4_URL_STR);
Test also bad nodes (btw - where is elem defined?)

[johnfargo, 2010-03-08 23:32:38]

On Fri, Mar 5, 2010 at 5:41 PM, <zhoresh@gmail.com> wrote:

>
> http://codereview.appspot.com/224097/diff/2001/2003
> File
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatLinkRewriter.java
> (right):
>
> http://codereview.appspot.com/224097/diff/2001/2003#newcode122
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatLinkRewriter.java:122:
> ConcatUriManager.ConcatUri.fromList(gadget, uriBatches, type), !split);
> It would be nice to actually figure out if split JS is needed. Meaning
> if split js support is enabled, but there is really just 2 scripts that
> are sibling, split js is not needed.
>

I agree -- for simplicity, I'm starting with single-mode though, to add
conditional-split later.


>
> http://codereview.appspot.com/224097/diff/2001/2003#newcode166
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatLinkRewriter.java:166:
> elem.getAttribute("type").contains("css"));
> Check for null from elem.getAttribute
>

Per JavaDoc, Element.getAttribute() returns the empty string for missing
attribs.


>
> http://codereview.appspot.com/224097/diff/2001/2005
> File
>
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatLinkRewriterTest.java
> (right):
>
> http://codereview.appspot.com/224097/diff/2001/2005#newcode88
>
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatLinkRewriterTest.java:88:
> css4 = elem("link", "rel", "stylesheet", "type", "text/css", "href",
> CSS4_URL_STR);
> Test also bad nodes (btw - where is elem defined?)


A few tests added -- elem is defined in the test base class as a DOM helper.


>
>
> http://codereview.appspot.com/224097/show
>

[johnfargo, 2010-03-08 23:37:40]

Renamed ConcatVisitor; tweaked tests per zhoresh's comments.

[zhoresh, 2010-03-08 23:51:46]

LGTM

http://codereview.appspot.com/224097/diff/11001/12002
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java
(right):

http://codereview.appspot.com/224097/diff/11001/12002#newcode194
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:194:
} catch (Exception e) {
Use specific Exception?