Thanks for the feedback, Ziv! Comments inline. On Mon, Jun 14, 2010 at 4:56 PM, <[email protected]> wrote:
> And don't forget to write tests... > > > http://codereview.appspot.com/1626044/diff/5001/6002 > File > > java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java > (right): > > http://codereview.appspot.com/1626044/diff/5001/6002#newcode498 > > java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponse.java:498: > if (body == null || !(body instanceof > MutableContent.ByteArrayContentBytes)) { > I would postpond detection to the time you need it. > The encoding is really needed only in the case of batch request, so at > that point you should have all data. > It's actually "needed" in one critically important other way -- as a side effect of setting the Content-Type for the HttpResponse object. About to upload a patch that maintains these semantics while lazy-parsing the encoding when needed. > > http://codereview.appspot.com/1626044/diff/5001/6003 > File > > java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/MutableContent.java > (right): > > http://codereview.appspot.com/1626044/diff/5001/6003#newcode296 > > java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/MutableContent.java:296: > bufferedStream = IOUtils.toByteArray(getStream()); > What if someone already ask to get the stream? > Barf, by design :) > > http://codereview.appspot.com/1626044/diff/5001/6003#newcode301 > > java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/MutableContent.java:301: > return bufferedStream; > Set streamReturned to true > At this point, it's guaranteed to already be set to true. > > http://codereview.appspot.com/1626044/diff/5001/6003#newcode314 > > java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/MutableContent.java:314: > return stream; > Set streamReturned to true > Done. > > http://codereview.appspot.com/1626044/show >
