On Tue, May 25, 2010 at 10:20 AM, <[email protected]> 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 >
