Factor out NetworkID and caching mechanism from n_q_e.{h,cc}

Factor out NetworkID and caching mechanism from n_q_e.{h,cc}

This keeps the caching mechanism separate from NQE. This
is first step towards enabling persistent caching of
estimates.

This CL does not record any functional changes, although
tests have been expanded.

Also, add UMA to record how frequently cached network
quality is available.

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

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

Patchset #2 (id:20001) has been deleted

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

Patchset #1 (id:1) has been deleted

[tbansal1, 2016-07-07 22:36:33]

Patchset #1 (id:40001) has been deleted

[tbansal1, 2016-07-07 22:56:14]

Description was changed from

==========
w

w

w

w

w

w

w

w

w

w

w

w

w

w

w

w

w

w

w

w

w

w

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

to

==========
Factor out NetworkID and caching mechanism from n_q_e.{h,cc}

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

[tbansal1, 2016-07-08 00:53:35]

Patchset #1 (id:60001) has been deleted

[tbansal1, 2016-07-08 00:56:27]

Description was changed from

==========
Factor out NetworkID and caching mechanism from n_q_e.{h,cc}

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

to

==========
Factor out NetworkID and caching mechanism from n_q_e.{h,cc}

This keeps the caching mechanism separate from NQE. This
is first step towards enabling persistent caching of 
estimates.

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

[tbansal1, 2016-07-08 00:57:45]

Description was changed from

==========
Factor out NetworkID and caching mechanism from n_q_e.{h,cc}

This keeps the caching mechanism separate from NQE. This
is first step towards enabling persistent caching of 
estimates.

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

to

==========
Factor out NetworkID and caching mechanism from n_q_e.{h,cc}

This keeps the caching mechanism separate from NQE. This
is first step towards enabling persistent caching of 
estimates.

This CL does not record any functional changes, although
tests are improved now.

Also, add UMA to record how frequently cached network 
quality is available.

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

[tbansal1, 2016-07-08 20:29:59]

Patchset #1 (id:80001) has been deleted

[tbansal1, 2016-07-08 20:30:19]

Description was changed from

==========
Factor out NetworkID and caching mechanism from n_q_e.{h,cc}

This keeps the caching mechanism separate from NQE. This
is first step towards enabling persistent caching of 
estimates.

This CL does not record any functional changes, although
tests are improved now.

Also, add UMA to record how frequently cached network 
quality is available.

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

to

==========
Factor out NetworkID and caching mechanism from n_q_e.{h,cc}

This keeps the caching mechanism separate from NQE. This
is first step towards enabling persistent caching of 
estimates.

This CL does not record any functional changes, although
tests are improved now.

Also, add UMA to record how frequently cached network 
quality is available.

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

[tbansal1, 2016-07-08 21:00:46]

Description was changed from

==========
Factor out NetworkID and caching mechanism from n_q_e.{h,cc}

This keeps the caching mechanism separate from NQE. This
is first step towards enabling persistent caching of 
estimates.

This CL does not record any functional changes, although
tests are improved now.

Also, add UMA to record how frequently cached network 
quality is available.

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

to

==========
Factor out NetworkID and caching mechanism from n_q_e.{h,cc}

This keeps the caching mechanism separate from NQE. This
is first step towards enabling persistent caching of 
estimates.

This CL does not record any functional changes, although
tests have been expanded.

Also, add UMA to record how frequently cached network 
quality is available.

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

[tbansal1, 2016-07-08 21:01:17]

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

[tbansal1, 2016-07-08 21:01:18]

bengr: ptal. Thanks.

[RyanSturm, 2016-07-12 18:45:37]

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

[RyanSturm, 2016-07-12 18:45:39]

https://codereview.chromium.org/2128793003/diff/120001/net/nqe/cached_network...
File net/nqe/cached_network_quality.h (right):

https://codereview.chromium.org/2128793003/diff/120001/net/nqe/cached_network...
net/nqe/cached_network_quality.h:20: // Cached entries are stored in LRU order,
and older entries may be evicted.
This seems like a detail of the manager, not the entries in the cache.

https://codereview.chromium.org/2128793003/diff/120001/net/nqe/network_id.h
File net/nqe/network_id.h (right):

https://codereview.chromium.org/2128793003/diff/120001/net/nqe/network_id.h#n...
net/nqe/network_id.h:8: #include <stdint.h>
Why do you need this?

https://codereview.chromium.org/2128793003/diff/120001/net/nqe/network_id.h#n...
net/nqe/network_id.h:13: #include "base/macros.h"
Why do you need this?

https://codereview.chromium.org/2128793003/diff/120001/net/nqe/network_qualit...
File net/nqe/network_qualities_manager.h (right):

https://codereview.chromium.org/2128793003/diff/120001/net/nqe/network_qualit...
net/nqe/network_qualities_manager.h:23: // in a cache which is backed up by
memory.
Can you mention that it is LRU in this comment?

https://codereview.chromium.org/2128793003/diff/120001/net/nqe/network_qualit...
File net/nqe/network_qualities_manager_unittest.cc (right):

https://codereview.chromium.org/2128793003/diff/120001/net/nqe/network_qualit...
net/nqe/network_qualities_manager_unittest.cc:7: #include "base/macros.h"
Is this needed?

https://codereview.chromium.org/2128793003/diff/120001/net/nqe/network_qualit...
net/nqe/network_qualities_manager_unittest.cc:110: const
nqe::internal::CachedNetworkQuality cached_network_quality(
Add a comment that this cached_network_quality is defined in the same way above
when it is added to the map, or better yet, share the cached_network_quality
with the code above.

https://codereview.chromium.org/2128793003/diff/120001/net/nqe/network_qualit...
net/nqe/network_qualities_manager_unittest.cc:160: const size_t network_count =
100;
I'd prefer kMaximumNetworkQualityCacheSize+1, if you are willing to expose that
outside of the NetworkQualitiesManager class. 100 is overkill, and staying in
sync with the limit would be better. If not,  you have a static_assert to keep
the limit under 10, so maybe should make the unittest use 11 as a hard-coded
value.

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

https://codereview.chromium.org/2128793003/diff/120001/net/nqe/network_qualit...
net/nqe/network_quality_estimator.cc:1218:
UMA_HISTOGRAM_BOOLEAN("NQE.CachedNetworkQualityAvailable", false);
Can you combine the two NQE.CacheNetworkQualityAvailable histogram lines?

https://codereview.chromium.org/2128793003/diff/120001/net/nqe/network_qualit...
File net/nqe/network_quality_observation_source.h (right):

https://codereview.chromium.org/2128793003/diff/120001/net/nqe/network_qualit...
net/nqe/network_quality_observation_source.h:37:
NETWORK_QUALITY_OBSERVATION_SOURCE_MAX,
This seems unrelated. Not worth its own CL, but it's confusing that it is part
of this CL.

[tbansal1, 2016-07-12 19:40:19]

ryansturm: PTAL. Thanks.

https://codereview.chromium.org/2128793003/diff/120001/net/nqe/cached_network...
File net/nqe/cached_network_quality.h (right):

https://codereview.chromium.org/2128793003/diff/120001/net/nqe/cached_network...
net/nqe/cached_network_quality.h:20: // Cached entries are stored in LRU order,
and older entries may be evicted.
On 2016/07/12 18:45:38, RyanSturm wrote:
> This seems like a detail of the manager, not the entries in the cache.

Moved to NetworkQualitiesManager file.

https://codereview.chromium.org/2128793003/diff/120001/net/nqe/network_id.h
File net/nqe/network_id.h (right):

https://codereview.chromium.org/2128793003/diff/120001/net/nqe/network_id.h#n...
net/nqe/network_id.h:8: #include <stdint.h>
On 2016/07/12 18:45:38, RyanSturm wrote:
> Why do you need this?

Done.

https://codereview.chromium.org/2128793003/diff/120001/net/nqe/network_id.h#n...
net/nqe/network_id.h:13: #include "base/macros.h"
On 2016/07/12 18:45:38, RyanSturm wrote:
> Why do you need this?

Done.

https://codereview.chromium.org/2128793003/diff/120001/net/nqe/network_qualit...
File net/nqe/network_qualities_manager.h (right):

https://codereview.chromium.org/2128793003/diff/120001/net/nqe/network_qualit...
net/nqe/network_qualities_manager.h:23: // in a cache which is backed up by
memory.
On 2016/07/12 18:45:38, RyanSturm wrote:
> Can you mention that it is LRU in this comment?

Done.

https://codereview.chromium.org/2128793003/diff/120001/net/nqe/network_qualit...
File net/nqe/network_qualities_manager_unittest.cc (right):

https://codereview.chromium.org/2128793003/diff/120001/net/nqe/network_qualit...
net/nqe/network_qualities_manager_unittest.cc:7: #include "base/macros.h"
On 2016/07/12 18:45:38, RyanSturm wrote:
> Is this needed?

Done.

https://codereview.chromium.org/2128793003/diff/120001/net/nqe/network_qualit...
net/nqe/network_qualities_manager_unittest.cc:110: const
nqe::internal::CachedNetworkQuality cached_network_quality(
On 2016/07/12 18:45:38, RyanSturm wrote:
> Add a comment that this cached_network_quality is defined in the same way
above
> when it is added to the map, or better yet, share the cached_network_quality
> with the code above.

Done.

https://codereview.chromium.org/2128793003/diff/120001/net/nqe/network_qualit...
net/nqe/network_qualities_manager_unittest.cc:160: const size_t network_count =
100;
On 2016/07/12 18:45:38, RyanSturm wrote:
> I'd prefer kMaximumNetworkQualityCacheSize+1, if you are willing to expose
that
> outside of the NetworkQualitiesManager class. 100 is overkill, and staying in
> sync with the limit would be better. If not,  you have a static_assert to keep
> the limit under 10, so maybe should make the unittest use 11 as a hard-coded
> value.

Changed to 11. There is a static assert in network_qualities_manager.cc to check
that kMaximumNetworkQualityCacheSize <= 10.

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

https://codereview.chromium.org/2128793003/diff/120001/net/nqe/network_qualit...
net/nqe/network_quality_estimator.cc:1218:
UMA_HISTOGRAM_BOOLEAN("NQE.CachedNetworkQualityAvailable", false);
On 2016/07/12 18:45:38, RyanSturm wrote:
> Can you combine the two NQE.CacheNetworkQualityAvailable histogram lines?

Done.

https://codereview.chromium.org/2128793003/diff/120001/net/nqe/network_qualit...
File net/nqe/network_quality_observation_source.h (right):

https://codereview.chromium.org/2128793003/diff/120001/net/nqe/network_qualit...
net/nqe/network_quality_observation_source.h:37:
NETWORK_QUALITY_OBSERVATION_SOURCE_MAX,
On 2016/07/12 18:45:38, RyanSturm wrote:
> This seems unrelated. Not worth its own CL, but it's confusing that it is part
> of this CL.

Acknowledged.

[RyanSturm, 2016-07-12 20:00:13]

lgtm

[bengr, 2016-07-19 01:04:36]

https://codereview.chromium.org/2128793003/diff/140001/net/nqe/cached_network...
File net/nqe/cached_network_quality.h (right):

https://codereview.chromium.org/2128793003/diff/140001/net/nqe/cached_network...
net/nqe/cached_network_quality.h:39: base::TimeTicks last_update_time() { return
last_update_time_; }
Can this return a const &?

https://codereview.chromium.org/2128793003/diff/140001/net/nqe/network_id.h
File net/nqe/network_id.h (right):

https://codereview.chromium.org/2128793003/diff/140001/net/nqe/network_id.h#n...
net/nqe/network_id.h:38: // Overloaded because NetworkID is used as key in a
map.
I'd remove the comment because NetworkID should not need to know how it will be
used. You could say something like "Overloaded to support ordered collections."

https://codereview.chromium.org/2128793003/diff/140001/net/nqe/network_qualit...
File net/nqe/network_qualities_manager.cc (right):

https://codereview.chromium.org/2128793003/diff/140001/net/nqe/network_qualit...
net/nqe/network_qualities_manager.cc:15:
NetworkQualitiesManager::NetworkQualitiesManager() {
Why not just use a map? This class seems like overkill.

https://codereview.chromium.org/2128793003/diff/140001/net/nqe/network_qualit...
File net/nqe/network_qualities_manager.h (right):

https://codereview.chromium.org/2128793003/diff/140001/net/nqe/network_qualit...
net/nqe/network_qualities_manager.h:25: class NET_EXPORT_PRIVATE
NetworkQualitiesManager {
This sounds more like a store than a manager. Perhaps it should be called
NetworkQualityStore?

[tbansal1, 2016-07-20 00:08:47]

bengr: ptal. Thanks.

https://codereview.chromium.org/2128793003/diff/140001/net/nqe/cached_network...
File net/nqe/cached_network_quality.h (right):

https://codereview.chromium.org/2128793003/diff/140001/net/nqe/cached_network...
net/nqe/cached_network_quality.h:39: base::TimeTicks last_update_time() { return
last_update_time_; }
On 2016/07/19 01:04:35, bengr wrote:
> Can this return a const &?

Passing by ref is actually slower because it requires an extra step of
dereferencing:
https://cs.chromium.org/chromium/src/base/time/time.h?rcl=0&l=24

https://codereview.chromium.org/2128793003/diff/140001/net/nqe/network_id.h
File net/nqe/network_id.h (right):

https://codereview.chromium.org/2128793003/diff/140001/net/nqe/network_id.h#n...
net/nqe/network_id.h:38: // Overloaded because NetworkID is used as key in a
map.
On 2016/07/19 01:04:35, bengr wrote:
> I'd remove the comment because NetworkID should not need to know how it will
be
> used. You could say something like "Overloaded to support ordered
collections." 

Done.

https://codereview.chromium.org/2128793003/diff/140001/net/nqe/network_qualit...
File net/nqe/network_qualities_manager.cc (right):

https://codereview.chromium.org/2128793003/diff/140001/net/nqe/network_qualit...
net/nqe/network_qualities_manager.cc:15:
NetworkQualitiesManager::NetworkQualitiesManager() {
On 2016/07/19 01:04:35, bengr wrote:
> Why not just use a map? This class seems like overkill.

It will soon become complex enough: It will interact with classes that will be
writing quality to prefs. (see design doc for details).

https://codereview.chromium.org/2128793003/diff/140001/net/nqe/network_qualit...
File net/nqe/network_qualities_manager.h (right):

https://codereview.chromium.org/2128793003/diff/140001/net/nqe/network_qualit...
net/nqe/network_qualities_manager.h:25: class NET_EXPORT_PRIVATE
NetworkQualitiesManager {
On 2016/07/19 01:04:36, bengr wrote:
> This sounds more like a store than a manager. Perhaps it should be called
> NetworkQualityStore?

It will turn to a manager soon. It will interact with classes that will be
writing quality to prefs. (see design doc for details).

[bengr, 2016-07-21 00:16:24]

https://codereview.chromium.org/2128793003/diff/140001/net/nqe/cached_network...
File net/nqe/cached_network_quality.h (right):

https://codereview.chromium.org/2128793003/diff/140001/net/nqe/cached_network...
net/nqe/cached_network_quality.h:39: base::TimeTicks last_update_time() { return
last_update_time_; }
On 2016/07/20 00:08:47, tbansal1 wrote:
> On 2016/07/19 01:04:35, bengr wrote:
> > Can this return a const &?
> 
> Passing by ref is actually slower because it requires an extra step of
> dereferencing:
> https://cs.chromium.org/chromium/src/base/time/time.h?rcl=0&l=24

Interesting. Thanks.

https://codereview.chromium.org/2128793003/diff/140001/net/nqe/network_qualit...
File net/nqe/network_qualities_manager.h (right):

https://codereview.chromium.org/2128793003/diff/140001/net/nqe/network_qualit...
net/nqe/network_qualities_manager.h:25: class NET_EXPORT_PRIVATE
NetworkQualitiesManager {
On 2016/07/20 00:08:47, tbansal1 wrote:
> On 2016/07/19 01:04:36, bengr wrote:
> > This sounds more like a store than a manager. Perhaps it should be called
> > NetworkQualityStore?
> 
> It will turn to a manager soon. It will interact with classes that will be
> writing quality to prefs. (see design doc for details).

Acknowledged.

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

https://codereview.chromium.org/2128793003/diff/180001/net/nqe/network_qualit...
net/nqe/network_qualities_manager.h:25: class NET_EXPORT_PRIVATE
NetworkQualitiesManager {
Hmm. Even if this will become a manager, I wonder if you shouldn't introduce a
store. It simplifies names:

class NetworkQualityStore {
  void Add(...);
  void GetById(...);
...

https://codereview.chromium.org/2128793003/diff/180001/net/nqe/network_qualit...
net/nqe/network_qualities_manager.h:30: // Adds the network quality
|cached_network_quality| of network with id
Adds -> Stores

https://codereview.chromium.org/2128793003/diff/180001/net/nqe/network_qualit...
net/nqe/network_qualities_manager.h:31: // |network_id| to the cache.
to the cache. -> .

https://codereview.chromium.org/2128793003/diff/180001/net/nqe/network_qualit...
net/nqe/network_qualities_manager.h:32: void CacheNetworkQualityEstimate(
How about AddNetworkQualityEstimate or StoreNetworkQualityEstimate?

[tbansal1, 2016-07-21 01:16:05]

Patchset #6 (id:200001) has been deleted

[tbansal1, 2016-07-21 01:25:12]

Patchset #6 (id:220001) has been deleted

[tbansal1, 2016-07-21 01:25:40]

bengr: ptal. Thanks.

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

https://codereview.chromium.org/2128793003/diff/180001/net/nqe/network_qualit...
net/nqe/network_qualities_manager.h:25: class NET_EXPORT_PRIVATE
NetworkQualitiesManager {
On 2016/07/21 00:16:24, bengr wrote:
> Hmm. Even if this will become a manager, I wonder if you shouldn't introduce a
> store. It simplifies names:
> 
> class NetworkQualityStore {
>   void Add(...);
>   void GetById(...);
> ...

Done.

https://codereview.chromium.org/2128793003/diff/180001/net/nqe/network_qualit...
net/nqe/network_qualities_manager.h:30: // Adds the network quality
|cached_network_quality| of network with id
On 2016/07/21 00:16:24, bengr wrote:
> Adds -> Stores

Done.

https://codereview.chromium.org/2128793003/diff/180001/net/nqe/network_qualit...
net/nqe/network_qualities_manager.h:31: // |network_id| to the cache.
On 2016/07/21 00:16:24, bengr wrote:
> to the cache. -> .

Done.

https://codereview.chromium.org/2128793003/diff/180001/net/nqe/network_qualit...
net/nqe/network_qualities_manager.h:32: void CacheNetworkQualityEstimate(
On 2016/07/21 00:16:24, bengr wrote:
> How about AddNetworkQualityEstimate or StoreNetworkQualityEstimate?

Obsolete.

[bengr, 2016-07-21 21:48:15]

lgtm with nits on comments

https://codereview.chromium.org/2128793003/diff/240001/net/nqe/network_qualit...
File net/nqe/network_quality_store.h (right):

https://codereview.chromium.org/2128793003/diff/240001/net/nqe/network_qualit...
net/nqe/network_quality_store.h:29: // Stores the network quality
|cached_network_quality| of network with id
id -> ID

https://codereview.chromium.org/2128793003/diff/240001/net/nqe/network_qualit...
net/nqe/network_quality_store.h:35: // for network with id |network_id|, and
sets |cached_network_quality| to the
for -> for a

https://codereview.chromium.org/2128793003/diff/240001/net/nqe/network_qualit...
net/nqe/network_quality_store.h:42: // Smaller size may reduce the cache hit
rate due to frequent evictions.
Smaller -> A smaller
Larger -> A larger

https://codereview.chromium.org/2128793003/diff/240001/net/nqe/network_qualit...
net/nqe/network_quality_store.h:46: // This does not use a unordered_map or
hash_map for code simplicity (key just
a -> an
key -> the key

https://codereview.chromium.org/2128793003/diff/240001/net/nqe/network_qualit...
net/nqe/network_quality_store.h:53: // Data structure that stores the quality of
networks.
quality -> qualities

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

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

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

asvitkine: PTAL at histograms.xml.

https://codereview.chromium.org/2128793003/diff/240001/net/nqe/network_qualit...
File net/nqe/network_quality_store.h (right):

https://codereview.chromium.org/2128793003/diff/240001/net/nqe/network_qualit...
net/nqe/network_quality_store.h:29: // Stores the network quality
|cached_network_quality| of network with id
On 2016/07/21 21:48:15, bengr wrote:
> id -> ID 

Done.

https://codereview.chromium.org/2128793003/diff/240001/net/nqe/network_qualit...
net/nqe/network_quality_store.h:35: // for network with id |network_id|, and
sets |cached_network_quality| to the
On 2016/07/21 21:48:15, bengr wrote:
> for -> for a

Done.

https://codereview.chromium.org/2128793003/diff/240001/net/nqe/network_qualit...
net/nqe/network_quality_store.h:42: // Smaller size may reduce the cache hit
rate due to frequent evictions.
On 2016/07/21 21:48:15, bengr wrote:
> Smaller -> A smaller
> Larger -> A larger

Done.

https://codereview.chromium.org/2128793003/diff/240001/net/nqe/network_qualit...
net/nqe/network_quality_store.h:46: // This does not use a unordered_map or
hash_map for code simplicity (key just
On 2016/07/21 21:48:15, bengr wrote:
> a -> an
> key -> the key

Done.

https://codereview.chromium.org/2128793003/diff/240001/net/nqe/network_qualit...
net/nqe/network_quality_store.h:53: // Data structure that stores the quality of
networks.
On 2016/07/21 21:48:15, bengr wrote:
> quality -> qualities

Done.

[tbansal1, 2016-07-22 15:38:48]

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

[tbansal1, 2016-07-22 15:38:49]

mmenke: ptal at net.gypi. Thanks.

[mmenke, 2016-07-22 15:44:03]

On 2016/07/22 15:38:49, tbansal1 wrote:
> mmenke: ptal at net.gypi. Thanks.

LGTM, rubberstamp.  If one of you guys wants to update net/owners to give
yourselves per-file ownership of net/net.gyp, net/net.gypi, and net/BUILD.gn,
I'd be happy to sign off on it.

[mmenke, 2016-07-22 15:45:15]

On 2016/07/22 15:44:03, mmenke wrote:
> On 2016/07/22 15:38:49, tbansal1 wrote:
> > mmenke: ptal at net.gypi. Thanks.
> 
> LGTM, rubberstamp.  If one of you guys wants to update net/owners to give
> yourselves per-file ownership of net/net.gyp, net/net.gypi, and net/BUILD.gn,
> I'd be happy to sign off on it.

Or feel free to TBR me (And CC me) for changes to those files, just make sure to
say that you're TBRing me just for those files.

[tbansal1, 2016-07-22 20:25:11]

On 2016/07/22 15:45:15, mmenke wrote:
> On 2016/07/22 15:44:03, mmenke wrote:
> > On 2016/07/22 15:38:49, tbansal1 wrote:
> > > mmenke: ptal at net.gypi. Thanks.
> > 
> > LGTM, rubberstamp.  If one of you guys wants to update net/owners to give
> > yourselves per-file ownership of net/net.gyp, net/net.gypi, and
net/BUILD.gn,
> > I'd be happy to sign off on it.
> 
> Or feel free to TBR me (And CC me) for changes to those files, just make sure
to
> say that you're TBRing me just for those files.

I will add per-file-ownership. That seems preferable because with the TBR
solution,
you will still need to parse the email, and it might create unnecessary email
noise.

[Alexei Svitkine (slow), 2016-07-25 15:33:28]

lgtm

https://codereview.chromium.org/2128793003/diff/260001/net/nqe/network_id.h
File net/nqe/network_id.h (right):

https://codereview.chromium.org/2128793003/diff/260001/net/nqe/network_id.h#n...
net/nqe/network_id.h:15: 
Optional nit: I think it's fine to omit newlines between nested namespace
declarations like these here and when they close.

[tbansal1, 2016-07-25 17:17:43]

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

[commit-bot: I haz the power, 2016-07-25 17:18:07]

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

[tbansal1, 2016-07-25 17:22:37]

https://codereview.chromium.org/2128793003/diff/260001/net/nqe/network_id.h
File net/nqe/network_id.h (right):

https://codereview.chromium.org/2128793003/diff/260001/net/nqe/network_id.h#n...
net/nqe/network_id.h:15: 
On 2016/07/25 15:33:27, Alexei Svitkine (slow) wrote:
> Optional nit: I think it's fine to omit newlines between nested namespace
> declarations like these here and when they close.

Done.

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

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

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

Dry run: This issue passed the CQ dry run.

[tbansal1, 2016-07-25 18:23:00]

The CQ bit was checked by tbansal@chromium.org

[tbansal1, 2016-07-25 18:23:01]

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

[commit-bot: I haz the power, 2016-07-25 18:23:29]

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 18:29:08]

Description was changed from

==========
Factor out NetworkID and caching mechanism from n_q_e.{h,cc}

This keeps the caching mechanism separate from NQE. This
is first step towards enabling persistent caching of 
estimates.

This CL does not record any functional changes, although
tests have been expanded.

Also, add UMA to record how frequently cached network 
quality is available.

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

to

==========
Factor out NetworkID and caching mechanism from n_q_e.{h,cc}

This keeps the caching mechanism separate from NQE. This
is first step towards enabling persistent caching of 
estimates.

This CL does not record any functional changes, although
tests have been expanded.

Also, add UMA to record how frequently cached network 
quality is available.

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

[commit-bot: I haz the power, 2016-07-25 18:29:09]

Committed patchset #9 (id:300001)

[commit-bot: I haz the power, 2016-07-25 18:31:08]

Description was changed from

==========
Factor out NetworkID and caching mechanism from n_q_e.{h,cc}

This keeps the caching mechanism separate from NQE. This
is first step towards enabling persistent caching of 
estimates.

This CL does not record any functional changes, although
tests have been expanded.

Also, add UMA to record how frequently cached network 
quality is available.

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

to

==========
Factor out NetworkID and caching mechanism from n_q_e.{h,cc}

This keeps the caching mechanism separate from NQE. This
is first step towards enabling persistent caching of
estimates.

This CL does not record any functional changes, although
tests have been expanded.

Also, add UMA to record how frequently cached network
quality is available.

BUG=490870

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

[commit-bot: I haz the power, 2016-07-25 18:31:09]

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