Enable QUIC for non core data saver proxies

Enable QUIC for non core data saver proxies

This is guarded behind a field trial experiment.

BUG=720163
Review-Url: https://codereview.chromium.org/2869863002
Cr-Commit-Position: refs/heads/master@{#470791}
Committed: https://chromium.googlesource.com/chromium/src/+/13a38a39ce74d1524bca95c1f4c17b03e078af76

[tbansal1, 2017-05-08 17:23:36]

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

[commit-bot: I haz the power, 2017-05-08 17:24:00]

Dry run: CQ is trying da patch.

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

[tbansal1, 2017-05-08 18:20:59]

Description was changed from

==========
Enable QUIC for non core data saver proxies

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

to

==========
Enable QUIC for non core data saver proxies

This is guarded behind a field trial experiment.

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

[commit-bot: I haz the power, 2017-05-08 18:54:14]

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

[commit-bot: I haz the power, 2017-05-08 18:54:15]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2017-05-08 23:13:29]

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

[tbansal1, 2017-05-08 23:13:29]

megjablon: ptal when you have time. Thanks.

[megjablon, 2017-05-09 18:16:23]

Please add a bug.

https://codereview.chromium.org/2869863002/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/common/data_reduction_proxy_params_unittest.cc
(right):

https://codereview.chromium.org/2869863002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/common/data_reduction_proxy_params_unittest.cc:469:
{"Enabled_Control", true, "true", true},
What is the difference between "Enabled_Control" vs "Control"? These names are a
little confusing. Can we rename them to something clearer or add comments?

https://codereview.chromium.org/2869863002/diff/1/tools/chrome_proxy/webdrive...
File tools/chrome_proxy/webdriver/quic.py (right):

https://codereview.chromium.org/2869863002/diff/1/tools/chrome_proxy/webdrive...
tools/chrome_proxy/webdriver/quic.py:27: # of Chrome.
Can you add the version for this change? Something like "via switch for versions
of Chrome M59 and prior."

https://codereview.chromium.org/2869863002/diff/1/tools/chrome_proxy/webdrive...
tools/chrome_proxy/webdriver/quic.py:48: # of Chrome.
Same here.

[tbansal1, 2017-05-09 23:57:05]

Description was changed from

==========
Enable QUIC for non core data saver proxies

This is guarded behind a field trial experiment.

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

to

==========
Enable QUIC for non core data saver proxies

This is guarded behind a field trial experiment.

BUG=720163
==========

[tbansal1, 2017-05-09 23:57:41]

megjablon: Thanks for the comments. PTAL at the latest patch.

https://codereview.chromium.org/2869863002/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/common/data_reduction_proxy_params_unittest.cc
(right):

https://codereview.chromium.org/2869863002/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/common/data_reduction_proxy_params_unittest.cc:469:
{"Enabled_Control", true, "true", true},
On 2017/05/09 18:16:22, megjablon wrote:
> What is the difference between "Enabled_Control" vs "Control"? These names are
a
> little confusing. Can we rename them to something clearer or add comments?

I just removed them to avoid confusion since that part is also tested in
QuicFieldTrial test above.

https://codereview.chromium.org/2869863002/diff/1/tools/chrome_proxy/webdrive...
File tools/chrome_proxy/webdriver/quic.py (right):

https://codereview.chromium.org/2869863002/diff/1/tools/chrome_proxy/webdrive...
tools/chrome_proxy/webdriver/quic.py:27: # of Chrome.
On 2017/05/09 18:16:22, megjablon wrote:
> Can you add the version for this change? Something like "via switch for
versions
> of Chrome M59 and prior." 

Done.

https://codereview.chromium.org/2869863002/diff/1/tools/chrome_proxy/webdrive...
tools/chrome_proxy/webdriver/quic.py:48: # of Chrome.
On 2017/05/09 18:16:22, megjablon wrote:
> Same here.

Done.

[tbansal1, 2017-05-09 23:58:04]

tbansal@chromium.org changed reviewers:
+ robertogden@google.com

[tbansal1, 2017-05-09 23:58:04]

robertogden: ptal at quic.py. Thanks.

[Robert Ogden, 2017-05-10 15:15:23]

robertogden@chromium.org changed reviewers:
+ robertogden@chromium.org

[Robert Ogden, 2017-05-10 15:15:24]

lgtm

[tbansal1, 2017-05-10 16:39:00]

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

[commit-bot: I haz the power, 2017-05-10 16:39:54]

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, 2017-05-10 18:17:33]

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

[commit-bot: I haz the power, 2017-05-10 18:17:34]

Dry run: This issue passed the CQ dry run.

[megjablon, 2017-05-10 23:11:24]

lgtm % suggestion and test question

https://codereview.chromium.org/2869863002/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc
(right):

https://codereview.chromium.org/2869863002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc:247:
return params::IsQuicForNonCoreProxiesEnabled() ||
suggestion: IsQuicEnabledForNonCoreProxies reads a little better

https://codereview.chromium.org/2869863002/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/common/data_reduction_proxy_params_unittest.cc
(right):

https://codereview.chromium.org/2869863002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_params_unittest.cc:471:
{"Control", false, "true", true},
Shouldn't this case not happen since IsIncludedInQuicFieldTrial should be true
for noncore proxies to be enabled?

[tbansal1, 2017-05-11 01:29:28]

https://codereview.chromium.org/2869863002/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc
(right):

https://codereview.chromium.org/2869863002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc:247:
return params::IsQuicForNonCoreProxiesEnabled() ||
On 2017/05/10 23:11:24, megjablon wrote:
> suggestion: IsQuicEnabledForNonCoreProxies reads a little better

Done.

https://codereview.chromium.org/2869863002/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/common/data_reduction_proxy_params_unittest.cc
(right):

https://codereview.chromium.org/2869863002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_params_unittest.cc:471:
{"Control", false, "true", true},
On 2017/05/10 23:11:24, megjablon wrote:
> Shouldn't this case not happen since IsIncludedInQuicFieldTrial should be true
> for noncore proxies to be enabled?

Right. When QUIC is not enabled, behavior of IsQuicForNonCoreProxiesEnabled() is
undefined.
IsQuicForNonCoreProxiesEnabled() should be called only when QUIC is enabled.
There is a DCHECK in IsQuicForNonCoreProxiesEnabled() which ensures that.

[tbansal1, 2017-05-11 01:29:32]

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

[commit-bot: I haz the power, 2017-05-11 01:30:37]

Dry run: CQ is trying da patch.

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

[tbansal1, 2017-05-11 01:31:46]

The CQ bit was unchecked by tbansal@chromium.org

[tbansal1, 2017-05-11 01:31:50]

The CQ bit was checked by tbansal@chromium.org

[tbansal1, 2017-05-11 01:31:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
robertogden@chromium.org, megjablon@chromium.org
Link to the patchset: https://codereview.chromium.org/2869863002/#ps80001
(title: "Reorder functions")

[commit-bot: I haz the power, 2017-05-11 01:33:21]

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, 2017-05-11 03:16:09]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494466310520930,
"parent_rev": "df33f5f39300a46b43eefb4f8940fac99b37b491", "commit_rev":
"13a38a39ce74d1524bca95c1f4c17b03e078af76"}

[commit-bot: I haz the power, 2017-05-11 03:16:18]

Description was changed from

==========
Enable QUIC for non core data saver proxies

This is guarded behind a field trial experiment.

BUG=720163
==========

to

==========
Enable QUIC for non core data saver proxies

This is guarded behind a field trial experiment.

BUG=720163

Review-Url: https://codereview.chromium.org/2869863002
Cr-Commit-Position: refs/heads/master@{#470791}
Committed:
https://chromium.googlesource.com/chromium/src/+/13a38a39ce74d1524bca95c1f4c1...
==========

[commit-bot: I haz the power, 2017-05-11 03:16:21]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/13a38a39ce74d1524bca95c1f4c1...