Issue 10102005: ABP/Android Process chunked requests (Closed)
|
Created:
April 5, 2013, 1:45 p.m. by Andrey Novikov Modified:
April 10, 2013, 8:31 a.m. Visibility:
Public. |
DescriptionABP/Android Process chunked requests
Patch Set 1 #
Total comments: 15
Patch Set 2 : ABP/Android Process chunked requests #
Total comments: 1
Patch Set 3 : ABP/Android Process chunked requests #Patch Set 4 : ABP/Android Process chunked requests #
Total comments: 5
MessagesTotal messages: 12
http://codereview.adblockplus.org/10102005/diff/1/src/org/adblockplus/brazil/... File src/org/adblockplus/brazil/RequestHandler.java (right): http://codereview.adblockplus.org/10102005/diff/1/src/org/adblockplus/brazil/... src/org/adblockplus/brazil/RequestHandler.java:188: target.setHttpInputStream(request.in); I don't think we want to do that in the general case - only if request.postData is null (meaning in the "else" clause of the if above). http://codereview.adblockplus.org/10102005/diff/1/src/sunlabs/brazil/util/htt... File src/sunlabs/brazil/util/http/HttpRequest.java (right): http://codereview.adblockplus.org/10102005/diff/1/src/sunlabs/brazil/util/htt... src/sunlabs/brazil/util/http/HttpRequest.java:677: this.cs = cs; Bad indentation here - are you mixing tabs and spaces? http://codereview.adblockplus.org/10102005/diff/1/src/sunlabs/brazil/util/htt... src/sunlabs/brazil/util/http/HttpRequest.java:882: if (encoding != null && encoding.equals("chunked") && cs != null) Should we really limit it to the chunked encoding? This probably should just check for cs. http://codereview.adblockplus.org/10102005/diff/1/src/sunlabs/brazil/util/htt... src/sunlabs/brazil/util/http/HttpRequest.java:891: bytesLeft = getChunkSize(cs); Why do we care about the encoding and make tons of assumptions while at it? byte[] buf = new byte[4096]; while (true) { int bytesRead = cs.read(buf); if (bytesRead < 0) break; p.write(buf, 0 bytesRead); } This should do the same. Does it? http://codereview.adblockplus.org/10102005/diff/1/src/sunlabs/brazil/util/htt... src/sunlabs/brazil/util/http/HttpRequest.java:905: p.write(buf, 0, count); Mixing tabs and spaces again.
http://codereview.adblockplus.org/10102005/diff/1/src/org/adblockplus/brazil/... File src/org/adblockplus/brazil/RequestHandler.java (right): http://codereview.adblockplus.org/10102005/diff/1/src/org/adblockplus/brazil/... src/org/adblockplus/brazil/RequestHandler.java:188: target.setHttpInputStream(request.in); On 2013/04/05 14:27:09, Wladimir Palant wrote: > I don't think we want to do that in the general case - only if request.postData > is null (meaning in the "else" clause of the if above). Done. http://codereview.adblockplus.org/10102005/diff/1/src/sunlabs/brazil/util/htt... File src/sunlabs/brazil/util/http/HttpRequest.java (right): http://codereview.adblockplus.org/10102005/diff/1/src/sunlabs/brazil/util/htt... src/sunlabs/brazil/util/http/HttpRequest.java:677: this.cs = cs; On 2013/04/05 14:27:09, Wladimir Palant wrote: > Bad indentation here - are you mixing tabs and spaces? No, I'm trying to preserve ugly Brazil code style. http://codereview.adblockplus.org/10102005/diff/1/src/sunlabs/brazil/util/htt... src/sunlabs/brazil/util/http/HttpRequest.java:882: if (encoding != null && encoding.equals("chunked") && cs != null) On 2013/04/05 14:27:09, Wladimir Palant wrote: > Should we really limit it to the chunked encoding? This probably should just > check for cs. Why should we not? http://codereview.adblockplus.org/10102005/diff/1/src/sunlabs/brazil/util/htt... src/sunlabs/brazil/util/http/HttpRequest.java:891: bytesLeft = getChunkSize(cs); On 2013/04/05 14:27:09, Wladimir Palant wrote: > Why do we care about the encoding and make tons of assumptions while at it? > > byte[] buf = new byte[4096]; > while (true) > { > int bytesRead = cs.read(buf); > if (bytesRead < 0) > break; > p.write(buf, 0 bytesRead); > } > > This should do the same. Does it? Because input stream never ends. I've talked about it yesterday in chat. We have a Keep-alive request. Your code (in fact I began doing it with the same code) will cause timeout error.
http://codereview.adblockplus.org/10102005/diff/6001/src/org/adblockplus/andr... File src/org/adblockplus/android/ProxyService.java (right): http://codereview.adblockplus.org/10102005/diff/6001/src/org/adblockplus/andr... src/org/adblockplus/android/ProxyService.java:855: // TODO Should we use ConnectivityManagerCompat.getNetworkInfoFromBroadcast() instead? Disregard this, it does not belong to this patch set.
http://codereview.adblockplus.org/10102005/diff/1/src/sunlabs/brazil/util/htt... File src/sunlabs/brazil/util/http/HttpRequest.java (right): http://codereview.adblockplus.org/10102005/diff/1/src/sunlabs/brazil/util/htt... src/sunlabs/brazil/util/http/HttpRequest.java:677: this.cs = cs; On 2013/04/05 14:34:28, Andrey Novikov wrote: > On 2013/04/05 14:27:09, Wladimir Palant wrote: > > Bad indentation here - are you mixing tabs and spaces? > > No, I'm trying to preserve ugly Brazil code style. Indentation is fine in the Brazil code above... http://codereview.adblockplus.org/10102005/diff/1/src/sunlabs/brazil/util/htt... src/sunlabs/brazil/util/http/HttpRequest.java:882: if (encoding != null && encoding.equals("chunked") && cs != null) Yes, if we cannot avoid parsing chunked encoding then we need to special-case it of course... http://codereview.adblockplus.org/10102005/diff/1/src/sunlabs/brazil/util/htt... src/sunlabs/brazil/util/http/HttpRequest.java:929: getChunkSize(HttpInputStream is) Feel free to add a comment: This function has been copy&paste'd from HttpRequest.java.
http://codereview.adblockplus.org/10102005/diff/1/src/sunlabs/brazil/util/htt... File src/sunlabs/brazil/util/http/HttpRequest.java (right): http://codereview.adblockplus.org/10102005/diff/1/src/sunlabs/brazil/util/htt... src/sunlabs/brazil/util/http/HttpRequest.java:929: getChunkSize(HttpInputStream is) What's the reason? I have copied it from Brazil code to Brazil code, not our code.
http://codereview.adblockplus.org/10102005/diff/1/src/sunlabs/brazil/util/htt... File src/sunlabs/brazil/util/http/HttpRequest.java (right): http://codereview.adblockplus.org/10102005/diff/1/src/sunlabs/brazil/util/htt... src/sunlabs/brazil/util/http/HttpRequest.java:929: getChunkSize(HttpInputStream is) On 2013/04/09 08:03:56, Andrey Novikov wrote: > What's the reason? I have copied it from Brazil code to Brazil code, not our > code. You forked Brazil code, later somebody will still have to understand what you did. I found the source of this function by doing a Google search, this shouldn't be necessary.
http://codereview.adblockplus.org/10102005/diff/1/src/sunlabs/brazil/util/htt... File src/sunlabs/brazil/util/http/HttpRequest.java (right): http://codereview.adblockplus.org/10102005/diff/1/src/sunlabs/brazil/util/htt... src/sunlabs/brazil/util/http/HttpRequest.java:929: getChunkSize(HttpInputStream is) But this is the same file. :) Anyway, done.
Some nits, but otherwise LGTM http://codereview.adblockplus.org/10102005/diff/17001/src/sunlabs/brazil/util... File src/sunlabs/brazil/util/http/HttpRequest.java (right): http://codereview.adblockplus.org/10102005/diff/17001/src/sunlabs/brazil/util... src/sunlabs/brazil/util/http/HttpRequest.java:885: if (encoding != null && encoding.equals("chunked") && cs != null) You could just do '"chunked".equals(encoding)' to avoid the null check. http://codereview.adblockplus.org/10102005/diff/17001/src/sunlabs/brazil/util... src/sunlabs/brazil/util/http/HttpRequest.java:887: byte[] buf = new byte[4096]; Indentation is off here http://codereview.adblockplus.org/10102005/diff/17001/src/sunlabs/brazil/util... src/sunlabs/brazil/util/http/HttpRequest.java:950: } while ((line != null) && (line.length() == 0)); Here's a shorter alternative: while("".equals(line))
http://codereview.adblockplus.org/10102005/diff/17001/src/sunlabs/brazil/util... File src/sunlabs/brazil/util/http/HttpRequest.java (right): http://codereview.adblockplus.org/10102005/diff/17001/src/sunlabs/brazil/util... src/sunlabs/brazil/util/http/HttpRequest.java:885: if (encoding != null && encoding.equals("chunked") && cs != null) On 2013/04/09 09:16:48, Felix H. Dahlke wrote: > You could just do '"chunked".equals(encoding)' to avoid the null check. Done. http://codereview.adblockplus.org/10102005/diff/17001/src/sunlabs/brazil/util... src/sunlabs/brazil/util/http/HttpRequest.java:950: } while ((line != null) && (line.length() == 0)); This was copied from another part of Brazil, I'll keep it as is for consistency.
Sure, LGTM. On 2013/04/10 08:30:30, Andrey Novikov wrote: > http://codereview.adblockplus.org/10102005/diff/17001/src/sunlabs/brazil/util... > File src/sunlabs/brazil/util/http/HttpRequest.java (right): > > http://codereview.adblockplus.org/10102005/diff/17001/src/sunlabs/brazil/util... > src/sunlabs/brazil/util/http/HttpRequest.java:885: if (encoding != null && > encoding.equals("chunked") && cs != null) > On 2013/04/09 09:16:48, Felix H. Dahlke wrote: > > You could just do '"chunked".equals(encoding)' to avoid the null check. > > Done. > > http://codereview.adblockplus.org/10102005/diff/17001/src/sunlabs/brazil/util... > src/sunlabs/brazil/util/http/HttpRequest.java:950: } while ((line != null) && > (line.length() == 0)); > This was copied from another part of Brazil, I'll keep it as is for consistency. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
