http://codereview.appspot.com/3480041/diff/33001/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java (right):
http://codereview.appspot.com/3480041/diff/33001/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java#newcode37 java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:37: import java.util.*; no wildcards pls http://codereview.appspot.com/3480041/diff/33001/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java#newcode66 java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:66: if ((!imageElement.getAttribute("height").isEmpty() && split getAttribute(key).isEmpty() into helper method for readability http://codereview.appspot.com/3480041/diff/33001/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java#newcode68 java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:68: imageElement.getAttribute("id").isEmpty() && why is the emptiness of ID important? http://codereview.appspot.com/3480041/diff/33001/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java#newcode70 java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:70: (!imageElement.getAttribute("style").isEmpty())) { So the rewriter accepts tags w/ style *and* class nodes? No CSS complications there? http://codereview.appspot.com/3480041/diff/33001/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java#newcode78 java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:78: proxied = proxyUriManager.process(uri); You could just choose to mutate the URL right here and return VisitStatus.MODIFY. revisit(...) is useful really only when you want (or need) batched operations performed on multiple nodes at a time. http://codereview.appspot.com/3480041/diff/33001/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java#newcode145 java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:145: builder.putQueryParameter(UriCommon.Param.RESIZE_WIDTH.getKey(), width); With chained proxy syntax, you can't always add these params directly. For tighter control over syntax, it would be ideal to simply take the processed ProxyUri you've pulled in visit(), call setResize(...) on it, then resend to proxyUriManager.make(...). That introduces some inefficiency (double-make of proxyUri), but you'd get chained syntax support. As noted above, you could do this directly in visit(...) rather than revisit(...) since you're not using batched operation here. Perhaps better still would be to improve ProxyingVisitor to extract the resizing "hints" from the DOM and pass them to the ProxyUri objects created there. You could do so by refactoring this class into a helper class ImageSizeExtractor with method getSizes(Element). http://codereview.appspot.com/3480041/
