Cleanup DataReductionProxy (DRP) Brotli code

Cleanup DataReductionProxy (DRP) Brotli code

Add thread checkers to request options class.

Force the generation of the chrome-proxy header, and use it later when
generating the MockWrite for the sockets in tests.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

BUG=634061
Committed: https://crrev.com/1d20801e2b126cfec1d66cb78c05082d0f90b3ba
Cr-Commit-Position: refs/heads/master@{#438978}

[tbansal1, 2016-12-14 00:55:55]

Description was changed from

==========
DRP brotli cleanup

BUG=
==========

to

==========
DRP brotli cleanup

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

BUG=634061
==========

[tbansal1, 2016-12-14 01:00:48]

Patchset #1 (id:1) has been deleted

[tbansal1, 2016-12-14 01:04:52]

Patchset #1 (id:20001) has been deleted

[tbansal1, 2016-12-14 01:05:50]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-14 01:06:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tbansal1, 2016-12-14 01:13:00]

Patchset #1 (id:40001) has been deleted

[tbansal1, 2016-12-14 01:19:49]

Patchset #1 (id:60001) has been deleted

[tbansal1, 2016-12-14 01:21:07]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-14 01:21:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tbansal1, 2016-12-14 01:39:21]

Description was changed from

==========
DRP brotli cleanup

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

BUG=634061
==========

to

==========
Cleanup DataReductionProxy (DRP) Brotli code

Add thread checkers to request options class

Force the generation of the chrome-proxy header, and use it later when
generating the MockWrite for the sockets in tests.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

BUG=634061
==========

[tbansal1, 2016-12-14 01:39:30]

Description was changed from

==========
Cleanup DataReductionProxy (DRP) Brotli code

Add thread checkers to request options class

Force the generation of the chrome-proxy header, and use it later when
generating the MockWrite for the sockets in tests.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

BUG=634061
==========

to

==========
Cleanup DataReductionProxy (DRP) Brotli code

Add thread checkers to request options class.

Force the generation of the chrome-proxy header, and use it later when
generating the MockWrite for the sockets in tests.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

BUG=634061
==========

[tbansal1, 2016-12-14 01:39:46]

tbansal@chromium.org changed reviewers:
+ ryansturm@chromium.org

[tbansal1, 2016-12-14 01:39:46]

ryansturm: ptal. Thanks.

[commit-bot: I haz the power, 2016-12-14 01:58:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-14 01:58:55]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tbansal1, 2016-12-14 02:01:20]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[tbansal1, 2016-12-14 02:01:25]

Patchset #1 (id:80001) has been deleted

[commit-bot: I haz the power, 2016-12-14 02:02:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-14 03:03:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-14 03:03:19]

Dry run: This issue passed the CQ dry run.

[RyanSturm, 2016-12-15 20:34:28]

lgtm % nits

https://codereview.chromium.org/2571993002/diff/100001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc
(left):

https://codereview.chromium.org/2571993002/diff/100001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:1077:
FetchURLRequestAndVerifyBrotli(nullptr, response_headers, true, false);
Why did you drop the expect_cached case? From the perspective of calling this
method without knowing its full implementation, you should still call this even
if only (..., true, true) has different behavior from the other possibilities.

https://codereview.chromium.org/2571993002/diff/100001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc
(right):

https://codereview.chromium.org/2571993002/diff/100001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:325:
std::string(
nit: make the two /r/n/r/n strings consistent.

https://codereview.chromium.org/2571993002/diff/100001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:331:
std::string(
Can you make Get, Host, Proxy-connection, user agent all in one temp variable so
that this long string isn't repeated.

Also, consider making the Accept-language header its own variable to be
referenced.

Those two changes will make it really obvious what is different between the two,
but you might as well add a comment to point out the difference.

[tbansal1, 2016-12-15 21:26:05]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[tbansal1, 2016-12-15 21:26:48]

https://codereview.chromium.org/2571993002/diff/100001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc
(left):

https://codereview.chromium.org/2571993002/diff/100001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:1077:
FetchURLRequestAndVerifyBrotli(nullptr, response_headers, true, false);
On 2016/12/15 20:34:28, Ryan Sturm wrote:
> Why did you drop the expect_cached case? From the perspective of calling this
> method without knowing its full implementation, you should still call this
even
> if only (..., true, true) has different behavior from the other possibilities.

Done.

https://codereview.chromium.org/2571993002/diff/100001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc
(right):

https://codereview.chromium.org/2571993002/diff/100001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:325:
std::string(
On 2016/12/15 20:34:28, Ryan Sturm wrote:
> nit: make the two /r/n/r/n strings consistent.

Done.

https://codereview.chromium.org/2571993002/diff/100001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:331:
std::string(
On 2016/12/15 20:34:28, Ryan Sturm wrote:
> Can you make Get, Host, Proxy-connection, user agent all in one temp variable
so
> that this long string isn't repeated.
> 
> Also, consider making the Accept-language header its own variable to be
> referenced.
> 
> Those two changes will make it really obvious what is different between the
two,
> but you might as well add a comment to point out the difference.

Done.

[commit-bot: I haz the power, 2016-12-15 21:26:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[RyanSturm, 2016-12-15 21:37:09]

https://codereview.chromium.org/2571993002/diff/120001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc
(right):

https://codereview.chromium.org/2571993002/diff/120001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:336:
if (!expect_cached && !expect_brotli) {
Should this be if(expect_cached || !expect_brotli)?

[tbansal1, 2016-12-15 22:09:50]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-15 22:10:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-15 23:10:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-15 23:10:34]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2016-12-16 00:34:21]

https://codereview.chromium.org/2571993002/diff/120001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc
(right):

https://codereview.chromium.org/2571993002/diff/120001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:336:
if (!expect_cached && !expect_brotli) {
On 2016/12/15 21:37:09, Ryan Sturm wrote:
> Should this be if(expect_cached || !expect_brotli)?

Done. For cached responses, it probably does not matter since the request
probably never goes to the socket layer. But, I have not dug deep into it.

[tbansal1, 2016-12-16 00:34:25]

The CQ bit was checked by tbansal@chromium.org

[tbansal1, 2016-12-16 00:34:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
ryansturm@chromium.org
Link to the patchset: https://codereview.chromium.org/2571993002/#ps140001
(title: "ps")

[commit-bot: I haz the power, 2016-12-16 00:35:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-16 01:49:49]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1481848465842970,
"parent_rev": "3032352e4606715b1f5a86aa047c53b430d7470a", "commit_rev":
"9fc70cc5fd62da94c9f47322d2f0d552e0c0ea35"}

[commit-bot: I haz the power, 2016-12-16 01:50:21]

Description was changed from

==========
Cleanup DataReductionProxy (DRP) Brotli code

Add thread checkers to request options class.

Force the generation of the chrome-proxy header, and use it later when
generating the MockWrite for the sockets in tests.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

BUG=634061
==========

to

==========
Cleanup DataReductionProxy (DRP) Brotli code

Add thread checkers to request options class.

Force the generation of the chrome-proxy header, and use it later when
generating the MockWrite for the sockets in tests.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

BUG=634061

Review-Url: https://codereview.chromium.org/2571993002
==========

[commit-bot: I haz the power, 2016-12-16 01:50:22]

Committed patchset #3 (id:140001)

[commit-bot: I haz the power, 2016-12-16 01:54:06]

Description was changed from

==========
Cleanup DataReductionProxy (DRP) Brotli code

Add thread checkers to request options class.

Force the generation of the chrome-proxy header, and use it later when
generating the MockWrite for the sockets in tests.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

BUG=634061

Review-Url: https://codereview.chromium.org/2571993002
==========

to

==========
Cleanup DataReductionProxy (DRP) Brotli code

Add thread checkers to request options class.

Force the generation of the chrome-proxy header, and use it later when
generating the MockWrite for the sockets in tests.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

BUG=634061

Committed: https://crrev.com/1d20801e2b126cfec1d66cb78c05082d0f90b3ba
Cr-Commit-Position: refs/heads/master@{#438978}
==========

[commit-bot: I haz the power, 2016-12-16 01:54:07]

Patchset 3 (id:??) landed as
https://crrev.com/1d20801e2b126cfec1d66cb78c05082d0f90b3ba
Cr-Commit-Position: refs/heads/master@{#438978}

[Mathieu, 2016-12-16 14:53:25]

A revert of this CL (patchset #3 id:140001) has been created in
https://codereview.chromium.org/2577413002/ by mathp@chromium.org.

The reason for reverting is: Reverting because it's hitting a DCHECK on top of
tree (android). It's going to make it hard for people to work.

Filed a bug at https://bugs.chromium.org/p/chromium/issues/detail?id=674918.

[tbansal1, 2016-12-16 17:38:56]

Patchset #4 (id:160001) has been deleted

[tbansal1, 2016-12-16 17:41:00]

Patchset #4 (id:180001) has been deleted

[tbansal1, 2016-12-16 19:40:57]

Patchset #4 (id:200001) has been deleted

[tbansal1, 2016-12-16 19:43:43]

Patchset #4 (id:220001) has been deleted