Update Sanitizing rewriters to use ProxyUriManager, Visitor pattern

The existing Sanitizing rewriters, especially SanitizingGadgetRewriter, use both LinkRewriter, which is being removed in favor of ProxyUriManager, as well as implementing a custom Visitor pattern.

This CL:
A) removes the use of LinkRewriter in favor of the ProxyUriManager interface
B) removes the custom NodeVisitor implementation in favor of DomWalker

[johnfargo, 2010-03-03 19:47:36]

Sync'd to head.

[zhoresh, 2010-03-05 18:27:03]

New tests are appreciated!

http://codereview.appspot.com/223095/diff/1001/2007
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/SanitizingGadgetRewriter.java
(right):

http://codereview.appspot.com/223095/diff/1001/2007#newcode186
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/SanitizingGadgetRewriter.java:186:
VisitStatus status = VisitStatus.MODIFY;
Please doc why status is set to MODIFY always and not just when removing
attributes. It seems that the boolean removeAttr can also change attributes (not
very clean...)

Maybe change removeAttr to modifyAttr that return true if attribute was
modified/deleted

http://codereview.appspot.com/223095/diff/1001/2008
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/SanitizingProxyingLinkRewriter.java
(right):

http://codereview.appspot.com/223095/diff/1001/2008#newcode35
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/SanitizingProxyingLinkRewriter.java:35:
public class SanitizingProxyingLinkRewriter implements ProxyUriManager {
Is it a rewriter or a uri manager? I think it is confusing to follow the code
without naming consistency

I start to like the original LinkRewriter name better then 
UriManager

http://codereview.appspot.com/223095/diff/1001/2001
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/render/SanitizingGadgetRewriterTest.java
(right):

http://codereview.appspot.com/223095/diff/1001/2001#newcode203
java/gadgets/src/test/java/org/apache/shindig/gadgets/render/SanitizingGadgetRewriterTest.java:203:
+ "@import
url('http://host.com/proxy?url=www.example.org%2Fwww.evil.com%2Fx.js&"
Where did the gadget=... value is gone?
I thought it would be useful for statistics.

[johnfargo, 2010-03-05 18:51:56]

On Fri, Mar 5, 2010 at 10:27 AM, <zhoresh@gmail.com> wrote

> New tests are appreciated!
>
>
> http://codereview.appspot.com/223095/diff/1001/2007
> File
>
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/SanitizingGadgetRewriter.java
> (right):
>
> http://codereview.appspot.com/223095/diff/1001/2007#newcode186
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/SanitizingGadgetRewriter.java:186:
> VisitStatus status = VisitStatus.MODIFY;
> Please doc why status is set to MODIFY always and not just when removing
> attributes. It seems that the boolean removeAttr can also change
> attributes (not very clean...)
>
> Maybe change removeAttr to modifyAttr that return true if attribute was
> modified/deleted
>

Agreed -- I missed one use case from the previous class (modifying the
attribute) so have to return MODIFY at all times in the current scheme
(found this during test phase). The impact should be relatively low, in that
all MODIFY does is set a dirty-bit on the DOM, forcing reserialization --
which has negative performance impact only when no other rewriter or Visitor
has modified it.

Even so, I'll fix it up to return a trinary state (NONE, REMOVE, MODIFY)


>
> http://codereview.appspot.com/223095/diff/1001/2008
> File
>
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/SanitizingProxyingLinkRewriter.java
> (right):
>
> http://codereview.appspot.com/223095/diff/1001/2008#newcode35
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/SanitizingProxyingLinkRewriter.java:35:
> public class SanitizingProxyingLinkRewriter implements ProxyUriManager {
> Is it a rewriter or a uri manager? I think it is confusing to follow the
> code without naming consistency
>
> I start to like the original LinkRewriter name better then
> UriManager
>

Yeah it's actually a UriManager at this point.. I'm just reusing the class.
I'm starting to think it best to just completely replace the "old" classes
with new ones and keep the old infrastructure (for HTMLContentRewriter) in
place. I'll send you a CL to that effect and update these w/ more accurate
class names to avoid confusion.


>
> http://codereview.appspot.com/223095/diff/1001/2001
> File
>
>
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/render/SanitizingGadgetRewriterTest.java
> (right):
>
> http://codereview.appspot.com/223095/diff/1001/2001#newcode203
>
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/render/SanitizingGadgetRewriterTest.java:203:
> + "@import
> url('http://host.com/proxy?url=www.example.org%2Fwww.evil.com%2Fx.js&"
> Where did the gadget=... value is gone?
> I thought it would be useful for statistics.


It's only gone for the test. The PassthruManager that manipulates the URI is
a dummy/test implementation that performs a very simple (one query param,
well-known scheme, authority, and path) transformation for easier testing.
The "real" UriManager that would be used for this is DefaultProxyUriManager
or a subclass, which adds gadget (and container, debug, nocache and the
rest).


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