Refactor Shindig BasicImageRewriter

The existing Image Rewriter's (BasicImageRewriter) rewrite method is a huge monster that executes different steps and the resize checks are scattered through out the method.

This change is an attempt to refactor the main method 'rewrite' with the added flexibility for any child class to override the four discrete steps of rewrite viz ReadImage, resizeImage, updateResponse and applyOptimizer.

In the proposed refactoring, the resize checks are wrapped under a private method 'canRewrite'. The specific checks and their relative orders are very much dependent on the underlying Image processing package (Sanselan) used. 

This Rewriter can be further generalised/re-factored to make this method 'canRewrite' public and provide abstract methods for extracting ImageFormat & ImageInfo; thereby removing the hard dependency on Apache Sanselan package.

[johnfargo, 2010-08-04 23:14:01]

Looks like a great start, thanks Vikas. I've got only one substantive comment
for the moment.

http://codereview.appspot.com/1876047/diff/1/3
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BasicImageRewriter.java
(right):

http://codereview.appspot.com/1876047/diff/1/3#newcode159
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BasicImageRewriter.java:159:
setRequireResize(false);
Since rewriters are injected once/as singletons, I suspect maintaining state in
them (ie. here) will lead to threading issues. Up until now, we've relied on
each thread's individual stack to maintain such config. Perhaps this method
could return a wrapper object of some kind indicating the resize info?

[vikaas.arora, 2010-08-05 10:40:22]

http://codereview.appspot.com/1876047/diff/1/3
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BasicImageRewriter.java
(right):

http://codereview.appspot.com/1876047/diff/1/3#newcode159
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BasicImageRewriter.java:159:
setRequireResize(false);
On 2010/08/04 23:14:01, johnfargo wrote:
> Since rewriters are injected once/as singletons, I suspect maintaining state
in
> them (ie. here) will lead to threading issues. Up until now, we've relied on
> each thread's individual stack to maintain such config. Perhaps this method
> could return a wrapper object of some kind indicating the resize info?

Agreed, it's not a good idea to maintain state in the class.
In the next version, I am returning the wrapped object (pair).

[vikaas.arora, 2010-08-05 10:42:25]

Removed class variables (maintaining state)

[jtarrio, 2010-08-06 21:12:48]

http://codereview.appspot.com/1876047/diff/7001/8002
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BasicImageRewriter.java
(right):

http://codereview.appspot.com/1876047/diff/7001/8002#newcode223
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BasicImageRewriter.java:223:
image = resizeImageImpl(image, requestedWidth, requestedHeight, widthDelta,
heightDelta);
It looks like most of this method is just a build-up to this line, which is a
method call.

I'd suggest making this method only return the new dimensions (or null if no
resize is needed); then the caller of this method would call the resizeImageImpl
itself.

I'd also suggest, if you do this, to rename this method to getNewSize and
resizeImageImpl to just resizeImage.

Example:

ResizeDimensions newSize = getNewSize(request, response, image, imageInfo);
if (newSize != null) {
  image = resizeImage(image, newSize);
}

private class ResizeDimensions {
  int requestedWidth;
  int requestedHeight;
  int widthDelta;
  int heightDelta;

  ResizeDimensions(int requestedWidth, int requestedHeight, int widthDelta, int
heightDelta) {
    ...
  }
}

http://codereview.appspot.com/1876047/diff/7001/8002#newcode262
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BasicImageRewriter.java:262:
Pair<BufferedImage, Boolean> resize = resizeImage(request, response, image,
imageInfo,
If you do the refactoring I described above, you would be able to remove the
requireResize parameter here and the imageResized parameter in updateResponse
(and the corresponding conditional branches in each method):

if (imageRewrite.two) {
  ResizeDimensions newSize = getNewSize(request, response, image, imageInfo);
  if (newSize != null) {
    image = resizeImage(image, newSize);
    updateResponse(response, image);
  }
}
applyOptimizer(response, imageFormat, image);

http://codereview.appspot.com/1876047/diff/7001/8002#newcode270
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BasicImageRewriter.java:270:
applyOptimizer(response, imageFormat, image);
Even if you don't do the above refactorings, take note that this is the original
image, not the resized image.

[johnfargo, 2010-08-18 16:25:58]

Hey Vikas, just giving a friendly ping. Thoughts on this one? Cheers, John

On Fri, Aug 6, 2010 at 2:12 PM, <jtarrio@gmail.com> wrote:

>
> http://codereview.appspot.com/1876047/diff/7001/8002
>
> File
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BasicImageRewriter.java
> (right):
>
> http://codereview.appspot.com/1876047/diff/7001/8002#newcode223
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BasicImageRewriter.java:223:
> image = resizeImageImpl(image, requestedWidth, requestedHeight,
> widthDelta, heightDelta);
> It looks like most of this method is just a build-up to this line, which
> is a method call.
>
> I'd suggest making this method only return the new dimensions (or null
> if no resize is needed); then the caller of this method would call the
> resizeImageImpl itself.
>
> I'd also suggest, if you do this, to rename this method to getNewSize
> and resizeImageImpl to just resizeImage.
>
> Example:
>
> ResizeDimensions newSize = getNewSize(request, response, image,
> imageInfo);
> if (newSize != null) {
>  image = resizeImage(image, newSize);
> }
>
> private class ResizeDimensions {
>  int requestedWidth;
>  int requestedHeight;
>  int widthDelta;
>  int heightDelta;
>
>  ResizeDimensions(int requestedWidth, int requestedHeight, int
> widthDelta, int heightDelta) {
>    ...
>  }
> }
>
> http://codereview.appspot.com/1876047/diff/7001/8002#newcode262
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BasicImageRewriter.java:262:
> Pair<BufferedImage, Boolean> resize = resizeImage(request, response,
> image, imageInfo,
> If you do the refactoring I described above, you would be able to remove
> the requireResize parameter here and the imageResized parameter in
> updateResponse (and the corresponding conditional branches in each
> method):
>
> if (imageRewrite.two) {
>  ResizeDimensions newSize = getNewSize(request, response, image,
> imageInfo);
>  if (newSize != null) {
>    image = resizeImage(image, newSize);
>    updateResponse(response, image);
>  }
> }
> applyOptimizer(response, imageFormat, image);
>
> http://codereview.appspot.com/1876047/diff/7001/8002#newcode270
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BasicImageRewriter.java:270:
> applyOptimizer(response, imageFormat, image);
> Even if you don't do the above refactorings, take note that this is the
> original image, not the resized image.
>
>
> http://codereview.appspot.com/1876047/show
>

[vikaas.arora, 2010-08-19 04:33:47]

Hi John -

The day Jacobo gave the feedback, I went on vacation.
Will close on these once I am back from vacation.

Thanks for the ping though,
Vikas

On Wed, Aug 18, 2010 at 9:55 PM, John Hjelmstad <johnfargo@gmail.com> wrote:
> Hey Vikas, just giving a friendly ping. Thoughts on this one? Cheers, John
>
> On Fri, Aug 6, 2010 at 2:12 PM, <jtarrio@gmail.com> wrote:
>>
>> http://codereview.appspot.com/1876047/diff/7001/8002
>> File
>>
>>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BasicImageRewriter.java
>> (right):
>>
>> http://codereview.appspot.com/1876047/diff/7001/8002#newcode223
>>
>>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BasicImageRewriter.java:223:
>> image = resizeImageImpl(image, requestedWidth, requestedHeight,
>> widthDelta, heightDelta);
>> It looks like most of this method is just a build-up to this line, which
>> is a method call.
>>
>> I'd suggest making this method only return the new dimensions (or null
>> if no resize is needed); then the caller of this method would call the
>> resizeImageImpl itself.
>>
>> I'd also suggest, if you do this, to rename this method to getNewSize
>> and resizeImageImpl to just resizeImage.
>>
>> Example:
>>
>> ResizeDimensions newSize = getNewSize(request, response, image,
>> imageInfo);
>> if (newSize != null) {
>>  image = resizeImage(image, newSize);
>> }
>>
>> private class ResizeDimensions {
>>  int requestedWidth;
>>  int requestedHeight;
>>  int widthDelta;
>>  int heightDelta;
>>
>>  ResizeDimensions(int requestedWidth, int requestedHeight, int
>> widthDelta, int heightDelta) {
>>    ...
>>  }
>> }
>>
>> http://codereview.appspot.com/1876047/diff/7001/8002#newcode262
>>
>>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BasicImageRewriter.java:262:
>> Pair<BufferedImage, Boolean> resize = resizeImage(request, response,
>> image, imageInfo,
>> If you do the refactoring I described above, you would be able to remove
>> the requireResize parameter here and the imageResized parameter in
>> updateResponse (and the corresponding conditional branches in each
>> method):
>>
>> if (imageRewrite.two) {
>>  ResizeDimensions newSize = getNewSize(request, response, image,
>> imageInfo);
>>  if (newSize != null) {
>>    image = resizeImage(image, newSize);
>>    updateResponse(response, image);
>>  }
>> }
>> applyOptimizer(response, imageFormat, image);
>>
>> http://codereview.appspot.com/1876047/diff/7001/8002#newcode270
>>
>>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BasicImageRewriter.java:270:
>> applyOptimizer(response, imageFormat, image);
>> Even if you don't do the above refactorings, take note that this is the
>> original image, not the resized image.
>>
>> http://codereview.appspot.com/1876047/show
>
>

[johnfargo, 2010-08-19 04:34:55]

Aha, I'd missed that you were on vacation. Enjoy the remainder of your rest!

On Wed, Aug 18, 2010 at 9:33 PM, Vikas Arora <vikaas.arora@gmail.com> wrote:

> Hi John -
>
> The day Jacobo gave the feedback, I went on vacation.
> Will close on these once I am back from vacation.
>
> Thanks for the ping though,
> Vikas
>
> On Wed, Aug 18, 2010 at 9:55 PM, John Hjelmstad <johnfargo@gmail.com>
> wrote:
> > Hey Vikas, just giving a friendly ping. Thoughts on this one? Cheers,
> John
> >
> > On Fri, Aug 6, 2010 at 2:12 PM, <jtarrio@gmail.com> wrote:
> >>
> >> http://codereview.appspot.com/1876047/diff/7001/8002
> >> File
> >>
> >>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BasicImageRewriter.java
> >> (right):
> >>
> >> http://codereview.appspot.com/1876047/diff/7001/8002#newcode223
> >>
> >>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BasicImageRewriter.java:223:
> >> image = resizeImageImpl(image, requestedWidth, requestedHeight,
> >> widthDelta, heightDelta);
> >> It looks like most of this method is just a build-up to this line, which
> >> is a method call.
> >>
> >> I'd suggest making this method only return the new dimensions (or null
> >> if no resize is needed); then the caller of this method would call the
> >> resizeImageImpl itself.
> >>
> >> I'd also suggest, if you do this, to rename this method to getNewSize
> >> and resizeImageImpl to just resizeImage.
> >>
> >> Example:
> >>
> >> ResizeDimensions newSize = getNewSize(request, response, image,
> >> imageInfo);
> >> if (newSize != null) {
> >>  image = resizeImage(image, newSize);
> >> }
> >>
> >> private class ResizeDimensions {
> >>  int requestedWidth;
> >>  int requestedHeight;
> >>  int widthDelta;
> >>  int heightDelta;
> >>
> >>  ResizeDimensions(int requestedWidth, int requestedHeight, int
> >> widthDelta, int heightDelta) {
> >>    ...
> >>  }
> >> }
> >>
> >> http://codereview.appspot.com/1876047/diff/7001/8002#newcode262
> >>
> >>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BasicImageRewriter.java:262:
> >> Pair<BufferedImage, Boolean> resize = resizeImage(request, response,
> >> image, imageInfo,
> >> If you do the refactoring I described above, you would be able to remove
> >> the requireResize parameter here and the imageResized parameter in
> >> updateResponse (and the corresponding conditional branches in each
> >> method):
> >>
> >> if (imageRewrite.two) {
> >>  ResizeDimensions newSize = getNewSize(request, response, image,
> >> imageInfo);
> >>  if (newSize != null) {
> >>    image = resizeImage(image, newSize);
> >>    updateResponse(response, image);
> >>  }
> >> }
> >> applyOptimizer(response, imageFormat, image);
> >>
> >> http://codereview.appspot.com/1876047/diff/7001/8002#newcode270
> >>
> >>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BasicImageRewriter.java:270:
> >> applyOptimizer(response, imageFormat, image);
> >> Even if you don't do the above refactorings, take note that this is the
> >> original image, not the resized image.
> >>
> >> http://codereview.appspot.com/1876047/show
> >
> >
>

[vikaas.arora, 2010-09-13 11:57:19]

On 2010/08/19 04:34:55, johnfargo wrote:
> Aha, I'd missed that you were on vacation. Enjoy the remainder of your rest!
> 
> On Wed, Aug 18, 2010 at 9:33 PM, Vikas Arora <mailto:vikaas.arora@gmail.com>
wrote:
> 
> > Hi John -
> >
> > The day Jacobo gave the feedback, I went on vacation.
> > Will close on these once I am back from vacation.
> >
> > Thanks for the ping though,
> > Vikas
> >
> > On Wed, Aug 18, 2010 at 9:55 PM, John Hjelmstad <mailto:johnfargo@gmail.com>
> > wrote:
> > > Hey Vikas, just giving a friendly ping. Thoughts on this one? Cheers,
> > John
> > >
> > > On Fri, Aug 6, 2010 at 2:12 PM, <mailto:jtarrio@gmail.com> wrote:
> > >>
> > >> http://codereview.appspot.com/1876047/diff/7001/8002
> > >> File
> > >>
> > >>
> >
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BasicImageRewriter.java
> > >> (right):
> > >>
> > >> http://codereview.appspot.com/1876047/diff/7001/8002#newcode223
> > >>
> > >>
> >
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BasicImageRewriter.java:223:
> > >> image = resizeImageImpl(image, requestedWidth, requestedHeight,
> > >> widthDelta, heightDelta);
> > >> It looks like most of this method is just a build-up to this line, which
> > >> is a method call.
> > >>
> > >> I'd suggest making this method only return the new dimensions (or null
> > >> if no resize is needed); then the caller of this method would call the
> > >> resizeImageImpl itself.
> > >>
> > >> I'd also suggest, if you do this, to rename this method to getNewSize
> > >> and resizeImageImpl to just resizeImage.
> > >>
> > >> Example:
> > >>
> > >> ResizeDimensions newSize = getNewSize(request, response, image,
> > >> imageInfo);
> > >> if (newSize != null) {
> > >>  image = resizeImage(image, newSize);
> > >> }
> > >>
> > >> private class ResizeDimensions {
> > >>  int requestedWidth;
> > >>  int requestedHeight;
> > >>  int widthDelta;
> > >>  int heightDelta;
> > >>
> > >>  ResizeDimensions(int requestedWidth, int requestedHeight, int
> > >> widthDelta, int heightDelta) {
> > >>    ...
> > >>  }
> > >> }
> > >>
> > >> http://codereview.appspot.com/1876047/diff/7001/8002#newcode262
> > >>
> > >>
> >
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BasicImageRewriter.java:262:
> > >> Pair<BufferedImage, Boolean> resize = resizeImage(request, response,
> > >> image, imageInfo,
> > >> If you do the refactoring I described above, you would be able to remove
> > >> the requireResize parameter here and the imageResized parameter in
> > >> updateResponse (and the corresponding conditional branches in each
> > >> method):
> > >>
> > >> if (imageRewrite.two) {
> > >>  ResizeDimensions newSize = getNewSize(request, response, image,
> > >> imageInfo);
> > >>  if (newSize != null) {
> > >>    image = resizeImage(image, newSize);
> > >>    updateResponse(response, image);
> > >>  }
> > >> }
> > >> applyOptimizer(response, imageFormat, image);
> > >>
> > >> http://codereview.appspot.com/1876047/diff/7001/8002#newcode270
> > >>
> > >>
> >
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BasicImageRewriter.java:270:
> > >> applyOptimizer(response, imageFormat, image);
> > >> Even if you don't do the above refactorings, take note that this is the
> > >> original image, not the resized image.
> > >>
> > >> http://codereview.appspot.com/1876047/show
> > >
> > >
> >
> 

Hi Jacobo -

Have another look.

Thanks,
Vikas

[vikaas.arora, 2010-09-14 06:32:25]

Adding Satya to this change

[vikaas.arora, 2010-09-15 03:16:29]

Ping ...

[satya3656, 2010-09-16 05:31:31]

http://codereview.appspot.com/1876047/diff/21001/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BasicImageRewriter.java
(right):

http://codereview.appspot.com/1876047/diff/21001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BasicImageRewriter.java:151:
private Pair<Boolean, Boolean> checkImage(HttpRequest request,
HttpResponseBuilder response,
Most of the checks here are for imageRewrite, except for one. Spliting it into
two functions one for each more modular?

http://codereview.appspot.com/1876047/diff/21001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BasicImageRewriter.java:320:
* If the image is resized, the request needs to change so that the optimizer can
Comment says only GIF's are converted to PNG's, but as per the code all
MIME_TYPES are converted to PNG's.

[vikaas.arora, 2010-09-16 08:42:37]

Incorporated Satya's feedback

[vikaas.arora, 2010-09-16 08:59:13]

Satya's feedback

[vikaas.arora, 2010-09-16 09:04:31]

http://codereview.appspot.com/1876047/diff/21001/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BasicImageRewriter.java
(right):

http://codereview.appspot.com/1876047/diff/21001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BasicImageRewriter.java:151:
private Pair<Boolean, Boolean> checkImage(HttpRequest request,
HttpResponseBuilder response,
On 2010/09/16 05:31:32, satya3656 wrote:
> Most of the checks here are for imageRewrite, except for one. Spliting it into
> two functions one for each more modular?

Splitted this function to two separate ones.

http://codereview.appspot.com/1876047/diff/21001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BasicImageRewriter.java:320:
* If the image is resized, the request needs to change so that the optimizer can
On 2010/09/16 05:31:32, satya3656 wrote:
> Comment says only GIF's are converted to PNG's, but as per the code all
> MIME_TYPES are converted to PNG's.

Have no idea. Maybe Fargo/Jacobo have the answer.

[satya3656, 2010-09-16 11:28:34]

LGTM

[jtarrio, 2010-09-17 03:25:34]

lgtm

On Tue, Sep 14, 2010 at 8:16 PM, <vikaas.arora@gmail.com> wrote:

> Ping ...
>
>
>
> http://codereview.appspot.com/1876047/
>



-- 
   Jacobo Tarrío     |     http://jacobo.tarrio.org/

[vikaas.arora, 2010-09-17 03:40:14]

Time to ping Gagan to use his commit powers :-)

On Fri, Sep 17, 2010 at 8:55 AM, Jacobo Tarrio <jtarrio@gmail.com> wrote:
> lgtm
>
> On Tue, Sep 14, 2010 at 8:16 PM, <vikaas.arora@gmail.com> wrote:
>>
>> Ping ...
>>
>>
>>
>> http://codereview.appspot.com/1876047/
>
>
>
> --
>    Jacobo Tarrío     |     http://jacobo.tarrio.org/
>

[gagan.goku, 2010-09-17 03:46:32]

Will take a look and submit sometime in the evening if thats okay with you.

Thanks
Gagan

-- 
The only thing missing in life is background music.
-- Gagandeep Singh


On Fri, Sep 17, 2010 at 9:10 AM, Vikas Arora <vikaas.arora@gmail.com> wrote:

> Time to ping Gagan to use his commit powers :-)
>
> On Fri, Sep 17, 2010 at 8:55 AM, Jacobo Tarrio <jtarrio@gmail.com> wrote:
> > lgtm
> >
> > On Tue, Sep 14, 2010 at 8:16 PM, <vikaas.arora@gmail.com> wrote:
> >>
> >> Ping ...
> >>
> >>
> >>
> >> http://codereview.appspot.com/1876047/
> >
> >
> >
> > --
> >    Jacobo Tarrío     |     http://jacobo.tarrio.org/
> >
>

[vikaas.arora, 2010-09-17 03:54:51]

Please take your time. No hurry.
Thanks,
Vikas

On Fri, Sep 17, 2010 at 9:16 AM, Gagandeep Singh <gagan.goku@gmail.com> wrote:
> Will take a look and submit sometime in the evening if thats okay with you.
> Thanks
> Gagan
>
> --
> The only thing missing in life is background music.
> -- Gagandeep Singh
>
>
> On Fri, Sep 17, 2010 at 9:10 AM, Vikas Arora <vikaas.arora@gmail.com> wrote:
>>
>> Time to ping Gagan to use his commit powers :-)
>>
>> On Fri, Sep 17, 2010 at 8:55 AM, Jacobo Tarrio <jtarrio@gmail.com> wrote:
>> > lgtm
>> >
>> > On Tue, Sep 14, 2010 at 8:16 PM, <vikaas.arora@gmail.com> wrote:
>> >>
>> >> Ping ...
>> >>
>> >>
>> >>
>> >> http://codereview.appspot.com/1876047/
>> >
>> >
>> >
>> > --
>> >    Jacobo Tarrío     |     http://jacobo.tarrio.org/
>> >
>
>

[gagan.goku, 2010-09-17 17:29:55]

All tests passing, committed patch as r998214.