On Wed, Mar 10, 2010 at 4:48 PM, <[email protected]> wrote: > > http://codereview.appspot.com/391042/diff/1/2 > File > > > java/gadgets/src/main/java/org/apache/shindig/gadgets/http/DefaultRequestPipeline.java > (right): > > http://codereview.appspot.com/391042/diff/1/2#newcode48 > > java/gadgets/src/main/java/org/apache/shindig/gadgets/http/DefaultRequestPipeline.java:48: > HttpResponseMetadataHelper metadataHelper) { > may want to make this @Nullable, and allow the hash to be omitted when > not present. > > http://codereview.appspot.com/391042/diff/1/6 > File > > > java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseMetadataHelper.java > (right): > > http://codereview.appspot.com/391042/diff/1/6#newcode48 > > java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseMetadataHelper.java:48: > public static HttpResponse updateMetadata(HttpResponse response, > Map<String, String> values) { > will this method ever be used with more than 1 k/v pair? or by any > calling context that's not within this class itself? >
I was thinking that the BasicImageRewriter will use that function to update both width and height using this function. > http://codereview.appspot.com/391042/diff/1/6#newcode61 > > java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseMetadataHelper.java:61: > public HttpResponse updateHash(HttpResponse response) { > this method doesn't really use any class state, so you could either: > A) make it static as well > B) refactor the top of this method into String getHash(HttpResponse > response), which in turn can be mocked (or overridden) in the > DefaultRequestPipeline test so you don't have to calculate data hash to > evaluate the test. > > I'd lean toward the latter. I will refactor according to B and do a unit test instead of integration just for you ;-) > > http://codereview.appspot.com/391042/show >
