Adding BaseTagRemoverRewriter

Adding a response rewriter that can remove base tag from html content being served.

[zhoresh, 2010-06-09 23:44:30]

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

[gagan.goku, 2010-06-13 06:04:23]

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.

[gagan.goku, 2010-06-21 05:08:03]

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.

[zhoresh, 2010-06-21 21:53:16]

lgtm, I removed @Override annotations and committed (r956714)

[gagan.goku, 2010-06-22 00:56:13]

On 2010/06/21 21:53:16, zhoresh wrote:
> lgtm, I removed @Override annotations and committed (r956714)

Thanks.