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/06/10 01:44:30 UTC

Adding BaseTagRemoverRewriter (issue1630042)

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

http://codereview.appspot.com/1630042/diff/1/3#newcode33
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/BaseTagRemoverRewriter.java:33:
public class BaseTagRemoverRewriter extends DomWalker.Rewriter {
If I understand correctly you don't take advantage of DomWalker at all,
so maybe just implement GadgetRewriter and RequestRewriter
It looks a bit forced and confusing as is.

http://codereview.appspot.com/1630042/diff/1/3#newcode58
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/BaseTagRemoverRewriter.java:58:
Gadget context = DomWalker.makeGadget(request);
Maybe add a check of RewritersUtil.isHtml like the DomWalker version
does.
I guess an alternative is to make the private rewrite with vistors
protected, and overwrite that instead (ignoring the vistors).

http://codereview.appspot.com/1630042/diff/1/2
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/BaseTagRemoverRewriterTest.java
(right):

http://codereview.appspot.com/1630042/diff/1/2#newcode69
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/BaseTagRemoverRewriterTest.java:69:
}
Test both rewrite functions,
Test without base tag
Test non html with base string (text or xml).

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