Add a network quality cache observer

Add a network quality cache observer

The observer is notified every time there is a change in the
cached network quality. In the later CLs, one such observer
would be created per-profile. On receiving the notification,
it would write the network id and the cached network quality
(both are included in the notification) to the prefs.

BUG=490870
Committed: https://crrev.com/24d7cace893a94082f913ddf0a1dbc6f725dda31
Cr-Commit-Position: refs/heads/master@{#414815}

[tbansal1, 2016-08-19 21:42:10]

Description was changed from

==========
Add a network quality cache observer

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

to

==========
Add a network quality cache observer

The observer is notified every time there is a change in the
cached network quality. In the later CLs, one such observer
would be created per-profile. On receiving the notification,
it would write the network id and the cached network quality 
(both are included in the notification) to the prefs.

BUG=490870
==========

[tbansal1, 2016-08-19 21:43:52]

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

[tbansal1, 2016-08-19 21:44:03]

Patchset #1 (id:1) has been deleted

[tbansal1, 2016-08-19 21:44:25]

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

[tbansal1, 2016-08-19 21:44:26]

ryansturm: ptal. Thanks.

[commit-bot: I haz the power, 2016-08-19 21:44:33]

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

[RyanSturm, 2016-08-19 22:41:02]

lgtm % nit

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

https://codereview.chromium.org/2261813002/diff/20001/net/nqe/network_quality...
net/nqe/network_quality_store.h:85:
network_qualities_persistent_caching_observer_list_;
nit: Can this match the type name (ie.
s/network_qualities_persistent_caching_observer_list_/network_qualities_cache_observer_list_)

[tbansal1, 2016-08-20 00:50:57]

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

[tbansal1, 2016-08-20 00:50:58]

bengr: ptal. Thanks.

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

https://codereview.chromium.org/2261813002/diff/20001/net/nqe/network_quality...
net/nqe/network_quality_store.h:85:
network_qualities_persistent_caching_observer_list_;
On 2016/08/19 22:41:02, RyanSturm wrote:
> nit: Can this match the type name (ie.
>
s/network_qualities_persistent_caching_observer_list_/network_qualities_cache_observer_list_)

Done. I forgot to update the variable name after I changed the class name.

[tbansal1, 2016-08-22 15:47:35]

Patchset #3 (id:60001) has been deleted

[bengr, 2016-08-25 18:42:59]

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

https://codereview.chromium.org/2261813002/diff/40001/net/nqe/network_quality...
net/nqe/network_quality_estimator.h:289: // Returns |network_quality_store_|.
Used only for testing.
Technically, this should be "Returns a pointer to" but I don't care if you add
that.

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

https://codereview.chromium.org/2261813002/diff/40001/net/nqe/network_quality...
net/nqe/network_quality_estimator_unittest.cc:2271:
estimator.NetworkQualityStoreForTesting()->AddNetworkQualitiesCacheObserver(
Could you construct the estimator with a store so you wouldn't need this
accessor for testing?

https://codereview.chromium.org/2261813002/diff/40001/net/nqe/network_quality...
net/nqe/network_quality_estimator_unittest.cc:2285: // ("test3g") right before
the connection change event. Second notification
nit: Second -> The second

https://codereview.chromium.org/2261813002/diff/40001/net/nqe/network_quality...
File net/nqe/network_quality_store.h (right):

https://codereview.chromium.org/2261813002/diff/40001/net/nqe/network_quality...
net/nqe/network_quality_store.h:61: // Adds and removes |observer| from the list
of cache observers.
Might be good to say that these are not threadsafe, if true, and that all
observers must live on the IO thread.

[tbansal1, 2016-08-25 20:31:07]

bengr: ptal. thanks.

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

https://codereview.chromium.org/2261813002/diff/40001/net/nqe/network_quality...
net/nqe/network_quality_estimator.h:289: // Returns |network_quality_store_|.
Used only for testing.
On 2016/08/25 18:42:59, bengr wrote:
> Technically, this should be "Returns a pointer to" but I don't care if you add
> that.

Done.

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

https://codereview.chromium.org/2261813002/diff/40001/net/nqe/network_quality...
net/nqe/network_quality_estimator_unittest.cc:2271:
estimator.NetworkQualityStoreForTesting()->AddNetworkQualitiesCacheObserver(
On 2016/08/25 18:42:59, bengr wrote:
> Could you construct the estimator with a store so you wouldn't need this
> accessor for testing?

I could, but then I would need to expose it through IOThread. It is also unclear
who would own it (IOThread or Estimator). IOThread exposing pointers to objects
that it does not own is also somewhat inconsistent.

Further, I would need to duplicate that code in Cronet since Cronet has its own
logic for constructing NQE.

Third, I think Store is an internal detail of NQE, and NQE initialization logic
(in IOThread/Cronet) should not know about this.

https://codereview.chromium.org/2261813002/diff/40001/net/nqe/network_quality...
net/nqe/network_quality_estimator_unittest.cc:2285: // ("test3g") right before
the connection change event. Second notification
On 2016/08/25 18:42:59, bengr wrote:
> nit: Second -> The second

Done.

https://codereview.chromium.org/2261813002/diff/40001/net/nqe/network_quality...
File net/nqe/network_quality_store.h (right):

https://codereview.chromium.org/2261813002/diff/40001/net/nqe/network_quality...
net/nqe/network_quality_store.h:61: // Adds and removes |observer| from the list
of cache observers.
On 2016/08/25 18:42:59, bengr wrote:
> Might be good to say that these are not threadsafe, if true, and that all
> observers must live on the IO thread.

Done.

[bengr, 2016-08-26 17:17:22]

lgtm

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

https://codereview.chromium.org/2261813002/diff/40001/net/nqe/network_quality...
net/nqe/network_quality_estimator_unittest.cc:2271:
estimator.NetworkQualityStoreForTesting()->AddNetworkQualitiesCacheObserver(
On 2016/08/25 20:31:07, tbansal1 wrote:
> On 2016/08/25 18:42:59, bengr wrote:
> > Could you construct the estimator with a store so you wouldn't need this
> > accessor for testing?
> 
> I could, but then I would need to expose it through IOThread. It is also
unclear
> who would own it (IOThread or Estimator). IOThread exposing pointers to
objects
> that it does not own is also somewhat inconsistent.
> 
> Further, I would need to duplicate that code in Cronet since Cronet has its
own
> logic for constructing NQE.
> 
> Third, I think Store is an internal detail of NQE, and NQE initialization
logic
> (in IOThread/Cronet) should not know about this.

Acknowledged.

[tbansal1, 2016-08-26 17:48:36]

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

[commit-bot: I haz the power, 2016-08-26 17:49:37]

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-08-26 20:19:22]

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

[commit-bot: I haz the power, 2016-08-26 20:19: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, 2016-08-26 20:24:45]

The CQ bit was checked by tbansal@chromium.org

[tbansal1, 2016-08-26 20:24:45]

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/2261813002/#ps100001
(title: "Rebased")

[commit-bot: I haz the power, 2016-08-26 20:25:16]

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-08-26 21:37:02]

Description was changed from

==========
Add a network quality cache observer

The observer is notified every time there is a change in the
cached network quality. In the later CLs, one such observer
would be created per-profile. On receiving the notification,
it would write the network id and the cached network quality 
(both are included in the notification) to the prefs.

BUG=490870
==========

to

==========
Add a network quality cache observer

The observer is notified every time there is a change in the
cached network quality. In the later CLs, one such observer
would be created per-profile. On receiving the notification,
it would write the network id and the cached network quality 
(both are included in the notification) to the prefs.

BUG=490870
==========

[commit-bot: I haz the power, 2016-08-26 21:37:06]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2016-08-26 21:41:05]

Description was changed from

==========
Add a network quality cache observer

The observer is notified every time there is a change in the
cached network quality. In the later CLs, one such observer
would be created per-profile. On receiving the notification,
it would write the network id and the cached network quality 
(both are included in the notification) to the prefs.

BUG=490870
==========

to

==========
Add a network quality cache observer

The observer is notified every time there is a change in the
cached network quality. In the later CLs, one such observer
would be created per-profile. On receiving the notification,
it would write the network id and the cached network quality
(both are included in the notification) to the prefs.

BUG=490870

Committed: https://crrev.com/24d7cace893a94082f913ddf0a1dbc6f725dda31
Cr-Commit-Position: refs/heads/master@{#414815}
==========

[commit-bot: I haz the power, 2016-08-26 21:41:06]

Patchset 4 (id:??) landed as
https://crrev.com/24d7cace893a94082f913ddf0a1dbc6f725dda31
Cr-Commit-Position: refs/heads/master@{#414815}