NQE: Add default RTT and throughput observations

NQE: Add default RTT and throughput observations

Add default estimates for different connection types in the network
quality estimator (NQE). Putting the estimates in the src code itself
makes them accessible for Cronet embedders, and simplies the field
trial configs.

Additionally, RTT and throughput observers are notified of the default
observations as soon as they are added

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

BUG=638304
Committed: https://crrev.com/e71312541dfbef6a53b0dd81923e9db1816dfc36
Cr-Commit-Position: refs/heads/master@{#437396}

[tbansal1, 2016-11-08 02:29:23]

Description was changed from

==========
Add default estimates

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

to

==========
Add default estimates

BUG=638304
==========

[tbansal1, 2016-11-08 02:29:43]

Description was changed from

==========
Add default estimates

BUG=638304
==========

to

==========
Add default estimates

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

BUG=638304
==========

[tbansal1, 2016-11-08 05:17:30]

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

[commit-bot: I haz the power, 2016-11-08 05:17:40]

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-11-08 05:30:25]

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

[commit-bot: I haz the power, 2016-11-08 05:30:26]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)

[tbansal1, 2016-11-08 06:06:23]

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

[commit-bot: I haz the power, 2016-11-08 06:06:36]

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-11-08 07:02:35]

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

[commit-bot: I haz the power, 2016-11-08 07:02:36]

Dry run: Try jobs failed on following builders:
  android_cronet_tester on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)

[tbansal1, 2016-11-08 17:16:43]

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

[commit-bot: I haz the power, 2016-11-08 17:17: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-11-08 18:01:00]

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

[commit-bot: I haz the power, 2016-11-08 18:01:00]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tbansal1, 2016-11-08 21:56:53]

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

[commit-bot: I haz the power, 2016-11-08 21:57:24]

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

[tbansal1, 2016-11-08 22:09:28]

Patchset #4 (id:60001) has been deleted

[tbansal1, 2016-11-08 22:09:42]

Patchset #2 (id:20001) has been deleted

[tbansal1, 2016-11-08 22:09:50]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2016-11-08 23:35:34]

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

[commit-bot: I haz the power, 2016-11-08 23:35:35]

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-11-09 01:45:28]

Description was changed from

==========
Add default estimates

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

BUG=638304
==========

to

==========
NQE: Add default estimates

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

BUG=638304
==========

[tbansal1, 2016-11-09 01:45:40]

Patchset #1 (id:40001) has been deleted

[tbansal1, 2016-11-09 01:47:31]

Patchset #2 (id:100001) has been deleted

[tbansal1, 2016-11-09 01:49:43]

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

[commit-bot: I haz the power, 2016-11-09 01:50:06]

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-11-09 03:44:04]

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

[commit-bot: I haz the power, 2016-11-09 03:44:05]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2016-11-09 22:42:39]

Description was changed from

==========
NQE: Add default estimates

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

BUG=638304
==========

to

==========
NQE: Add default estimates

Observer is notified of current estimates as soon as it is added

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

BUG=638304
==========

[tbansal1, 2016-11-21 07:54:35]

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

[commit-bot: I haz the power, 2016-11-21 07:54:49]

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-11-21 07:57:18]

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

[commit-bot: I haz the power, 2016-11-21 07:57:19]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tbansal1, 2016-11-21 18:30:22]

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

[commit-bot: I haz the power, 2016-11-21 18:31:10]

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-11-21 19:36:25]

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

[commit-bot: I haz the power, 2016-11-21 19:36:26]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2016-11-22 03:23:04]

Patchset #1 (id:80001) has been deleted

[tbansal1, 2016-11-22 03:23:11]

Patchset #1 (id:120001) has been deleted

[tbansal1, 2016-11-22 03:23:19]

Patchset #1 (id:140001) has been deleted

[tbansal1, 2016-11-22 22:00:10]

Patchset #1 (id:160001) has been deleted

[tbansal1, 2016-11-22 22:00:15]

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

[commit-bot: I haz the power, 2016-11-22 22:00:55]

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

[tbansal1, 2016-11-22 22:02:03]

Description was changed from

==========
NQE: Add default estimates

Observer is notified of current estimates as soon as it is added

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

BUG=638304
==========

to

==========
NQE: Add default RTT and throughput observations

Add default estimates for different connection types in the network
quality estimator (NQE). Putting the estimates in the src code itself 
makes them accessible for Cronet embedders, and simplies the field 
trial configs.

Additionally, RTT and throughput observers are notified of the default 
observations as soon as they are added

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

BUG=638304
==========

[tbansal1, 2016-11-22 22:02:46]

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

[tbansal1, 2016-11-22 22:02:47]

ryansturm: ptal when you are back. Thanks.

[commit-bot: I haz the power, 2016-11-23 00:00:06]

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

[commit-bot: I haz the power, 2016-11-23 00:00:07]

Dry run: This issue passed the CQ dry run.

[RyanSturm, 2016-11-28 21:10:03]

https://codereview.chromium.org/2481373004/diff/180001/net/nqe/network_qualit...
File net/nqe/network_quality_estimator.cc (right):

https://codereview.chromium.org/2481373004/diff/180001/net/nqe/network_qualit...
net/nqe/network_quality_estimator.cc:234: bool use_default_platform_values)
nit: maybe
s/use_default_platform_observations/add_default_platform_observations/

https://codereview.chromium.org/2481373004/diff/180001/net/nqe/network_qualit...
File net/nqe/network_quality_estimator.h (right):

https://codereview.chromium.org/2481373004/diff/180001/net/nqe/network_qualit...
net/nqe/network_quality_estimator.h:301: // A protected constructor for testing
that allows settting the value of
nit: s/settting/setting

https://codereview.chromium.org/2481373004/diff/180001/net/nqe/network_qualit...
File net/nqe/network_quality_estimator_unittest.cc (right):

https://codereview.chromium.org/2481373004/diff/180001/net/nqe/network_qualit...
net/nqe/network_quality_estimator_unittest.cc:536:
TEST(NetworkQualityEstimatorTest, DefaultObservations) {
nit: Can you switch the order of these tests? It makes more sense to have
DefaultObservations before DefaultObservationsOverridden IMO.

[tbansal1, 2016-11-28 22:50:46]

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

[tbansal1, 2016-11-28 22:50:57]

ryansturm: ptal. thanks.

https://codereview.chromium.org/2481373004/diff/180001/net/nqe/network_qualit...
File net/nqe/network_quality_estimator.cc (right):

https://codereview.chromium.org/2481373004/diff/180001/net/nqe/network_qualit...
net/nqe/network_quality_estimator.cc:234: bool use_default_platform_values)
On 2016/11/28 21:10:03, Ryan Sturm wrote:
> nit: maybe
> s/use_default_platform_observations/add_default_platform_observations/
> 

Done.

https://codereview.chromium.org/2481373004/diff/180001/net/nqe/network_qualit...
File net/nqe/network_quality_estimator.h (right):

https://codereview.chromium.org/2481373004/diff/180001/net/nqe/network_qualit...
net/nqe/network_quality_estimator.h:301: // A protected constructor for testing
that allows settting the value of
On 2016/11/28 21:10:03, Ryan Sturm wrote:
> nit: s/settting/setting

Done.

https://codereview.chromium.org/2481373004/diff/180001/net/nqe/network_qualit...
File net/nqe/network_quality_estimator_unittest.cc (right):

https://codereview.chromium.org/2481373004/diff/180001/net/nqe/network_qualit...
net/nqe/network_quality_estimator_unittest.cc:536:
TEST(NetworkQualityEstimatorTest, DefaultObservations) {
On 2016/11/28 21:10:03, Ryan Sturm wrote:
> nit: Can you switch the order of these tests? It makes more sense to have
> DefaultObservations before DefaultObservationsOverridden IMO.

Done.

[commit-bot: I haz the power, 2016-11-28 22:51:44]

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

[RyanSturm, 2016-11-28 22:52:59]

lgtm

[tbansal1, 2016-11-28 22:53:25]

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

[tbansal1, 2016-11-28 22:53:26]

bengr: ptal. thanks.

[commit-bot: I haz the power, 2016-11-29 01:04:36]

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

[commit-bot: I haz the power, 2016-11-29 01:04:38]

Dry run: This issue passed the CQ dry run.

[bengr, 2016-11-29 18:39:15]

https://codereview.chromium.org/2481373004/diff/200001/net/nqe/network_qualit...
File net/nqe/network_quality_estimator.h (right):

https://codereview.chromium.org/2481373004/diff/200001/net/nqe/network_qualit...
net/nqe/network_quality_estimator.h:635:
default_observations_[NetworkChangeNotifier::CONNECTION_LAST + 1];
Why is this +1 now?

https://codereview.chromium.org/2481373004/diff/200001/net/nqe/network_qualit...
File net/nqe/network_quality_estimator_params.cc (right):

https://codereview.chromium.org/2481373004/diff/200001/net/nqe/network_qualit...
net/nqe/network_quality_estimator_params.cc:136: // First set the default
thresholds.
Add to this comment to be a little clearer:

// Default thresholds for HTTP RTT, transport RTT, and downstream throughput
Kbps for the various effective connection types. These may be overridden by
variations p[arams.

Also, I have no idea what threshold means in this context. Is it the starting
observation, the lowest values, or what?

https://codereview.chromium.org/2481373004/diff/200001/net/nqe/network_qualit...
File net/nqe/network_quality_estimator_unittest.cc (right):

https://codereview.chromium.org/2481373004/diff/200001/net/nqe/network_qualit...
net/nqe/network_quality_estimator_unittest.cc:454: // Verify that the observers
receive the notifications when default estimates
Verify -> Verifies

https://codereview.chromium.org/2481373004/diff/200001/net/nqe/network_qualit...
net/nqe/network_quality_estimator_unittest.cc:491: // Taken from
network_quality_estimator_params.cc.
I would say only once per test that the constant values are taken from that
file.

[tbansal1, 2016-12-02 03:09:27]

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

[commit-bot: I haz the power, 2016-12-02 03:09:59]

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-02 04:25:17]

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

[commit-bot: I haz the power, 2016-12-02 04:25:17]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2016-12-02 17:41:18]

bengr: ptal. Thanks.

https://codereview.chromium.org/2481373004/diff/200001/net/nqe/network_qualit...
File net/nqe/network_quality_estimator.h (right):

https://codereview.chromium.org/2481373004/diff/200001/net/nqe/network_qualit...
net/nqe/network_quality_estimator.h:635:
default_observations_[NetworkChangeNotifier::CONNECTION_LAST + 1];
On 2016/11/29 18:39:15, bengr wrote:
> Why is this +1 now?

CONNECTION_LAST + 1 is correct. The size of the array should be 1 more than the
last connection type.

https://codereview.chromium.org/2481373004/diff/200001/net/nqe/network_qualit...
File net/nqe/network_quality_estimator_params.cc (right):

https://codereview.chromium.org/2481373004/diff/200001/net/nqe/network_qualit...
net/nqe/network_quality_estimator_params.cc:136: // First set the default
thresholds.
On 2016/11/29 18:39:15, bengr wrote:
> Add to this comment to be a little clearer:
> 
> // Default thresholds for HTTP RTT, transport RTT, and downstream throughput
> Kbps for the various effective connection types. These may be overridden by
> variations p[arams.
> 
> Also, I have no idea what threshold means in this context. Is it the starting
> observation, the lowest values, or what?

Done.

https://codereview.chromium.org/2481373004/diff/200001/net/nqe/network_qualit...
File net/nqe/network_quality_estimator_unittest.cc (right):

https://codereview.chromium.org/2481373004/diff/200001/net/nqe/network_qualit...
net/nqe/network_quality_estimator_unittest.cc:454: // Verify that the observers
receive the notifications when default estimates
On 2016/11/29 18:39:15, bengr wrote:
> Verify -> Verifies

Done.

https://codereview.chromium.org/2481373004/diff/200001/net/nqe/network_qualit...
net/nqe/network_quality_estimator_unittest.cc:491: // Taken from
network_quality_estimator_params.cc.
On 2016/11/29 18:39:15, bengr wrote:
> I would say only once per test that the constant values are taken from that
> file.

Done.

[tbansal1, 2016-12-06 22:44:08]

bengr: ping. Thanks.

[bengr, 2016-12-08 21:54:39]

lgtm

[tbansal1, 2016-12-08 21:56:41]

The CQ bit was checked by tbansal@chromium.org

[tbansal1, 2016-12-08 21:56:42]

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/2481373004/#ps220001
(title: "Rebased, addressed bengr comments")

[commit-bot: I haz the power, 2016-12-08 21:57:09]

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-09 01:00:19]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1481234201923410,
"parent_rev": "8bd48a37447bb9420d830cfdfc2f578fea2fd1ab", "commit_rev":
"65b42f52c2a98698887750456cd471942f7c3e32"}

[commit-bot: I haz the power, 2016-12-09 01:00:57]

Description was changed from

==========
NQE: Add default RTT and throughput observations

Add default estimates for different connection types in the network
quality estimator (NQE). Putting the estimates in the src code itself 
makes them accessible for Cronet embedders, and simplies the field 
trial configs.

Additionally, RTT and throughput observers are notified of the default 
observations as soon as they are added

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

BUG=638304
==========

to

==========
NQE: Add default RTT and throughput observations

Add default estimates for different connection types in the network
quality estimator (NQE). Putting the estimates in the src code itself 
makes them accessible for Cronet embedders, and simplies the field 
trial configs.

Additionally, RTT and throughput observers are notified of the default 
observations as soon as they are added

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

BUG=638304
==========

[commit-bot: I haz the power, 2016-12-09 01:01:00]

Committed patchset #3 (id:220001)

[commit-bot: I haz the power, 2016-12-09 01:03:39]

Description was changed from

==========
NQE: Add default RTT and throughput observations

Add default estimates for different connection types in the network
quality estimator (NQE). Putting the estimates in the src code itself 
makes them accessible for Cronet embedders, and simplies the field 
trial configs.

Additionally, RTT and throughput observers are notified of the default 
observations as soon as they are added

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

BUG=638304
==========

to

==========
NQE: Add default RTT and throughput observations

Add default estimates for different connection types in the network
quality estimator (NQE). Putting the estimates in the src code itself
makes them accessible for Cronet embedders, and simplies the field
trial configs.

Additionally, RTT and throughput observers are notified of the default
observations as soon as they are added

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

BUG=638304

Committed: https://crrev.com/e71312541dfbef6a53b0dd81923e9db1816dfc36
Cr-Commit-Position: refs/heads/master@{#437396}
==========

[commit-bot: I haz the power, 2016-12-09 01:03:40]

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