Provide default thresholds for effective connection types

Provide default thresholds for effective connection types

Provide default HTTP RTT and transport RTT thresholds for
Slow2G and 2G effective connection types. The default
thresholds can still be overriden using field trial
variation params.

Advantage of this approach is that we would no longer need
to specify the variation params in the field trial configs
(keeps the configs cleaner). Also, it enables net stack
embedders such as Cronet that do not have the concept of
field trial to also compute ECT because the thresholds
would be available in the code itself.

BUG=625305
Committed: https://crrev.com/2739868cb29af2c9978389c38c437899dd827208
Cr-Commit-Position: refs/heads/master@{#407993}

[tbansal1, 2016-07-22 20:15:56]

Patchset #1 (id:1) has been deleted

[tbansal1, 2016-07-22 20:20:01]

Description was changed from

==========
Provide default thresholds for Slow2G and 2G effective connection types

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

to

==========
Provide default thresholds for effective connection types

Provide default HTTP RTT and transport RTT thresholds for 
Slow2G and 2G effective connection types. The default thresholds can still be
overriden using field trial variation params.

Advantage of this approach is that we would no longer need 
to specify the variation params in the field trial configs 
(keeps the configs cleaner). Also, it enables net stack 
embedders such as Cronet that do not have the concept of 
field trial to also compute ECT because the thresholds 
would be available in the code itself.

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

[tbansal1, 2016-07-22 20:20:27]

Description was changed from

==========
Provide default thresholds for effective connection types

Provide default HTTP RTT and transport RTT thresholds for 
Slow2G and 2G effective connection types. The default thresholds can still be
overriden using field trial variation params.

Advantage of this approach is that we would no longer need 
to specify the variation params in the field trial configs 
(keeps the configs cleaner). Also, it enables net stack 
embedders such as Cronet that do not have the concept of 
field trial to also compute ECT because the thresholds 
would be available in the code itself.

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

to

==========
Provide default thresholds for effective connection types

Provide default HTTP RTT and transport RTT thresholds for 
Slow2G and 2G effective connection types. The default 
thresholds can still be overriden using field trial variation params.

Advantage of this approach is that we would no longer need 
to specify the variation params in the field trial configs 
(keeps the configs cleaner). Also, it enables net stack 
embedders such as Cronet that do not have the concept of 
field trial to also compute ECT because the thresholds 
would be available in the code itself.

BUG=625305
==========

[tbansal1, 2016-07-22 20:20:41]

Description was changed from

==========
Provide default thresholds for effective connection types

Provide default HTTP RTT and transport RTT thresholds for 
Slow2G and 2G effective connection types. The default 
thresholds can still be overriden using field trial variation params.

Advantage of this approach is that we would no longer need 
to specify the variation params in the field trial configs 
(keeps the configs cleaner). Also, it enables net stack 
embedders such as Cronet that do not have the concept of 
field trial to also compute ECT because the thresholds 
would be available in the code itself.

BUG=625305
==========

to

==========
Provide default thresholds for effective connection types

Provide default HTTP RTT and transport RTT thresholds for 
Slow2G and 2G effective connection types. The default 
thresholds can still be overriden using field trial 
variation params.

Advantage of this approach is that we would no longer need 
to specify the variation params in the field trial configs 
(keeps the configs cleaner). Also, it enables net stack 
embedders such as Cronet that do not have the concept of 
field trial to also compute ECT because the thresholds 
would be available in the code itself.

BUG=625305
==========

[tbansal1, 2016-07-22 20:21:32]

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

[tbansal1, 2016-07-22 20:21:32]

ryansturm: PTAL at *. This is factored out from
https://codereview.chromium.org/2146563002/ (with some changes) that you
reviewed before. Thanks!

[RyanSturm, 2016-07-22 21:01:16]

https://codereview.chromium.org/2172403002/diff/20001/net/nqe/network_quality...
File net/nqe/network_quality_estimator.cc (right):

https://codereview.chromium.org/2172403002/diff/20001/net/nqe/network_quality...
net/nqe/network_quality_estimator.cc:381: // Set to 1870 milliseconds which
corresponds to the below 33rd
s/the below 33rd/the 33rd/

https://codereview.chromium.org/2172403002/diff/20001/net/nqe/network_quality...
net/nqe/network_quality_estimator.cc:417:
base::TimeDelta(base::TimeDelta::FromMilliseconds(variations_value)));
Why are you constructing a TimeDelta from a TimeDelta? You should only need
base::TimeDelta::FromMilliseconds. Same elsewhere.

https://codereview.chromium.org/2172403002/diff/20001/net/nqe/network_quality...
File net/nqe/network_quality_estimator.h (right):

https://codereview.chromium.org/2172403002/diff/20001/net/nqe/network_quality...
net/nqe/network_quality_estimator.h:612:
default_ect_thresholds_[EFFECTIVE_CONNECTION_TYPE_LAST];
nit: s/default_ect_thresholds/default_effective_connection_type_thresholds/

It might be too long, but it would be a lot clearer.

[tbansal1, 2016-07-22 21:35:50]

ryansturm: ptal. Thanks.

https://codereview.chromium.org/2172403002/diff/20001/net/nqe/network_quality...
File net/nqe/network_quality_estimator.cc (right):

https://codereview.chromium.org/2172403002/diff/20001/net/nqe/network_quality...
net/nqe/network_quality_estimator.cc:381: // Set to 1870 milliseconds which
corresponds to the below 33rd
On 2016/07/22 21:01:16, RyanSturm wrote:
> s/the below 33rd/the 33rd/

Done.

https://codereview.chromium.org/2172403002/diff/20001/net/nqe/network_quality...
net/nqe/network_quality_estimator.cc:417:
base::TimeDelta(base::TimeDelta::FromMilliseconds(variations_value)));
On 2016/07/22 21:01:16, RyanSturm wrote:
> Why are you constructing a TimeDelta from a TimeDelta? You should only need
> base::TimeDelta::FromMilliseconds. Same elsewhere.

Done.

https://codereview.chromium.org/2172403002/diff/20001/net/nqe/network_quality...
File net/nqe/network_quality_estimator.h (right):

https://codereview.chromium.org/2172403002/diff/20001/net/nqe/network_quality...
net/nqe/network_quality_estimator.h:612:
default_ect_thresholds_[EFFECTIVE_CONNECTION_TYPE_LAST];
On 2016/07/22 21:01:16, RyanSturm wrote:
> nit: s/default_ect_thresholds/default_effective_connection_type_thresholds/
> 
> It might be too long, but it would be a lot clearer.

Done.

[RyanSturm, 2016-07-22 21:45:44]

lgtm % a nit.

https://codereview.chromium.org/2172403002/diff/40001/net/nqe/network_quality...
File net/nqe/network_quality_estimator_unittest.cc (right):

https://codereview.chromium.org/2172403002/diff/40001/net/nqe/network_quality...
net/nqe/network_quality_estimator_unittest.cc:838: // Thresholds are not set
using |variation_params|. Default values should be
This comment confuses me, and the space below it seems weird. Same with above.

[tbansal1, 2016-07-22 21:51:22]

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

[tbansal1, 2016-07-22 21:51:23]

bengr: ptal. Thanks.

https://codereview.chromium.org/2172403002/diff/40001/net/nqe/network_quality...
File net/nqe/network_quality_estimator_unittest.cc (right):

https://codereview.chromium.org/2172403002/diff/40001/net/nqe/network_quality...
net/nqe/network_quality_estimator_unittest.cc:838: // Thresholds are not set
using |variation_params|. Default values should be
On 2016/07/22 21:45:43, RyanSturm wrote:
> This comment confuses me, and the space below it seems weird. Same with above.

Done.

[tbansal1, 2016-07-25 20:35:25]

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

[commit-bot: I haz the power, 2016-07-25 20:36:13]

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-07-25 21:52:24]

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

[commit-bot: I haz the power, 2016-07-25 21:52:25]

Dry run: This issue passed the CQ dry run.

[bengr, 2016-07-26 23:14:51]

lgtm with nits.

https://codereview.chromium.org/2172403002/diff/80001/net/nqe/network_quality...
File net/nqe/network_quality_estimator.cc (right):

https://codereview.chromium.org/2172403002/diff/80001/net/nqe/network_quality...
net/nqe/network_quality_estimator.cc:432: // Set to 2010 milliseconds which
corresponds to the 33rd percentile
nit: milliseconds, which

everywhere.

https://codereview.chromium.org/2172403002/diff/80001/net/nqe/network_quality...
net/nqe/network_quality_estimator.cc:477: connection_thresholds_[i].http_rtt()
==
It might be clearer and reduce redundancy if you had a helper method for these
comparisons. I'm ok with it as is, but your methods tend to get uncomfortably
long.

[tbansal1, 2016-07-26 23:42:24]

https://codereview.chromium.org/2172403002/diff/80001/net/nqe/network_quality...
File net/nqe/network_quality_estimator.cc (right):

https://codereview.chromium.org/2172403002/diff/80001/net/nqe/network_quality...
net/nqe/network_quality_estimator.cc:432: // Set to 2010 milliseconds which
corresponds to the 33rd percentile
On 2016/07/26 23:14:51, bengr wrote:
> nit: milliseconds, which
> 
> everywhere.

Done.

https://codereview.chromium.org/2172403002/diff/80001/net/nqe/network_quality...
net/nqe/network_quality_estimator.cc:477: connection_thresholds_[i].http_rtt()
==
On 2016/07/26 23:14:51, bengr wrote:
> It might be clearer and reduce redundancy if you had a helper method for these
> comparisons. I'm ok with it as is, but your methods tend to get uncomfortably
> long.

Done.

[tbansal1, 2016-07-26 23:42:28]

The CQ bit was checked by tbansal@chromium.org

[tbansal1, 2016-07-26 23:42:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
ryansturm@chromium.org, bengr@chromium.org
Link to the patchset: https://codereview.chromium.org/2172403002/#ps120001
(title: "bengr comments")

[commit-bot: I haz the power, 2016-07-26 23:42:59]

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-07-27 00:44:40]

Description was changed from

==========
Provide default thresholds for effective connection types

Provide default HTTP RTT and transport RTT thresholds for 
Slow2G and 2G effective connection types. The default 
thresholds can still be overriden using field trial 
variation params.

Advantage of this approach is that we would no longer need 
to specify the variation params in the field trial configs 
(keeps the configs cleaner). Also, it enables net stack 
embedders such as Cronet that do not have the concept of 
field trial to also compute ECT because the thresholds 
would be available in the code itself.

BUG=625305
==========

to

==========
Provide default thresholds for effective connection types

Provide default HTTP RTT and transport RTT thresholds for 
Slow2G and 2G effective connection types. The default 
thresholds can still be overriden using field trial 
variation params.

Advantage of this approach is that we would no longer need 
to specify the variation params in the field trial configs 
(keeps the configs cleaner). Also, it enables net stack 
embedders such as Cronet that do not have the concept of 
field trial to also compute ECT because the thresholds 
would be available in the code itself.

BUG=625305
==========

[commit-bot: I haz the power, 2016-07-27 00:44:41]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2016-07-27 00:48:15]

Description was changed from

==========
Provide default thresholds for effective connection types

Provide default HTTP RTT and transport RTT thresholds for 
Slow2G and 2G effective connection types. The default 
thresholds can still be overriden using field trial 
variation params.

Advantage of this approach is that we would no longer need 
to specify the variation params in the field trial configs 
(keeps the configs cleaner). Also, it enables net stack 
embedders such as Cronet that do not have the concept of 
field trial to also compute ECT because the thresholds 
would be available in the code itself.

BUG=625305
==========

to

==========
Provide default thresholds for effective connection types

Provide default HTTP RTT and transport RTT thresholds for
Slow2G and 2G effective connection types. The default
thresholds can still be overriden using field trial
variation params.

Advantage of this approach is that we would no longer need
to specify the variation params in the field trial configs
(keeps the configs cleaner). Also, it enables net stack
embedders such as Cronet that do not have the concept of
field trial to also compute ECT because the thresholds
would be available in the code itself.

BUG=625305

Committed: https://crrev.com/2739868cb29af2c9978389c38c437899dd827208
Cr-Commit-Position: refs/heads/master@{#407993}
==========

[commit-bot: I haz the power, 2016-07-27 00:48:16]

Patchset 6 (id:??) landed as
https://crrev.com/2739868cb29af2c9978389c38c437899dd827208
Cr-Commit-Position: refs/heads/master@{#407993}