Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code
Issues
All Issues

Issue 29382555: Issue 4977 - Request for compressed response in AndroidWebRequest (Closed)

Created:
March 13, 2017, 6:59 a.m. by anton
Modified:
March 28, 2017, 10:07 a.m.
CC:
sergei, vicky, René Jeschke
Visibility:
Public.

Description

Issue 4977 - Request for compressed response in AndroidWebRequest

Patch Set 1 #

Total comments: 6

Patch Set 2 : diego's suggestions #

Total comments: 7

Patch Set 3 : updated test #

Unified diffs Side-by-side diffs Delta from patch set Stats (+26 lines, -6 lines) Patch
M libadblockplus-android-tests/src/org/adblockplus/libadblockplus/tests/AndroidWebRequestTest.java View 1 2 1 chunk +1 line, -1 line 0 comments Download
M libadblockplus-android/src/org/adblockplus/libadblockplus/android/AdblockEngine.java View 1 chunk +1 line, -1 line 0 comments Download
M libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidWebRequest.java View 1 5 chunks +24 lines, -4 lines 0 comments Download

Messages

Total messages: 7
anton
https://codereview.adblockplus.org/29382555/diff/29382556/libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidWebRequest.java File libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidWebRequest.java (right): https://codereview.adblockplus.org/29382555/diff/29382556/libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidWebRequest.java#newcode66 libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidWebRequest.java:66: this(false, true); i believe we should apply gzipping by ...
March 13, 2017, 7:03 a.m. (2017-03-13 07:03:34 UTC) #1
diegocarloslima
LGTM with 2 small nits regarding the javadoc https://codereview.adblockplus.org/29382555/diff/29382556/libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidWebRequest.java File libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidWebRequest.java (right): https://codereview.adblockplus.org/29382555/diff/29382556/libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidWebRequest.java#newcode54 libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidWebRequest.java:54: * ...
March 16, 2017, 7:49 p.m. (2017-03-16 19:49:23 UTC) #2
anton
https://codereview.adblockplus.org/29382555/diff/29382556/libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidWebRequest.java File libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidWebRequest.java (right): https://codereview.adblockplus.org/29382555/diff/29382556/libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidWebRequest.java#newcode54 libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidWebRequest.java:54: * Element hiding required significantly more memory On 2017/03/16 ...
March 17, 2017, 5:54 a.m. (2017-03-17 05:54:44 UTC) #3
Felix Dahlke
Looks good, minor stuff. https://codereview.adblockplus.org/29382555/diff/29386555/libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidWebRequest.java File libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidWebRequest.java (right): https://codereview.adblockplus.org/29382555/diff/29386555/libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidWebRequest.java#newcode58 libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidWebRequest.java:58: public AndroidWebRequest(boolean enableElemhide, boolean compressedStream) ...
March 27, 2017, 2:13 p.m. (2017-03-27 14:13:16 UTC) #4
anton
https://codereview.adblockplus.org/29382555/diff/29386555/libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidWebRequest.java File libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidWebRequest.java (right): https://codereview.adblockplus.org/29382555/diff/29386555/libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidWebRequest.java#newcode58 libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidWebRequest.java:58: public AndroidWebRequest(boolean enableElemhide, boolean compressedStream) On 2017/03/27 14:13:16, Felix ...
March 27, 2017, 6:56 p.m. (2017-03-27 18:56:13 UTC) #5
anton
https://codereview.adblockplus.org/29382555/diff/29386555/libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidWebRequest.java File libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidWebRequest.java (right): https://codereview.adblockplus.org/29382555/diff/29386555/libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidWebRequest.java#newcode58 libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidWebRequest.java:58: public AndroidWebRequest(boolean enableElemhide, boolean compressedStream) On 2017/03/27 18:56:13, anton ...
March 28, 2017, 7:18 a.m. (2017-03-28 07:18:54 UTC) #6
Felix Dahlke
March 28, 2017, 8:52 a.m. (2017-03-28 08:52:00 UTC) #7 
LGTM

https://codereview.adblockplus.org/29382555/diff/29386555/libadblockplus-andr...
File
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidWebRequest.java
(right):

https://codereview.adblockplus.org/29382555/diff/29386555/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidWebRequest.java:58:
public AndroidWebRequest(boolean enableElemhide, boolean compressedStream)
On 2017/03/28 07:18:54, anton wrote:
> On 2017/03/27 18:56:13, anton wrote:
> > On 2017/03/27 14:13:16, Felix Dahlke wrote:
> > > I don't really see a use case for passing `compressedStream` as `false` -
> why
> > > not always compress?
> > 
> > The server can not support it or some or compressing takes more battery -
who
> > knows why it can be undesired.
> > In ctor with no arguments i use `true` it seems to be the best by default.
> 
> * decompressing (on user's device)

If the server doesn't support compression it'll just return an uncompressed
response. But yeah, maybe clients will want to overwrite this for reasons we
cannot think of, fine by me if we leave it like this.

https://codereview.adblockplus.org/29382555/diff/29386555/libadblockplus-andr...
libadblockplus-android/src/org/adblockplus/libadblockplus/android/AndroidWebRequest.java:102:
(compressedStream ? ENCODING_GZIP : ENCODING_IDENTITY));
On 2017/03/27 18:56:13, anton wrote:
> On 2017/03/27 14:13:16, Felix Dahlke wrote:
> > Why not just leave the `Accept-Encoding` header out if `compressedStream` is
> > false? That's how the behaviour was before.
> 
> Not true sometimes, previously it depended on android version:
> https://android-developers.googleblog.com/2011/09/androids-http-clients.html
> "In Gingerbread, we added transparent response compression"
> 
> So i decided to add ENCODING_IDENTITY if the compressed stream is not desired
> for some reason.

Acknowledged.

Powered by Google App Engine
This is Rietveld