On Tue, Apr 14, 2009 at 8:06 PM, <[email protected]> wrote: > > http://codereview.appspot.com/41063/diff/94/121 > File > java/gadgets/src/main/java/org/apache/shindig/gadgets/render/HtmlRenderer.java > (right): > > http://codereview.appspot.com/41063/diff/94/121#newcode55 > Line 55: GadgetHtmlParser htmlParser) { > Looking at this, I kind of like having a class that encapsulates the > rewriters and parser. It makes testing of HtmlRenderer a little easier. > Admittedly I find it hard to justify a class that just does parse + > iterate beyond that, though.
Ditto. I liked how HtmlRenderer looked before, but couldn't justify the extra class. > http://codereview.appspot.com/41063/diff/94/123 > File > java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingGadgetRewriter.java > (right): > > http://codereview.appspot.com/41063/diff/94/123#newcode85 > Line 85: public class RenderingGadgetRewriter implements GadgetRewriter > { > "Rendering" is redundant here, but I'm at a loss for a better name. > > *however*, the TODO above can be completed now that we've been using > full DOM for the last 6 months. It's safe to say that the code is > 'proven' enough to do that work (later...) Removed the "if-when" clause from the TODO. > http://codereview.appspot.com/41063/diff/94/120 > File > java/gadgets/src/main/java/org/apache/shindig/gadgets/render/SanitizedRenderingGadgetRewriter.java > (right): > > http://codereview.appspot.com/41063/diff/94/120#newcode63 > Line 63: public class SanitizedRenderingGadgetRewriter implements > GadgetRewriter { > Sanitiz[ing]GadgetRewriter? SanitizingGadgetRewriter and SanitizingRequestRewriter are much better names, done. > http://codereview.appspot.com/41063/diff/94/128 > File > java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/DefaultRequestRewriterRegistry.java > (right): > > http://codereview.appspot.com/41063/diff/94/128#newcode34 > Line 34: public class DefaultRequestRewriterRegistry implements > RequestRewriterRegistry { > Is there more than one place that uses this? So far as I can tell it > looks like only RequestPipeline touches this. RequestPipeline doesn't use it (it uses an ImageRewriter). But HttpRequestHandler, MakeRequestHandler, and ProxyHandler all use a RequestRewriterRegistry. > > http://codereview.appspot.com/41063/diff/94/131 > File > java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/GadgetRewriter.java > (right): > > http://codereview.appspot.com/41063/diff/94/131#newcode33 > Line 33: void rewrite(Gadget gadget, MutableContent content); > It would be nice if this could throw a checked exception. Added RewritingException, which can be thrown by either GadgetRewriter or RequestRewriter. > > http://codereview.appspot.com/41063 >
