Bug Fix: Cache headers are removed by BaseOptimizer for all rewritten images.

Bug Fix: Cache headers are removed by BaseOptimizer for all rewritten images responses.

Which makes ProxyHandler to add default "max-age=300" as it won't be able to find the Cache-Control headers, even though original response has max-age=86400.

[satya3656, 2011-02-09 13:20:18]

Removing the pom.xml file

[satya3656, 2011-02-09 13:21:32]

Removing the pom.xml file

[nikhilmadan23, 2011-02-09 13:39:41]

lgtm

http://codereview.appspot.com/4148044/diff/5001/java/gadgets/src/main/java/or...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java
(right):

http://codereview.appspot.com/4148044/diff/5001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:142:
.setHeader("Content-Type", getOutputContentType())
move one line up. Could you also add a comment here explaining this?

[satya3656, 2011-02-09 13:44:49]

Addressing Nikhil's comments

[satya3656, 2011-02-09 13:45:10]

http://codereview.appspot.com/4148044/diff/5001/java/gadgets/src/main/java/or...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java
(right):

http://codereview.appspot.com/4148044/diff/5001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:142:
.setHeader("Content-Type", getOutputContentType())
On 2011/02/09 13:39:41, nikhilmadan23 wrote:
> move one line up. Could you also add a comment here explaining this?

Done.

[gagan.goku, 2011-02-10 18:44:45]

http://codereview.appspot.com/4148044/diff/14001/java/gadgets/src/test/java/o...
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/image/JPEGOptimizerTest.java
(right):

http://codereview.appspot.com/4148044/diff/14001/java/gadgets/src/test/java/o...
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/image/JPEGOptimizerTest.java:86:
public void testLargeJPEGWithEtag() throws Exception {
might as well club both tests into 1.

http://codereview.appspot.com/4148044/diff/14001/java/gadgets/src/test/java/o...
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/image/JPEGOptimizerTest.java:101:
responseBuilder.addHeader(CACHE_CONTROL_HEADER_NAME,
CACHE_CONTROL_HEADER_VALUE);
replace vars with the values, since they are not getting used anywhere else, and
it would also make the code more readable:
responseBuilder.addHeader("Cache-Control", "public, max-age=86400");

[gagan.goku, 2011-02-10 18:45:07]

lgtm, address small nits and il commit

[johnfargo, 2011-02-10 20:54:57]

http://codereview.appspot.com/4148044/diff/14001/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java
(right):

http://codereview.appspot.com/4148044/diff/14001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:146:
.setResponse(minBytes);
let's remove the ETAG header within HttpResponse(Builder) itself on write ie. in
the setResponse() method, since that is the actual operation that changes it.

[satya3656, 2011-02-14 07:36:47]

Addressing Comments

[satya3656, 2011-02-14 08:26:52]

Synced to head 1070397

[satya3656, 2011-02-14 08:29:18]

http://codereview.appspot.com/4148044/diff/14001/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java
(right):

http://codereview.appspot.com/4148044/diff/14001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BaseOptimizer.java:146:
.setResponse(minBytes);
On 2011/02/10 20:54:57, johnfargo wrote:
> let's remove the ETAG header within HttpResponse(Builder) itself on write ie.
in
> the setResponse() method, since that is the actual operation that changes it.

Done.

http://codereview.appspot.com/4148044/diff/14001/java/gadgets/src/test/java/o...
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/image/JPEGOptimizerTest.java
(right):

http://codereview.appspot.com/4148044/diff/14001/java/gadgets/src/test/java/o...
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/image/JPEGOptimizerTest.java:86:
public void testLargeJPEGWithEtag() throws Exception {
On 2011/02/10 18:44:45, gagan.goku wrote:
> might as well club both tests into 1.

Done.

http://codereview.appspot.com/4148044/diff/14001/java/gadgets/src/test/java/o...
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/image/JPEGOptimizerTest.java:101:
responseBuilder.addHeader(CACHE_CONTROL_HEADER_NAME,
CACHE_CONTROL_HEADER_VALUE);
On 2011/02/10 18:44:45, gagan.goku wrote:
> replace vars with the values, since they are not getting used anywhere else,
and
> it would also make the code more readable:
> responseBuilder.addHeader("Cache-Control", "public, max-age=86400");

Done.

[gagan.goku, 2011-02-14 10:35:19]

http://codereview.appspot.com/4148044/diff/24002/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java
(right):

http://codereview.appspot.com/4148044/diff/24002/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java:100:
removeHeader("ETag");
This approach seems kind of messy because one would not expect a method called
setResponseString to modify the response headers really.

Plus its easy to do things like:
new HttpResponseBuilder()
    .addHeader( compuedEtag )
    .setResponseString("blah")
    .create()
and we would actually clear the etag header in that case.

If you want to do this, mention this quirk in the class comment and this methods
comment so ppl can be vary of this.

http://codereview.appspot.com/4148044/diff/24002/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java:145:
removeHeader("ETag");
same here

http://codereview.appspot.com/4148044/diff/24002/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java:157:
removeHeader("ETag");
same here

[satya3656, 2011-02-14 11:19:37]

Removing the changes to HttpRsponseBuilder.

[satya3656, 2011-02-14 11:22:22]

http://codereview.appspot.com/4148044/diff/24002/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java
(right):

http://codereview.appspot.com/4148044/diff/24002/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java:100:
removeHeader("ETag");
Removing this piece as it might add breakages to the functionality of existing
code who does 
new HttpResponseBuilder() 
  .addHeader( compuedEtag ) 
  .setResponseString("blah") 
  .create()

http://codereview.appspot.com/4148044/diff/24002/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java:145:
removeHeader("ETag");
On 2011/02/14 10:35:20, gagan.goku wrote:
> same here

Removed

http://codereview.appspot.com/4148044/diff/24002/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java:157:
removeHeader("ETag");
On 2011/02/14 10:35:20, gagan.goku wrote:
> same here

removed

[gagan.goku, 2011-02-14 11:26:59]

lgtm

[gagan.goku, 2011-02-14 13:17:33]

Build looks good.
Committed as r1070483.