On Fri, Mar 5, 2010 at 10:27 AM, <[email protected]> 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 >
