http://codereview.appspot.com/10841/diff/1001/818 File ../trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/DefaultGuiceModule.java (right):
http://codereview.appspot.com/10841/diff/1001/818#newcode57 Line 57: bind(DefaultImageRewriter.class).annotatedWith(Names.named("org.apache.shindig.image-rewriter")) On 2009/01/27 00:46:05, awiner wrote:
think this should be binding ImageRewriter.class. Not sure why we need a naming annotation here either; especially not
one with
that generic of a name. If there's reason to have > 1 image rewriter,
names
should be describe the purpose of each.
Removed completely. No need for this when using ImplementedBy http://codereview.appspot.com/10841/diff/1001/804 File ../trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java (right): http://codereview.appspot.com/10841/diff/1001/804#newcode117 Line 117: private ByteArrayOutputStream baos; On 2009/01/27 00:46:05, awiner wrote:
move up with the rest of the variables
Done. http://codereview.appspot.com/10841/diff/1001/824 File ../trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/DefaultImageRewriter.java (right): http://codereview.appspot.com/10841/diff/1001/824#newcode47 Line 47: * Uses the Sanselan lilbrary to parse image content and metadata to avoid security On 2009/01/27 00:46:05, awiner wrote:
lilbrary -> library
Done. http://codereview.appspot.com/10841/diff/1001/824#newcode91 Line 91: if (response.getContentLength() < config.getIgnoreThresholdBytes()) { On 2009/01/27 00:46:05, awiner wrote:
could be moved up to top. And the direction seems wrong... > than
threshold
bytes, not < than...
Moved. Direction is correct though. We don't bother re-writing images below a certain size. Upper bound is the max in memory size. Renamed to getMinThresholdBytes http://codereview.appspot.com/10841/diff/1001/824#newcode104 Line 104: if (Math.abs(imageInfo.getWidth()) * Math.abs(imageInfo.getHeight()) * On 2009/01/27 00:46:05, awiner wrote:
Math.abs() can be negative. Math.abs(Integer.MIN_VALUE) == Integer.MIN_VALUE.
Cast int to long before calling abs to workaround. http://codereview.appspot.com/10841/diff/1001/811 File ../trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/GIFOptimizer.java (right): http://codereview.appspot.com/10841/diff/1001/811#newcode22 Line 22: import java.awt.*; On 2009/01/27 00:46:05, awiner wrote:
personal gripe against wide imports. YMMV.
Me too. Silly IntelliJ exceptions.. http://codereview.appspot.com/10841/diff/1001/811#newcode59 Line 59: return "gif"; On 2009/01/27 00:46:05, awiner wrote:
should be image/gif
Done. http://codereview.appspot.com/10841/diff/1001/811#newcode63 Line 63: return "gif"; On 2009/01/27 00:46:05, awiner wrote:
should be image/gif
Done. http://codereview.appspot.com/10841/diff/1001/822 File ../trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JPEGOptimizer.java (right): http://codereview.appspot.com/10841/diff/1001/822#newcode52 Line 52: if (pngOptimizer.minBytes != null) { On 2009/01/27 00:46:05, awiner wrote:
checking minBytes != null is a bit ugly. Add
BaseOptimizer.getRewrittenImage(),
avoid need to read any instance state?
Done. http://codereview.appspot.com/10841/diff/1001/822#newcode67 Line 67: this.minBytes = pngOptimizer.minBytes; On 2009/01/27 00:46:05, awiner wrote:
setting minBytes shouldn't be necessary; minBytes should only get set
if JPEG
won
Done. http://codereview.appspot.com/10841/diff/1001/815 File ../trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java (right): http://codereview.appspot.com/10841/diff/1001/815#newcode71 Line 71: //response.setHeader("Content-Disposition", "attachment;filename=p.txt"); On 2009/01/27 00:46:05, awiner wrote:
Intentional?
Testing artifact, reverted http://codereview.appspot.com/10841/diff/1001/806 File ../trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyBase.java (right): http://codereview.appspot.com/10841/diff/1001/806#newcode119 Line 119: //response.setHeader("Content-Disposition", "attachment;filename=p.txt"); On 2009/01/27 00:46:05, awiner wrote:
Intentional?
test artifact, reverted http://codereview.appspot.com/10841
