Refactor ImageRewriter as HttpResponseRewriter

Introduces new interface HttpResponseRewriter, and for the moment - as a proof-of-concept and for functional backward compatibility - implements only BasicImageRewriter in terms of this interface.

HttpResponseRewriter's API parallels that of GadgetRewriter:
void rewrite(ContextObject in, MutableContent out);

Specifically:
void rewrite(HttpRequest in, HttpResponseBuilder out);

This change thus utilizes the HttpResponseBuilder-as-MutableContent CL submitted a few weeks ago.

Additional notes:
* [Basic]ImageRewriter simply modifies an HttpResponse. Its implementation is updated in this CL to reflect the same. 
* HttpResponseRegistry is introduced, with default binding to BasicImageRewriter. The registry's rewriters are executed at the same place as ImageRewriter previously was - in DefaultRequestPipeline, after fetch and before caching.
* HttpResponseBuilder optimization implemented to return the "source" HttpResponse object from which it was created when no changes were made to its data. This avoids pointless object creation and data copying, maintaining ImageRewriter's performance in such cases.

This CL is transitional, as its goals are to move to the following:
* Ability to perform image rewriting steps independently, in separate rewriting passes (resizing, optimization, downsampling, format conversion, and so on).
* Situation in which all RequestRewriters are made into ResponseRewriters, removing one Rewriter type.
* Ability to precisely define the caching characteristics of [Response]Rewriters, of any type.

[Paul Lindner, 2010-05-25 10:46:27]

eyeball insepction seems fine.  might be worthwhile adding docs to UPGRADING for
the few people that might be customizing these interfaces.

Still holding out hope that there's a way different independent rewriter chains
can inject themselves without overriding / subclassing the module...

[johnfargo, 2010-05-25 16:33:17]

Adding UPGRADING notes.

[zhoresh, 2010-05-25 17:17:31]

LGTM

My only concern with the ResponseRewritters, is that the cache store data after
rewrite, so if the rewriters change the cache data would need to be cleared. 
This is not a new issue, it just something we might want to consider.

Currently the ImageRewriter get resize parameters, which are passed in using
HttpResponse params. To make sure the cache store different items for different
resize, there is specific code in AbstractHttpCache.createKey that handle the
specific params.
I think this might need a more generic code instead of handling only three
params.

[jtarrio, 2010-05-25 17:20:42]

http://codereview.appspot.com/1276042/diff/1/12
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java
(right):

http://codereview.appspot.com/1276042/diff/1/12#newcode157
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java:157:
headers.clear();
Doesn't this need a call to incrementNumChanges() too?

http://codereview.appspot.com/1276042/diff/10005/19016
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BasicImageRewriter.java
(right):

http://codereview.appspot.com/1276042/diff/10005/19016#newcode329
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BasicImageRewriter.java:329:
if (!response.create().getResponseAsString().contains("NETSCAPE2.0")) {
Why call create()? Won't it mess with the accounting of the number of changes,
etc.? Don't you have a method to get the response contents directly as a string?
:)

[fargo, 2010-05-25 17:20:44]

On Tue, May 25, 2010 at 10:17 AM, <zhoresh@gmail.com> wrote:

> LGTM
>
> My only concern with the ResponseRewritters, is that the cache store
> data after rewrite, so if the rewriters change the cache data would need
> to be cleared.
> This is not a new issue, it just something we might want to consider.
>
> Currently the ImageRewriter get resize parameters, which are passed in
> using HttpResponse params. To make sure the cache store different items
> for different resize, there is specific code in
> AbstractHttpCache.createKey that handle the specific params.
> I think this might need a more generic code instead of handling only
> three params.


Fully agreed -- this logic is way too distributed for my tastes. My plan for
following CLs is:

* Migrate RequestRewriters to ResponseRewriter -- these will be run *after*
caching, as existing RequestRewriters are.
* Try to generalize the caching logic as you've mentioned.

Cheers,
John


>
>
>
>
> http://codereview.appspot.com/1276042/show
>

[johnfargo, 2010-05-25 17:44:35]

On Tue, May 25, 2010 at 10:20 AM, <jtarrio@gmail.com> wrote:

>
> http://codereview.appspot.com/1276042/diff/1/12
> File
>
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java
> (right):
>
> http://codereview.appspot.com/1276042/diff/1/12#newcode157
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java:157:
> headers.clear();
> Doesn't this need a call to incrementNumChanges() too?
>

Good catch; it does. Done.


>
> http://codereview.appspot.com/1276042/diff/10005/19016
> File
>
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BasicImageRewriter.java
> (right):
>
> http://codereview.appspot.com/1276042/diff/10005/19016#newcode329
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BasicImageRewriter.java:329:
> if (!response.create().getResponseAsString().contains("NETSCAPE2.0")) {
> Why call create()? Won't it mess with the accounting of the number of
> changes, etc.? Don't you have a method to get the response contents
> directly as a string? :)


Good question. I did it this way mostly out of laziness, but kept it since
the accounting actually isn't messed up. If the object is mutated before
this call, a new HttpResponse object is cached internal to
HttpResponseBuilder. Its responseString is computed by the method here (also
lazily). If the object is _not_ mutated, the previous HttpResponse object's
responseString will be lazily computed. If a mutation occurs in
GIFOptimizer, yet another HttpResponse object will end up being created --
same as the previous class impl.

This is suboptimal, but is par for the course and avoids me having to
implement a responseString copy in HttpResponseBuilder (or just generating
one and throwing it away, which in theory is less efficient if someone were
to repeatedly retrieve the response-as-string from the Builder).


>
>
> http://codereview.appspot.com/1276042/show
>

[jtarrio, 2010-05-25 17:47:11]

LGTM