Make NQE a derived class of NetworkQualityProvider

Make NQE  a derived class of NetworkQualityProvider

NetworkQualityProvider provides functions to get network quality 
metrics, and to listen to changes in the network quality.

This CL changes Network Quality Estimator (NQE) to be an implementation
of Network Quality Provider (NQP).

Long term, this allows callers to depend only on NQP when they only 
need to read the network quality, while the heavy-weight NQE can be 
used in other cases.

BUG=704339
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
TBR=petewil@chromium.org

Review-Url: https://codereview.chromium.org/2927453002
Cr-Commit-Position: refs/heads/master@{#477452}
Committed: https://chromium.googlesource.com/chromium/src/+/1bd4a950be946c149226979410c5cc934b1d8b47

[tbansal1, 2017-06-05 21:05:33]

Patchset #1 (id:1) has been deleted

[tbansal1, 2017-06-05 21:05:39]

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

[commit-bot: I haz the power, 2017-06-05 21:06:28]

Dry run: CQ is trying da patch.

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

[tbansal1, 2017-06-05 21:06:54]

Description was changed from

==========
NQE implements NQP

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

to

==========
Make NQE  a derived class of NetworkQualityProvider

NetworkQualityProvider provides functions to get network quality 
metrics, and to listen to changes in the network quality.

This CL changes Network Quality Estimator (NQE) to be an implementation
of Network Quality Provider.

BUG=704339
==========

[tbansal1, 2017-06-05 21:08:50]

Description was changed from

==========
Make NQE  a derived class of NetworkQualityProvider

NetworkQualityProvider provides functions to get network quality 
metrics, and to listen to changes in the network quality.

This CL changes Network Quality Estimator (NQE) to be an implementation
of Network Quality Provider.

BUG=704339
==========

to

==========
Make NQE  a derived class of NetworkQualityProvider

NetworkQualityProvider provides functions to get network quality 
metrics, and to listen to changes in the network quality.

This CL changes Network Quality Estimator (NQE) to be an implementation
of Network Quality Provider (NQP).

Long term, this allows callers to depend only on NQP when they only 
need to read the network quality, while the heavy-weight NQE can be 
used in other cases.

BUG=704339
==========

[tbansal1, 2017-06-05 21:09:02]

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

[tbansal1, 2017-06-05 21:09:15]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-06-05 21:09:25]

Dry run: CQ is trying da patch.

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

[tbansal1, 2017-06-05 21:09:49]

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

[tbansal1, 2017-06-05 21:09:49]

ryansturm: ptal. This is inspired by mmenke's comment here:
https://codereview.chromium.org/2899313006/diff/80001/net/http/http_network_s...

[tbansal1, 2017-06-05 21:17:30]

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

[tbansal1, 2017-06-05 21:17:30]

mmenke: Can you please take a high level look at
net/nqe/network_quality_estimator.h and net/nqe/network_quality_provider.h
to see if it makes sense?

In long term, it would be good to move the NQE consumers to NQP. But, I will
handle that later. Thanks.

[commit-bot: I haz the power, 2017-06-05 21:19:07]

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

[commit-bot: I haz the power, 2017-06-05 21:19:08]

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...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[RyanSturm, 2017-06-05 21:21:03]

I'll need to look through it again in more detail later, but it seems fine.

https://codereview.chromium.org/2927453002/diff/40001/net/nqe/effective_conne...
File net/nqe/effective_connection_type_observer.h (right):

https://codereview.chromium.org/2927453002/diff/40001/net/nqe/effective_conne...
net/nqe/effective_connection_type_observer.h:25: // be notified on the IO
thread.
Remove parts about IO thread? That should be commented in the observer owner not
the observer interface (in case we want to use it on the UI thread.

https://codereview.chromium.org/2927453002/diff/40001/net/nqe/rtt_throughput_...
File net/nqe/rtt_throughput_estimates_observer.h (right):

https://codereview.chromium.org/2927453002/diff/40001/net/nqe/rtt_throughput_...
net/nqe/rtt_throughput_estimates_observer.h:24: // register and unregister
itself on the IO thread. All the observers would
same comment about IO thread.

[mmenke, 2017-06-05 21:50:53]

Awesome, thanks for doing this!  Mind fixing build failures before I look at it?

[tbansal1, 2017-06-05 22:23:18]

On 2017/06/05 21:50:53, mmenke wrote:
> Awesome, thanks for doing this!  Mind fixing build failures before I look at
it?

In progress, it might take some time for me to update all consumers (cronet
etc.),
and run CQ.
However, the core changes of the CL would be in 
net/nqe/network_quality_estimator.h and net/nqe/network_quality_provider.h.
Rest would be all mechanical changes.

My goal was to get a high level feedback on
net/nqe/network_quality_estimator.h and net/nqe/network_quality_provider.h
before I fix all consumers.

[mmenke, 2017-06-05 22:24:04]

On 2017/06/05 22:23:18, tbansal1 wrote:
> On 2017/06/05 21:50:53, mmenke wrote:
> > Awesome, thanks for doing this!  Mind fixing build failures before I look at
> it?
> 
> In progress, it might take some time for me to update all consumers (cronet
> etc.),
> and run CQ.
> However, the core changes of the CL would be in 
> net/nqe/network_quality_estimator.h and net/nqe/network_quality_provider.h.
> Rest would be all mechanical changes.
> 
> My goal was to get a high level feedback on
> net/nqe/network_quality_estimator.h and net/nqe/network_quality_provider.h
> before I fix all consumers.

SGTM, I'll do a high level review tomorrow.

[tbansal1, 2017-06-05 22:27:16]

On 2017/06/05 22:23:18, tbansal1 wrote:
> On 2017/06/05 21:50:53, mmenke wrote:
> > Awesome, thanks for doing this!  Mind fixing build failures before I look at
> it?
> 
> In progress, it might take some time for me to update all consumers (cronet
> etc.),
> and run CQ.
> However, the core changes of the CL would be in 
> net/nqe/network_quality_estimator.h and net/nqe/network_quality_provider.h.
> Rest would be all mechanical changes.
> 
> My goal was to get a high level feedback on
> net/nqe/network_quality_estimator.h and net/nqe/network_quality_provider.h
> before I fix all consumers.

Thanks, I will try to fix CQ in the meantime. It might take me
few runs of CQ to catch all consumers (Webkit, content, cronet, offline pages
etc.,
and their related tests).

[tbansal1, 2017-06-05 22:53:59]

Description was changed from

==========
Make NQE  a derived class of NetworkQualityProvider

NetworkQualityProvider provides functions to get network quality 
metrics, and to listen to changes in the network quality.

This CL changes Network Quality Estimator (NQE) to be an implementation
of Network Quality Provider (NQP).

Long term, this allows callers to depend only on NQP when they only 
need to read the network quality, while the heavy-weight NQE can be 
used in other cases.

BUG=704339
==========

to

==========
Make NQE  a derived class of NetworkQualityProvider

NetworkQualityProvider provides functions to get network quality 
metrics, and to listen to changes in the network quality.

This CL changes Network Quality Estimator (NQE) to be an implementation
of Network Quality Provider (NQP).

Long term, this allows callers to depend only on NQP when they only 
need to read the network quality, while the heavy-weight NQE can be 
used in other cases.

BUG=704339
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[tbansal1, 2017-06-05 22:59:22]

Patchset #2 (id:60001) has been deleted

[tbansal1, 2017-06-05 22:59:28]

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

[commit-bot: I haz the power, 2017-06-05 23:00:02]

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-06-05 23:20:09]

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

[commit-bot: I haz the power, 2017-06-05 23:20:10]

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...)

[tbansal1, 2017-06-05 23:22:55]

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

[commit-bot: I haz the power, 2017-06-05 23:23:35]

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-06-05 23:43:04]

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

[commit-bot: I haz the power, 2017-06-05 23:43:05]

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...)

[tbansal1, 2017-06-05 23:47:08]

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

[tbansal1, 2017-06-05 23:47:16]

Patchset #3 (id:100001) has been deleted

[commit-bot: I haz the power, 2017-06-05 23:47: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, 2017-06-06 00:45:22]

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

[commit-bot: I haz the power, 2017-06-06 00:45:23]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[tbansal1, 2017-06-06 05:07:15]

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

[tbansal1, 2017-06-06 05:07:19]

Patchset #3 (id:120001) has been deleted

[commit-bot: I haz the power, 2017-06-06 05:07:30]

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-06-06 05:16:26]

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

[commit-bot: I haz the power, 2017-06-06 05:16:27]

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

[tbansal1, 2017-06-06 05:40:21]

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

[tbansal1, 2017-06-06 05:40:25]

Patchset #4 (id:160001) has been deleted

[commit-bot: I haz the power, 2017-06-06 05:40:33]

Dry run: CQ is trying da patch.

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

[tbansal1, 2017-06-06 05:40:34]

Patchset #3 (id:140001) has been deleted

[tbansal1, 2017-06-06 05:40:54]

Patchset #2 (id:80001) has been deleted

[commit-bot: I haz the power, 2017-06-06 05:56:23]

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

[commit-bot: I haz the power, 2017-06-06 05:56:24]

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, 2017-06-06 06:08:36]

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

[tbansal1, 2017-06-06 06:08:44]

Patchset #2 (id:180001) has been deleted

[commit-bot: I haz the power, 2017-06-06 06:08:56]

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-06-06 07:42:02]

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

[commit-bot: I haz the power, 2017-06-06 07:42:03]

Dry run: This issue passed the CQ dry run.

[mmenke, 2017-06-06 15:57:00]

A couple missing headers, but otherwise, LGTM

https://codereview.chromium.org/2927453002/diff/50015/net/nqe/effective_conne...
File net/nqe/effective_connection_type_observer.h (right):

https://codereview.chromium.org/2927453002/diff/50015/net/nqe/effective_conne...
net/nqe/effective_connection_type_observer.h:16: class NET_EXPORT
EffectiveConnectionTypeObserver {
include net/base/net_export.h

https://codereview.chromium.org/2927453002/diff/50015/net/nqe/rtt_throughput_...
File net/nqe/rtt_throughput_estimates_observer.h (right):

https://codereview.chromium.org/2927453002/diff/50015/net/nqe/rtt_throughput_...
net/nqe/rtt_throughput_estimates_observer.h:11: #include
"base/gtest_prod_util.h"
Not used?

https://codereview.chromium.org/2927453002/diff/50015/net/nqe/rtt_throughput_...
net/nqe/rtt_throughput_estimates_observer.h:17: class NET_EXPORT
RTTAndThroughputEstimatesObserver {
include net/base/net_export.h

https://codereview.chromium.org/2927453002/diff/50015/net/nqe/rtt_throughput_...
net/nqe/rtt_throughput_estimates_observer.h:33: base::TimeDelta transport_rtt,
should include base/time.h

[RyanSturm, 2017-06-06 15:58:20]

Lgtm % mmenke@'s comments

[tbansal1, 2017-06-06 16:55:51]

https://codereview.chromium.org/2927453002/diff/40001/net/nqe/effective_conne...
File net/nqe/effective_connection_type_observer.h (right):

https://codereview.chromium.org/2927453002/diff/40001/net/nqe/effective_conne...
net/nqe/effective_connection_type_observer.h:25: // be notified on the IO
thread.
On 2017/06/05 21:21:02, RyanSturm wrote:
> Remove parts about IO thread? That should be commented in the observer owner
not
> the observer interface (in case we want to use it on the UI thread.

Done.

https://codereview.chromium.org/2927453002/diff/40001/net/nqe/rtt_throughput_...
File net/nqe/rtt_throughput_estimates_observer.h (right):

https://codereview.chromium.org/2927453002/diff/40001/net/nqe/rtt_throughput_...
net/nqe/rtt_throughput_estimates_observer.h:24: // register and unregister
itself on the IO thread. All the observers would
On 2017/06/05 21:21:02, RyanSturm wrote:
> same comment about IO thread.

Done.

https://codereview.chromium.org/2927453002/diff/50015/net/nqe/effective_conne...
File net/nqe/effective_connection_type_observer.h (right):

https://codereview.chromium.org/2927453002/diff/50015/net/nqe/effective_conne...
net/nqe/effective_connection_type_observer.h:16: class NET_EXPORT
EffectiveConnectionTypeObserver {
On 2017/06/06 15:56:58, mmenke wrote:
> include net/base/net_export.h

Done.

https://codereview.chromium.org/2927453002/diff/50015/net/nqe/rtt_throughput_...
File net/nqe/rtt_throughput_estimates_observer.h (right):

https://codereview.chromium.org/2927453002/diff/50015/net/nqe/rtt_throughput_...
net/nqe/rtt_throughput_estimates_observer.h:11: #include
"base/gtest_prod_util.h"
On 2017/06/06 15:56:58, mmenke wrote:
> Not used?

Done.

https://codereview.chromium.org/2927453002/diff/50015/net/nqe/rtt_throughput_...
net/nqe/rtt_throughput_estimates_observer.h:17: class NET_EXPORT
RTTAndThroughputEstimatesObserver {
On 2017/06/06 15:56:58, mmenke wrote:
> include net/base/net_export.h

Done.

https://codereview.chromium.org/2927453002/diff/50015/net/nqe/rtt_throughput_...
net/nqe/rtt_throughput_estimates_observer.h:33: base::TimeDelta transport_rtt,
On 2017/06/06 15:56:58, mmenke wrote:
> should include base/time.h

Done.

[tbansal1, 2017-06-06 17:02:32]

Patchset #4 (id:230001) has been deleted

[tbansal1, 2017-06-06 17:05:09]

Patchset #4 (id:230002) has been deleted

[tbansal1, 2017-06-06 17:06:15]

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

[commit-bot: I haz the power, 2017-06-06 17:06:34]

Dry run: CQ is trying da patch.

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

[tbansal1, 2017-06-06 17:13:18]

tbansal@chromium.org changed reviewers:
+ creis@chromium.org, petewil@chromium.org, rkaplow@chromium.org

[tbansal1, 2017-06-06 17:13:19]

petewil:
components/offline_pages/core/background/network_quality_provider_stub.*

rkaplow: components/metrics/net/network_metrics_provider.cc

creis: content/*

[commit-bot: I haz the power, 2017-06-06 18:40:16]

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

[commit-bot: I haz the power, 2017-06-06 18:40:17]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2017-06-06 20:16:26]

content/ LGTM

[rkaplow, 2017-06-06 20:57:55]

lgtm

[tbansal1, 2017-06-06 22:55:56]

Description was changed from

==========
Make NQE  a derived class of NetworkQualityProvider

NetworkQualityProvider provides functions to get network quality 
metrics, and to listen to changes in the network quality.

This CL changes Network Quality Estimator (NQE) to be an implementation
of Network Quality Provider (NQP).

Long term, this allows callers to depend only on NQP when they only 
need to read the network quality, while the heavy-weight NQE can be 
used in other cases.

BUG=704339
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
Make NQE  a derived class of NetworkQualityProvider

NetworkQualityProvider provides functions to get network quality 
metrics, and to listen to changes in the network quality.

This CL changes Network Quality Estimator (NQE) to be an implementation
of Network Quality Provider (NQP).

Long term, this allows callers to depend only on NQP when they only 
need to read the network quality, while the heavy-weight NQE can be 
used in other cases.

BUG=704339
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
TBR=petewil@chromium.org
==========

[tbansal1, 2017-06-06 22:56:02]

The CQ bit was checked by tbansal@chromium.org

[tbansal1, 2017-06-06 22:56:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
ryansturm@chromium.org, mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/2927453002/#ps260001
(title: "some more IWYU fixes")

[commit-bot: I haz the power, 2017-06-06 22:56: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, 2017-06-06 23:01:49]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1496789762365010,
"parent_rev": "7cdb0ae344a79121cbb440651966bf81102d258b", "commit_rev":
"1bd4a950be946c149226979410c5cc934b1d8b47"}

[commit-bot: I haz the power, 2017-06-06 23:02:01]

Description was changed from

==========
Make NQE  a derived class of NetworkQualityProvider

NetworkQualityProvider provides functions to get network quality 
metrics, and to listen to changes in the network quality.

This CL changes Network Quality Estimator (NQE) to be an implementation
of Network Quality Provider (NQP).

Long term, this allows callers to depend only on NQP when they only 
need to read the network quality, while the heavy-weight NQE can be 
used in other cases.

BUG=704339
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
TBR=petewil@chromium.org
==========

to

==========
Make NQE  a derived class of NetworkQualityProvider

NetworkQualityProvider provides functions to get network quality 
metrics, and to listen to changes in the network quality.

This CL changes Network Quality Estimator (NQE) to be an implementation
of Network Quality Provider (NQP).

Long term, this allows callers to depend only on NQP when they only 
need to read the network quality, while the heavy-weight NQE can be 
used in other cases.

BUG=704339
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
TBR=petewil@chromium.org

Review-Url: https://codereview.chromium.org/2927453002
Cr-Commit-Position: refs/heads/master@{#477452}
Committed:
https://chromium.googlesource.com/chromium/src/+/1bd4a950be946c149226979410c5...
==========

[commit-bot: I haz the power, 2017-06-06 23:02:03]

Committed patchset #4 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/1bd4a950be946c149226979410c5...