Rewrite to use Sanselan library to handler attacks from malicious images
and vulnerabilities in the ImageIO libraries.
ImageRewriter now an interface with a default impl.
http://codereview.appspot.com/10841/diff/1/21
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java
(right):
http://codereview.appspot.com/10841/diff/1/21#newcode35
Line 35: ImageReader reader;
On 2008/12/12 23:47:03, awiner wrote:
these three can be final, as can a few others
Done.
http://codereview.appspot.com/10841/diff/1/21#newcode55
Line 55: this.baos = new
ByteArrayOutputStream(original.getContentLength());
On 2008/12/12 23:47:03, awiner wrote:
this will allocate a byte[] of size getContentLength(), which is
wasteful for
large images that we're not going to resize anyway; should lazily
instantiate
Done.
http://codereview.appspot.com/10841/diff/1/21#newcode63
Line 63: * @throws IOException
On 2008/12/13 01:26:22, etnu00 wrote:
No point in the javadoc tags if there's no explanation.
Done.
http://codereview.appspot.com/10841/diff/1/21#newcode73
Line 73: if (image == null) return;
On 2008/12/12 23:47:03, awiner wrote:
Is:
if (image == null) {
return;
}
preferred code-style 'round these parts?
Done.
http://codereview.appspot.com/10841/diff/1/21#newcode92
Line 92: public HttpResponse rewrite() throws IOException {
On 2008/12/13 01:26:22, etnu00 wrote:
It might be a good idea to call this method 'optimize' to avoid
confusion with
ContentRewriter.
I think re-write is OK for the moment. ImageRewriter may become a
ContentRewriter later
http://codereview.appspot.com/10841/diff/1/18
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/GIFOptimizer.java
(right):
http://codereview.appspot.com/10841/diff/1/18#newcode60
Line 60: write(ImageUtils.palettize(image, config.getMaxPaletteSize()));
On 2008/12/13 01:26:22, etnu00 wrote:
What happens if getMaxPaletteSize returns something larger than 16
bits?
Shouldn't we check this and not bother calling palettize?
It shouldnt have any negative impact other than unnecessary CPU cost. We
still choose an optimal bit-depth based on the no. of found colors in
the image.
http://codereview.appspot.com/10841/diff/1/16
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/ImageRewriter.java
(right):
http://codereview.appspot.com/10841/diff/1/16#newcode59
Line 59: * @param response - Rewritten response
On 2008/12/13 01:26:22, etnu00 wrote:
I think this should be 'response to rewrite'.
Done.
http://codereview.appspot.com/10841/diff/1/16#newcode81
Line 81: ioe.printStackTrace();
On 2008/12/12 23:47:03, awiner wrote:
log, don't printStackTrace()
Done.
http://codereview.appspot.com/10841/diff/1/16#newcode81
Line 81: ioe.printStackTrace();
On 2008/12/13 01:26:22, etnu00 wrote:
ImageRewriteException ?
I dont think we care too much about propagating other than log messages
http://codereview.appspot.com/10841/diff/1/16#newcode110
Line 110: String path = uri.getPath().toLowerCase();
On 2008/12/13 01:26:22, etnu00 wrote:
Is this really worth passing the uri around for?
Its not much of an overhead to pass it in currently. Only used in the
fetchers.
http://codereview.appspot.com/10841/diff/1/16#newcode114
Line 114: return new HttpResponseBuilder(original)
On 2008/12/12 23:47:03, awiner wrote:
<< 2 chars
Done.
http://codereview.appspot.com/10841/diff/1/19
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/JPEGOptimizer.java
(right):
http://codereview.appspot.com/10841/diff/1/19#newcode28
Line 28: * Optimize JPEG images by either converting them to PNGs to
storing them with a more
On 2008/12/13 01:26:22, etnu00 wrote:
s/to/or/ ?
Done.
http://codereview.appspot.com/10841/diff/1/19#newcode47
Line 47: config.getIgnoreThresholdBytes()), originalResponse);
On 2008/12/13 01:26:22, etnu00 wrote:
This is hard to read.
Cleaned up somewhat
http://codereview.appspot.com/10841/diff/1/7
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizerTest.java
(right):
http://codereview.appspot.com/10841/diff/1/7#newcode30
Line 30: */
On 2008/12/13 01:26:22, etnu00 wrote:
Your code is under attack by stray asterisks.
Done.
http://codereview.appspot.com/10841
