On 2010/06/13 06:04:23, gagan.goku wrote:
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 {
On 2010/06/09 23:44:30, zhoresh wrote:
> 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.
Done.
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);
On 2010/06/09 23:44:30, zhoresh wrote:
> 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).
>
Done.
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:
}
On 2010/06/09 23:44:30, zhoresh wrote:
> Test both rewrite functions,
> Test without base tag
> Test non html with base string (text or xml).
>
added tests without base tag and for non html data as well. Do i need
to add a
test case for the rewrite(Gadget gadget, MutableContent mc) version ?
I think since rewrite(HttpRequest request, HttpResponseBuilder
response) calls
the other variant anyways, we have kind of tested both completely.
But if you feel otherwise, please suggest what i should be testing
there.
Please review.
http://codereview.appspot.com/1630042/show
