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

Issue 29377775: Issue 4916 - Request compression in CURL WebRequest. (Closed)

Created:
March 1, 2017, 10:44 p.m. by hub
Modified:
March 16, 2017, 5:10 p.m.
Reviewers:
sergei, Felix Dahlke
Base URL:
https://hg.adblockplus.org/libadblockplus/
Visibility:
Public.

Description

Issue 4916 - Request compression in CURL WebRequest.

Patch Set 1 #

Total comments: 1

Patch Set 2 : Issue 4916 - Request compression in CURL WebRequest. (updated test) #

Total comments: 6

Patch Set 3 : Updated patch to fix test. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+5 lines, -0 lines) Patch
M src/DefaultWebRequestCurl.cpp View 1 chunk +2 lines, -0 lines 0 comments Download
M test/WebRequest.cpp View 1 2 1 chunk +3 lines, -0 lines 0 comments Download

Messages

Total messages: 8
hub
March 1, 2017, 10:44 p.m. (2017-03-01 22:44:49 UTC) #1
sergei
https://codereview.adblockplus.org/29377775/diff/29377776/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29377775/diff/29377776/test/WebRequest.cpp#newcode93 test/WebRequest.cpp:93: ASSERT_EQ("gzip", jsEngine->Evaluate("foo.responseHeaders['content-encoding'].substr(0, 10)")->AsString()); I think instead of 10 there ...
March 2, 2017, 8:47 a.m. (2017-03-02 08:47:30 UTC) #2
hub
On 2017/03/02 08:47:30, sergei wrote: > https://codereview.adblockplus.org/29377775/diff/29377776/test/WebRequest.cpp > File test/WebRequest.cpp (right): > > https://codereview.adblockplus.org/29377775/diff/29377776/test/WebRequest.cpp#newcode93 > ...
March 2, 2017, 1:48 p.m. (2017-03-02 13:48:02 UTC) #3
sergei
LGTM
March 2, 2017, 1:49 p.m. (2017-03-02 13:49:00 UTC) #4
Felix Dahlke
https://codereview.adblockplus.org/29377775/diff/29377796/src/DefaultWebRequestCurl.cpp File src/DefaultWebRequestCurl.cpp (right): https://codereview.adblockplus.org/29377775/diff/29377796/src/DefaultWebRequestCurl.cpp#newcode142 src/DefaultWebRequestCurl.cpp:142: curl_easy_setopt(curl, CURLOPT_ACCEPT_ENCODING, ""); Won't it be possible to overwrite ...
March 2, 2017, 2:33 p.m. (2017-03-02 14:33:39 UTC) #5
hub
https://codereview.adblockplus.org/29377775/diff/29377796/src/DefaultWebRequestCurl.cpp File src/DefaultWebRequestCurl.cpp (right): https://codereview.adblockplus.org/29377775/diff/29377796/src/DefaultWebRequestCurl.cpp#newcode142 src/DefaultWebRequestCurl.cpp:142: curl_easy_setopt(curl, CURLOPT_ACCEPT_ENCODING, ""); On 2017/03/02 14:33:38, Felix Dahlke wrote: ...
March 2, 2017, 3:36 p.m. (2017-03-02 15:36:00 UTC) #6
Felix Dahlke
LGTM https://codereview.adblockplus.org/29377775/diff/29377796/src/DefaultWebRequestCurl.cpp File src/DefaultWebRequestCurl.cpp (right): https://codereview.adblockplus.org/29377775/diff/29377796/src/DefaultWebRequestCurl.cpp#newcode142 src/DefaultWebRequestCurl.cpp:142: curl_easy_setopt(curl, CURLOPT_ACCEPT_ENCODING, ""); On 2017/03/02 15:36:00, hub wrote: ...
March 2, 2017, 3:39 p.m. (2017-03-02 15:39:47 UTC) #7
sergei
March 2, 2017, 9:05 p.m. (2017-03-02 21:05:26 UTC) #8
LGTM

https://codereview.adblockplus.org/29377775/diff/29377796/test/WebRequest.cpp
File test/WebRequest.cpp (right):

https://codereview.adblockplus.org/29377775/diff/29377796/test/WebRequest.cpp...
test/WebRequest.cpp:93: ASSERT_EQ("gzip",
jsEngine->Evaluate("foo.responseHeaders['content-encoding'].substr(0,
5)")->AsString());
On 2017/03/02 14:33:39, Felix Dahlke wrote:
> Nit: Given how "gzip" is a four character string, wouldn't this be `substr(0,
> 4)` then? I might be missing something though.

Sorry, my bad, off by one error.

Powered by Google App Engine
This is Rietveld