You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@shindig.apache.org by zh...@gmail.com on 2010/03/06 02:41:00 UTC

Re: Refactor ConcatLinkRewriter as a Visitor (issue224097)

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?)

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

Re: Refactor ConcatLinkRewriter as a Visitor (issue224097)

Posted by John Hjelmstad <jo...@gmail.com>.
On Fri, Mar 5, 2010 at 5:41 PM, <zh...@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
>