Issue 1144163008: Add in-memory caching of network quality estimates across network changes.

tbansal1

tbansal@chromium.org changed reviewers: + bengr@chromium.org, mmenke@chromium.org, pauljensen@chromium.org

5 years, 6 months ago (2015-05-28 00:38:34 UTC) #1

mmenke

Some nits...I'll defer to Paul on this review. https://codereview.chromium.org/1144163008/diff/20001/net/base/network_quality_estimator.cc File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1144163008/diff/20001/net/base/network_quality_estimator.cc#newcode324 net/base/network_quality_estimator.cc:324: DCHECK_EQ(kMaximumNetworkQualityCacheSize, ...

5 years, 6 months ago (2015-05-28 15:25:03 UTC) #3

tbansal1

ptal. thanks. https://codereview.chromium.org/1144163008/diff/20001/net/base/network_quality_estimator.cc File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1144163008/diff/20001/net/base/network_quality_estimator.cc#newcode324 net/base/network_quality_estimator.cc:324: DCHECK_EQ(kMaximumNetworkQualityCacheSize, cached_network_quality_.size()); On 2015/05/28 15:25:02, mmenke wrote: ...

5 years, 6 months ago (2015-05-29 02:27:24 UTC) #4

bengr

https://codereview.chromium.org/1144163008/diff/40001/net/base/network_quality_estimator.cc File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1144163008/diff/40001/net/base/network_quality_estimator.cc#newcode216 net/base/network_quality_estimator.cc:216: NetworkChangeNotifier::ConnectionType::CONNECTION_UNKNOWN || Using a switch might be cleaner. https://codereview.chromium.org/1144163008/diff/40001/net/base/network_quality_estimator.cc#newcode250 ...

5 years, 6 months ago (2015-05-29 17:38:10 UTC) #5

tbansal1

ptal. thanks. https://codereview.chromium.org/1144163008/diff/40001/net/base/network_quality_estimator.cc File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1144163008/diff/40001/net/base/network_quality_estimator.cc#newcode216 net/base/network_quality_estimator.cc:216: NetworkChangeNotifier::ConnectionType::CONNECTION_UNKNOWN || On 2015/05/29 17:38:09, bengr wrote: ...

5 years, 6 months ago (2015-05-29 19:13:37 UTC) #6

bengr

https://codereview.chromium.org/1144163008/diff/40001/net/base/network_quality_estimator_unittest.cc File net/base/network_quality_estimator_unittest.cc (right): https://codereview.chromium.org/1144163008/diff/40001/net/base/network_quality_estimator_unittest.cc#newcode122 net/base/network_quality_estimator_unittest.cc:122: NetworkQualityEstimator::SetCurrentNetworkNameForTests( On 2015/05/29 19:13:37, tbansal1 wrote: > On 2015/05/29 ...

5 years, 6 months ago (2015-06-01 20:39:35 UTC) #7

tbansal1

thanks for the comments. ptal. https://codereview.chromium.org/1144163008/diff/40001/net/base/network_quality_estimator_unittest.cc File net/base/network_quality_estimator_unittest.cc (right): https://codereview.chromium.org/1144163008/diff/40001/net/base/network_quality_estimator_unittest.cc#newcode122 net/base/network_quality_estimator_unittest.cc:122: NetworkQualityEstimator::SetCurrentNetworkNameForTests( On 2015/06/01 20:39:34, ...

5 years, 6 months ago (2015-06-01 21:56:51 UTC) #8

thanks for the comments. ptal.

https://codereview.chromium.org/1144163008/diff/40001/net/base/network_qualit...
File net/base/network_quality_estimator_unittest.cc (right):

https://codereview.chromium.org/1144163008/diff/40001/net/base/network_qualit...
net/base/network_quality_estimator_unittest.cc:122:
NetworkQualityEstimator::SetCurrentNetworkNameForTests(
On 2015/06/01 20:39:34, bengr wrote:
> On 2015/05/29 19:13:37, tbansal1 wrote:
> > On 2015/05/29 17:38:10, bengr wrote:
> > > It would be better, maybe, if you could use a test NCN instead of
> introducing
> > a
> > > new pathway just for tests.
> > 
> > NCN only exposes ConnectionType and not 
> > the SSID name or MCC/MNC code.
> > SSID/MCCMNC names comes from different codebases
> > which IIUC do not have mocking/test classes.
> 
> Uggh. Could you configure NQE to register to listen to a little class you
write
> that poses as an NCN?

Not sure....NCN has nothing to do with SSID name or MCC/MNC names.
To find out the current SSID/MCCMNC name, this code calls a 
synchronous network library API (that in turn calls platform specific code). Not
sure how posing an NCN would help.

Mocking the network library API would definitely help but
I am not sure if that design is better than what I have now.

https://codereview.chromium.org/1144163008/diff/60001/net/base/network_qualit...
File net/base/network_quality_estimator.cc (right):

https://codereview.chromium.org/1144163008/diff/60001/net/base/network_qualit...
net/base/network_quality_estimator.cc:275: base::TimeDelta median_rtt =
base::TimeDelta();
On 2015/06/01 20:39:34, bengr wrote:
> No need to initialize. The default constructor will do that for you.

Done.

https://codereview.chromium.org/1144163008/diff/60001/net/base/network_qualit...
net/base/network_quality_estimator.cc:321: // Networks with empty names should
not be cached since they are harder to
On 2015/06/01 20:39:34, bengr wrote:
> I disagree. I think a cached entry is helpful regardless, even if it has no
> label.

Done.

https://codereview.chromium.org/1144163008/diff/60001/net/base/network_qualit...
net/base/network_quality_estimator.cc:331: const base::TimeDelta& median_rtt) {
On 2015/06/01 20:39:34, bengr wrote:
> Would be good to sanity check the parameters, e.g., with DCHECKs.

Added a TODO in ReadCachedNetworkQualityEstimate().

I did not add DCHECK because some of the estimates can
legitimately be 0.
e.g., if the responses were small, then RTT estimate would be
non-zero but the Kbps estimate would be zero.

https://codereview.chromium.org/1144163008/diff/60001/net/base/network_qualit...
File net/base/network_quality_estimator.h (right):

https://codereview.chromium.org/1144163008/diff/60001/net/base/network_qualit...
net/base/network_quality_estimator.h:50: // Returns true if cached network
quality estimate was successfully read.
On 2015/06/01 20:39:35, bengr wrote:
> "if" -> "if the"

Done.

https://codereview.chromium.org/1144163008/diff/60001/net/base/network_qualit...
net/base/network_quality_estimator.h:51: bool
ReadCachedNetworkQualityEstimate();
On 2015/06/01 20:39:34, bengr wrote:
> You might want to bite the bullet now and make this interface asynchronous.
You
> will almost certainly need to do so if you expect to persist these cache
> entries.

This CL is complete in the current form.
I would prefer to do it when doing the disk based caching.

bengr

https://codereview.chromium.org/1144163008/diff/40001/net/base/network_quality_estimator_unittest.cc File net/base/network_quality_estimator_unittest.cc (right): https://codereview.chromium.org/1144163008/diff/40001/net/base/network_quality_estimator_unittest.cc#newcode122 net/base/network_quality_estimator_unittest.cc:122: NetworkQualityEstimator::SetCurrentNetworkNameForTests( On 2015/06/01 21:56:51, tbansal1 wrote: > On 2015/06/01 ...

5 years, 6 months ago (2015-06-01 23:21:10 UTC) #9

https://codereview.chromium.org/1144163008/diff/40001/net/base/network_qualit...
File net/base/network_quality_estimator_unittest.cc (right):

https://codereview.chromium.org/1144163008/diff/40001/net/base/network_qualit...
net/base/network_quality_estimator_unittest.cc:122:
NetworkQualityEstimator::SetCurrentNetworkNameForTests(
On 2015/06/01 21:56:51, tbansal1 wrote:
> On 2015/06/01 20:39:34, bengr wrote:
> > On 2015/05/29 19:13:37, tbansal1 wrote:
> > > On 2015/05/29 17:38:10, bengr wrote:
> > > > It would be better, maybe, if you could use a test NCN instead of
> > introducing
> > > a
> > > > new pathway just for tests.
> > > 
> > > NCN only exposes ConnectionType and not 
> > > the SSID name or MCC/MNC code.
> > > SSID/MCCMNC names comes from different codebases
> > > which IIUC do not have mocking/test classes.
> > 
> > Uggh. Could you configure NQE to register to listen to a little class you
> write
> > that poses as an NCN?
> 
> Not sure....NCN has nothing to do with SSID name or MCC/MNC names.
> To find out the current SSID/MCCMNC name, this code calls a 
> synchronous network library API (that in turn calls platform specific code).
Not
> sure how posing an NCN would help.
> 
> Mocking the network library API would definitely help but
> I am not sure if that design is better than what I have now.
> 

My mistake. I guess what you have is fine.

https://codereview.chromium.org/1144163008/diff/60001/net/base/network_qualit...
File net/base/network_quality_estimator.cc (right):

https://codereview.chromium.org/1144163008/diff/60001/net/base/network_qualit...
net/base/network_quality_estimator.cc:331: const base::TimeDelta& median_rtt) {
On 2015/06/01 21:56:51, tbansal1 wrote:
> On 2015/06/01 20:39:34, bengr wrote:
> > Would be good to sanity check the parameters, e.g., with DCHECKs.
> 
> Added a TODO in ReadCachedNetworkQualityEstimate().
> 
> I did not add DCHECK because some of the estimates can
> legitimately be 0.
> e.g., if the responses were small, then RTT estimate would be
> non-zero but the Kbps estimate would be zero.

I'm not sure what you mean. rtt and kbps should never be negative, e.g. Also,
both should probably have an upper bound as well, though I'm not sure what good
values would be. Maybe 5 minutes for rtt?

https://codereview.chromium.org/1144163008/diff/60001/net/base/network_qualit...
File net/base/network_quality_estimator.h (right):

https://codereview.chromium.org/1144163008/diff/60001/net/base/network_qualit...
net/base/network_quality_estimator.h:51: bool
ReadCachedNetworkQualityEstimate();
On 2015/06/01 21:56:51, tbansal1 wrote:
> On 2015/06/01 20:39:34, bengr wrote:
> > You might want to bite the bullet now and make this interface asynchronous.
> You
> > will almost certainly need to do so if you expect to persist these cache
> > entries.
> 
> This CL is complete in the current form.
> I would prefer to do it when doing the disk based caching.

Acknowledged.

tbansal1

ptal https://codereview.chromium.org/1144163008/diff/40001/net/base/network_quality_estimator_unittest.cc File net/base/network_quality_estimator_unittest.cc (right): https://codereview.chromium.org/1144163008/diff/40001/net/base/network_quality_estimator_unittest.cc#newcode122 net/base/network_quality_estimator_unittest.cc:122: NetworkQualityEstimator::SetCurrentNetworkNameForTests( On 2015/06/01 23:21:10, bengr wrote: > On ...

5 years, 6 months ago (2015-06-02 18:18:05 UTC) #11

ptal

https://codereview.chromium.org/1144163008/diff/40001/net/base/network_qualit...
File net/base/network_quality_estimator_unittest.cc (right):

https://codereview.chromium.org/1144163008/diff/40001/net/base/network_qualit...
net/base/network_quality_estimator_unittest.cc:122:
NetworkQualityEstimator::SetCurrentNetworkNameForTests(
On 2015/06/01 23:21:10, bengr wrote:
> On 2015/06/01 21:56:51, tbansal1 wrote:
> > On 2015/06/01 20:39:34, bengr wrote:
> > > On 2015/05/29 19:13:37, tbansal1 wrote:
> > > > On 2015/05/29 17:38:10, bengr wrote:
> > > > > It would be better, maybe, if you could use a test NCN instead of
> > > introducing
> > > > a
> > > > > new pathway just for tests.
> > > > 
> > > > NCN only exposes ConnectionType and not 
> > > > the SSID name or MCC/MNC code.
> > > > SSID/MCCMNC names comes from different codebases
> > > > which IIUC do not have mocking/test classes.
> > > 
> > > Uggh. Could you configure NQE to register to listen to a little class you
> > write
> > > that poses as an NCN?
> > 
> > Not sure....NCN has nothing to do with SSID name or MCC/MNC names.
> > To find out the current SSID/MCCMNC name, this code calls a 
> > synchronous network library API (that in turn calls platform specific code).
> Not
> > sure how posing an NCN would help.
> > 
> > Mocking the network library API would definitely help but
> > I am not sure if that design is better than what I have now.
> > 
> 
> My mistake. I guess what you have is fine.

Done.

https://codereview.chromium.org/1144163008/diff/60001/net/base/network_qualit...
File net/base/network_quality_estimator.cc (right):

https://codereview.chromium.org/1144163008/diff/60001/net/base/network_qualit...
net/base/network_quality_estimator.cc:331: const base::TimeDelta& median_rtt) {
On 2015/06/01 23:21:10, bengr wrote:
> On 2015/06/01 21:56:51, tbansal1 wrote:
> > On 2015/06/01 20:39:34, bengr wrote:
> > > Would be good to sanity check the parameters, e.g., with DCHECKs.
> > 
> > Added a TODO in ReadCachedNetworkQualityEstimate().
> > 
> > I did not add DCHECK because some of the estimates can
> > legitimately be 0.
> > e.g., if the responses were small, then RTT estimate would be
> > non-zero but the Kbps estimate would be zero.
> 
> I'm not sure what you mean. rtt and kbps should never be negative, e.g. Also,
> both should probably have an upper bound as well, though I'm not sure what
good
> values would be. Maybe 5 minutes for rtt?

Added DCHECKs. Also, added filters in NotifyDataReceived().

https://codereview.chromium.org/1144163008/diff/60001/net/base/network_qualit...
File net/base/network_quality_estimator.h (right):

https://codereview.chromium.org/1144163008/diff/60001/net/base/network_qualit...
net/base/network_quality_estimator.h:51: bool
ReadCachedNetworkQualityEstimate();
On 2015/06/01 23:21:10, bengr wrote:
> On 2015/06/01 21:56:51, tbansal1 wrote:
> > On 2015/06/01 20:39:34, bengr wrote:
> > > You might want to bite the bullet now and make this interface
asynchronous.
> > You
> > > will almost certainly need to do so if you expect to persist these cache
> > > entries.
> > 
> > This CL is complete in the current form.
> > I would prefer to do it when doing the disk based caching.
> 
> Acknowledged.

Done.

bengr

https://codereview.chromium.org/1144163008/diff/140001/net/base/network_quality_estimator.cc File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1144163008/diff/140001/net/base/network_quality_estimator.cc#newcode225 net/base/network_quality_estimator.cc:225: #if defined(OS_ANDROID) Could we have a NetworkQualityEstimatorAndroid that overrides ...

5 years, 6 months ago (2015-06-05 20:34:31 UTC) #14

mmenke

Sorry I didn't get around to doing a full pass today. I will on Monday. ...

5 years, 6 months ago (2015-06-05 20:37:21 UTC) #15

tbansal1

ptal. thanks. https://codereview.chromium.org/1144163008/diff/140001/net/base/network_quality_estimator.cc File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1144163008/diff/140001/net/base/network_quality_estimator.cc#newcode225 net/base/network_quality_estimator.cc:225: #if defined(OS_ANDROID) On 2015/06/05 20:34:31, bengr wrote: ...

5 years, 6 months ago (2015-06-06 01:03:15 UTC) #16

ptal. thanks.

https://codereview.chromium.org/1144163008/diff/140001/net/base/network_quali...
File net/base/network_quality_estimator.cc (right):

https://codereview.chromium.org/1144163008/diff/140001/net/base/network_quali...
net/base/network_quality_estimator.cc:225: #if defined(OS_ANDROID)
On 2015/06/05 20:34:31, bengr wrote:
> Could we have a NetworkQualityEstimatorAndroid that overrides this method?
Maybe
> add a TODO.

Done. Added TODO

https://codereview.chromium.org/1144163008/diff/140001/net/base/network_quali...
net/base/network_quality_estimator.cc:226: case
NetworkChangeNotifier::ConnectionType::CONNECTION_WIFI:
On 2015/06/05 20:37:21, mmenke wrote:
> Erm...WIFI only on Android?

Added Linux and ChromeOS. GetWiFiSSID() is only implemented
on these 3 platforms.

https://codereview.chromium.org/1144163008/diff/140001/net/base/network_quali...
net/base/network_quality_estimator.cc:234: #endif  // OS_ANDROID
On 2015/06/05 20:37:20, mmenke wrote:
> Can't ChromeOS have cell connections?  What about iOS?  Heck, at some point,
we
> may be getting these types on other platforms, too.

AFAIK, GetTelephonyNetworkOperator() would compile only on Android.
Is there some way to obtain the MCC/MNC for non Android platforms?

Also, e.g., if the connection is cellular on iOS, this code will still
do the caching. It will just be less inaccurate because the network
name (MCC/MNC code) would be empty.

Rearranged the code to make it more obvious.

https://codereview.chromium.org/1144163008/diff/140001/net/base/network_quali...
net/base/network_quality_estimator.cc:348: base::TimeTicks
NetworkQualityEstimator::CachedNetworkQuality::GetLastUpdated()
On 2015/06/05 20:34:31, bengr wrote:
> Can this be inlined in the .h and renamed last_update_time()? Rename the
member
> variable last_update_time_

Done.

https://codereview.chromium.org/1144163008/diff/140001/net/base/network_quali...
File net/base/network_quality_estimator.h (right):

https://codereview.chromium.org/1144163008/diff/140001/net/base/network_quali...
net/base/network_quality_estimator.h:70: // A network is uniquely identified by
a combination of |connection_type_|
On 2015/06/05 20:34:31, bengr wrote:
> Not always unique. name could be empty.

I meant "This code uniquely identifies a network by..."

because otherwise even if the name is non-empty, two 
completely different WiFi networks can have same name.

mmenke

Mostly style nits. https://codereview.chromium.org/1144163008/diff/140001/net/base/network_quality_estimator.cc File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1144163008/diff/140001/net/base/network_quality_estimator.cc#newcode234 net/base/network_quality_estimator.cc:234: #endif // OS_ANDROID On 2015/06/06 01:03:14, ...

5 years, 6 months ago (2015-06-08 20:34:21 UTC) #17

Mostly style nits.

https://codereview.chromium.org/1144163008/diff/140001/net/base/network_quali...
File net/base/network_quality_estimator.cc (right):

https://codereview.chromium.org/1144163008/diff/140001/net/base/network_quali...
net/base/network_quality_estimator.cc:234: #endif  // OS_ANDROID
On 2015/06/06 01:03:14, tbansal1 wrote:
> On 2015/06/05 20:37:20, mmenke wrote:
> > Can't ChromeOS have cell connections?  What about iOS?  Heck, at some point,
> we
> > may be getting these types on other platforms, too.
> 
> AFAIK, GetTelephonyNetworkOperator() would compile only on Android.
> Is there some way to obtain the MCC/MNC for non Android platforms?
> 
> Also, e.g., if the connection is cellular on iOS, this code will still
> do the caching. It will just be less inaccurate because the network
> name (MCC/MNC code) would be empty.
> 
> Rearranged the code to make it more obvious.

There's not, but the old code defaulted to NOTREACHED() if we got
NetworkChangeNotifier::ConnectionType::CONNECTION_WIFI on a platform other than
Android, which seemed to be a bug.

https://codereview.chromium.org/1144163008/diff/160001/net/base/network_quali...
File net/base/network_quality_estimator.cc (right):

https://codereview.chromium.org/1144163008/diff/160001/net/base/network_quali...
net/base/network_quality_estimator.cc:195: const std::string& network_name) {
Suggest making UpdateCurrentNetworkName call a virtual GetCurrentNetworkName
method instead, and override it in tests.  That allows you to also validate we
update the network internally on network changes, by simulating an NCN
notification.

https://codereview.chromium.org/1144163008/diff/160001/net/base/network_quali...
net/base/network_quality_estimator.cc:216: // this method on Android platform.
Is there a bug for this that we could link to, to give context?

https://codereview.chromium.org/1144163008/diff/160001/net/base/network_quali...
net/base/network_quality_estimator.cc:226: current_network_name_ = "ethernet";
I don't think this is needed - we're recording network type and name in the
cache, right?

https://codereview.chromium.org/1144163008/diff/160001/net/base/network_quali...
net/base/network_quality_estimator.cc:237: current_network_name_ =
android::GetTelephonyNetworkOperator();
Suggest a struct:

struct NetworkID {
  ConnectionType type;
  std::string id;
};

Having a string "id" as part of NetworkID is a little weird, but I think it's
clearer than saying the "name" of a network is "ethernet" or the network
operator name.

Alternatively, could use prefixed strings ("wifi/" + GetWifiSSID(), "cell/" +
GetTelephonyNetworkOperator()).  Makes for simpler data structures, though does
lose a bit in clarity.

https://codereview.chromium.org/1144163008/diff/160001/net/base/network_quali...
net/base/network_quality_estimator.cc:277: for (auto& network_quality :
cached_network_quality_) {
Why not just use an std::map?

https://codereview.chromium.org/1144163008/diff/160001/net/base/network_quali...
net/base/network_quality_estimator.cc:290: median_rtt));
Rather than duplicate this line, suggest you do:

if (cached_network_quality_.size() >= kMaximumNetworkQualityCacheSize) {
  // Remove oldest entry here.
}

// Add new entry here, unconditionally.

https://codereview.chromium.org/1144163008/diff/160001/net/base/network_quali...
net/base/network_quality_estimator.cc:294:
DCHECK_EQ(kMaximumNetworkQualityCacheSize, cached_network_quality_.size());
optional:  Suggest a DCHECK_LE(kMaximumNetworkQualityCacheSize,
cached_network_quality_.size()); at the start of this function instead.

https://codereview.chromium.org/1144163008/diff/160001/net/base/network_quali...
net/base/network_quality_estimator.cc:341: DCHECK_GE(median_rtt_,
base::TimeDelta());
nit:  DCHECKs to validate input are generally put at the start of methods.

https://codereview.chromium.org/1144163008/diff/160001/net/base/network_quali...
File net/base/network_quality_estimator.h (right):

https://codereview.chromium.org/1144163008/diff/160001/net/base/network_quali...
net/base/network_quality_estimator.h:71: // and |network_name_|.
SSIDs are user-assigned, so unless GetWifiSSID() returns more than that, those
aren't unique.

Moreover, "ethernet" does not uniquely identify an ethernet network, and an
empty string does not uniquely identify anything (On platforms we can't get an
SSID on).

https://codereview.chromium.org/1144163008/diff/160001/net/base/network_quali...
net/base/network_quality_estimator.h:98: base::TimeDelta median_rtt_;
Seems like we should just have a NetworkQuality object here.

tbansal1

thanks for the comments. ptal. https://codereview.chromium.org/1144163008/diff/160001/net/base/network_quality_estimator.cc File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1144163008/diff/160001/net/base/network_quality_estimator.cc#newcode195 net/base/network_quality_estimator.cc:195: const std::string& network_name) { ...

5 years, 6 months ago (2015-06-09 02:23:13 UTC) #19

thanks for the comments. ptal.

https://codereview.chromium.org/1144163008/diff/160001/net/base/network_quali...
File net/base/network_quality_estimator.cc (right):

https://codereview.chromium.org/1144163008/diff/160001/net/base/network_quali...
net/base/network_quality_estimator.cc:195: const std::string& network_name) {
On 2015/06/08 20:34:19, mmenke wrote:
> Suggest making UpdateCurrentNetworkName call a virtual GetCurrentNetworkName
> method instead, and override it in tests.  That allows you to also validate we
> update the network internally on network changes, by simulating an NCN
> notification.

Done.

https://codereview.chromium.org/1144163008/diff/160001/net/base/network_quali...
net/base/network_quality_estimator.cc:216: // this method on Android platform.
On 2015/06/08 20:34:20, mmenke wrote:
> Is there a bug for this that we could link to, to give context?

Done.

https://codereview.chromium.org/1144163008/diff/160001/net/base/network_quali...
net/base/network_quality_estimator.cc:226: current_network_name_ = "ethernet";
On 2015/06/08 20:34:20, mmenke wrote:
> I don't think this is needed - we're recording network type and name in the
> cache, right?

Yes, it was not really needed.

https://codereview.chromium.org/1144163008/diff/160001/net/base/network_quali...
net/base/network_quality_estimator.cc:237: current_network_name_ =
android::GetTelephonyNetworkOperator();
On 2015/06/08 20:34:19, mmenke wrote:
> Suggest a struct:
> 
> struct NetworkID {
>   ConnectionType type;
>   std::string id;
> };
> 
> Having a string "id" as part of NetworkID is a little weird, but I think it's
> clearer than saying the "name" of a network is "ethernet" or the network
> operator name.
> 
> Alternatively, could use prefixed strings ("wifi/" + GetWifiSSID(), "cell/" +
> GetTelephonyNetworkOperator()).  Makes for simpler data structures, though
does
> lose a bit in clarity.

Done.

https://codereview.chromium.org/1144163008/diff/160001/net/base/network_quali...
net/base/network_quality_estimator.cc:277: for (auto& network_quality :
cached_network_quality_) {
On 2015/06/08 20:34:20, mmenke wrote:
> Why not just use an std::map?

Did not do it before because I was not using struct NetworkID.
Now, it makes more sense. Done.
Unfortunately, this caused a lot of churn in the code.

https://codereview.chromium.org/1144163008/diff/160001/net/base/network_quali...
net/base/network_quality_estimator.cc:290: median_rtt));
On 2015/06/08 20:34:20, mmenke wrote:
> Rather than duplicate this line, suggest you do:
> 
> if (cached_network_quality_.size() >= kMaximumNetworkQualityCacheSize) {
>   // Remove oldest entry here.
> }
> 
> // Add new entry here, unconditionally.

Done.

https://codereview.chromium.org/1144163008/diff/160001/net/base/network_quali...
net/base/network_quality_estimator.cc:294:
DCHECK_EQ(kMaximumNetworkQualityCacheSize, cached_network_quality_.size());
On 2015/06/08 20:34:19, mmenke wrote:
> optional:  Suggest a DCHECK_LE(kMaximumNetworkQualityCacheSize,
> cached_network_quality_.size()); at the start of this function instead.

Done.

https://codereview.chromium.org/1144163008/diff/160001/net/base/network_quali...
net/base/network_quality_estimator.cc:341: DCHECK_GE(median_rtt_,
base::TimeDelta());
On 2015/06/08 20:34:20, mmenke wrote:
> nit:  DCHECKs to validate input are generally put at the start of methods.

Done.

https://codereview.chromium.org/1144163008/diff/160001/net/base/network_quali...
File net/base/network_quality_estimator.h (right):

https://codereview.chromium.org/1144163008/diff/160001/net/base/network_quali...
net/base/network_quality_estimator.h:71: // and |network_name_|.
On 2015/06/08 20:34:21, mmenke wrote:
> SSIDs are user-assigned, so unless GetWifiSSID() returns more than that, those
> aren't unique.
> 
> Moreover, "ethernet" does not uniquely identify an ethernet network, and an
> empty string does not uniquely identify anything (On platforms we can't get an
> SSID on).

Agreed. Added more comments to make it clearer that "unique" is only
from the perspective of this algorithm.

https://codereview.chromium.org/1144163008/diff/160001/net/base/network_quali...
net/base/network_quality_estimator.h:98: base::TimeDelta median_rtt_;
On 2015/06/08 20:34:21, mmenke wrote:
> Seems like we should just have a NetworkQuality object here.

Done.

mmenke

https://codereview.chromium.org/1144163008/diff/200001/net/base/network_quality_estimator.cc File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1144163008/diff/200001/net/base/network_quality_estimator.cc#newcode179 net/base/network_quality_estimator.cc:179: CacheNetworkQualityEstimate(); Should we verify that the network actually changed ...

5 years, 6 months ago (2015-06-09 19:48:08 UTC) #20

tbansal1

thanks for the comments. Tests look much cleaner now. ptal. https://codereview.chromium.org/1144163008/diff/200001/net/base/network_quality_estimator.cc File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1144163008/diff/200001/net/base/network_quality_estimator.cc#newcode179 ...

5 years, 6 months ago (2015-06-10 00:23:29 UTC) #21

thanks for the comments. Tests look much cleaner now. ptal.

https://codereview.chromium.org/1144163008/diff/200001/net/base/network_quali...
File net/base/network_quality_estimator.cc (right):

https://codereview.chromium.org/1144163008/diff/200001/net/base/network_quali...
net/base/network_quality_estimator.cc:179: CacheNetworkQualityEstimate();
On 2015/06/09 19:48:07, mmenke wrote:
> Should we verify that the network actually changed before caching anything?

I think that would complicate the logic without much benefit. Right now, either
both events (caching and clearing of NQE local state) happen or neither of them.


If we decouple them, that means: we clear local state but do not write
to cache. In this case, we still need to add an entry to the 
circular ring buffer (deque) to seed the estimator. This would require
completely different logic than ReadCachedNQE().

I think when the device goes from 1 network to another, there 
is an intermediate notification with ConnectionType of None.
So, it seems it would be very rare that OnConnectionTypeChanged()
is called but there is no change in ConnectionType.

WDYT?

https://codereview.chromium.org/1144163008/diff/200001/net/base/network_quali...
File net/base/network_quality_estimator_unittest.cc (right):

https://codereview.chromium.org/1144163008/diff/200001/net/base/network_quali...
net/base/network_quality_estimator_unittest.cc:102: class
TestNetworkQualityEstimator : public NetworkQualityEstimator {
On 2015/06/09 19:48:07, mmenke wrote:
> This should be in an anonymous namespace.  May be able to put all the test in
it
> as well.

Moved to anon namespace. Not sure what test can I put there?

https://codereview.chromium.org/1144163008/diff/200001/net/base/network_quali...
net/base/network_quality_estimator_unittest.cc:104:
TestNetworkQualityEstimator() : current_network_name_(std::string()) {}
On 2015/06/09 19:48:07, mmenke wrote:
> nit:  No need to explicitly initialize current_network_name_

Done.

https://codereview.chromium.org/1144163008/diff/200001/net/base/network_quali...
net/base/network_quality_estimator_unittest.cc:118: // NetworkQualityEstimator
implementation that returns the overriden network
On 2015/06/09 19:48:08, mmenke wrote:
> overriden -> overridden

Done.

https://codereview.chromium.org/1144163008/diff/200001/net/base/network_quali...
net/base/network_quality_estimator_unittest.cc:133: size_t cache_size = 1;
On 2015/06/09 19:48:07, mmenke wrote:
> expected_cache_size

Done.

https://codereview.chromium.org/1144163008/diff/200001/net/base/network_quali...
net/base/network_quality_estimator_unittest.cc:136:
NetworkChangeNotifier::ConnectionType::CONNECTION_2G);
On 2015/06/09 19:48:07, mmenke wrote:
> Think this test would be much clearer as a single method.  Something like:
> 
> estimator.SimulateNetworkChangeTo(type, name);
> 
> And get rid of SetCurrentNetworkName.

Thanks, this helped a lot.

https://codereview.chromium.org/1144163008/diff/200001/net/base/network_quali...
net/base/network_quality_estimator_unittest.cc:137: // Cache entry will be added
for (NONE, "")
On 2015/06/09 19:48:07, mmenke wrote:
> Suggest putting this above the line that actually adds a line.

Done.

https://codereview.chromium.org/1144163008/diff/200001/net/base/network_quali...
net/base/network_quality_estimator_unittest.cc:142: // not change with multiple
iterations of the loop).
On 2015/06/09 19:48:08, mmenke wrote:
> Not true - the second iteration adds an entry.
> 
> I think this test is sufficiently confusing that you should just unroll the
> first two loop iterations.

Tried to make it simpler. Removed the for loop.

https://codereview.chromium.org/1144163008/diff/200001/net/base/network_quali...
net/base/network_quality_estimator_unittest.cc:157:
NetworkChangeNotifier::ConnectionType::CONNECTION_2G);
On 2015/06/09 19:48:08, mmenke wrote:
> Should also make a test-1 connection with type 3G, and make sure we correctly
> distinguish the two.

Done.

https://codereview.chromium.org/1144163008/diff/200001/net/base/network_quali...
net/base/network_quality_estimator_unittest.cc:162:
estimator.SetCurrentNetworkName("test-1");
On 2015/06/09 19:48:07, mmenke wrote:
> BUG:  This doesn't actually change current_network_id_.  Need a network change
> notification, too.

Fixed.

https://codereview.chromium.org/1144163008/diff/200001/net/base/network_quali...
net/base/network_quality_estimator_unittest.cc:165:
EXPECT_TRUE(estimator.ReadCachedNetworkQualityEstimate());
On 2015/06/09 19:48:07, mmenke wrote:
> This seems like a very weak check that something was reached.  I'd really like
> to store different information for test-1 and test-2, and see if we can
retrieve
> it.

Done.

https://codereview.chromium.org/1144163008/diff/200001/net/base/network_quali...
net/base/network_quality_estimator_unittest.cc:188: }
On 2015/06/09 19:48:07, mmenke wrote:
> Suggest exposing NetworkQualityEstimator::kMaximumNetworkQualityCacheSize, and
> using that explicitly.

Done.

bengr

https://codereview.chromium.org/1144163008/diff/220001/net/base/network_quality_estimator.cc File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1144163008/diff/220001/net/base/network_quality_estimator.cc#newcode206 net/base/network_quality_estimator.cc:206: // that overrides this method on Android platform. on ...

5 years, 6 months ago (2015-06-11 00:02:28 UTC) #22

tbansal1

Thanks for the comments. ptal. https://codereview.chromium.org/1144163008/diff/220001/net/base/network_quality_estimator.cc File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1144163008/diff/220001/net/base/network_quality_estimator.cc#newcode206 net/base/network_quality_estimator.cc:206: // that overrides this ...

5 years, 6 months ago (2015-06-11 02:20:24 UTC) #23

bengr

lgtm with one variable renaming nit. https://codereview.chromium.org/1144163008/diff/220001/net/base/network_quality_estimator.h File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1144163008/diff/220001/net/base/network_quality_estimator.h#newcode71 net/base/network_quality_estimator.h:71: // same name ...

5 years, 6 months ago (2015-06-13 00:28:55 UTC) #26

pauljensen

On 2015/05/28 15:25:03, mmenke wrote: > Some nits...I'll defer to Paul on this review. Do ...

5 years, 6 months ago (2015-06-16 17:55:38 UTC) #31

mmenke

On 2015/06/16 17:55:38, pauljensen wrote: > On 2015/05/28 15:25:03, mmenke wrote: > > Some nits...I'll ...

5 years, 6 months ago (2015-06-16 19:17:33 UTC) #32

pauljensen

In commit description: Change this: This is the first step towards preserving network quality across ...

5 years, 6 months ago (2015-06-17 12:16:30 UTC) #33

pauljensen

https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quality_estimator.cc File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quality_estimator.cc#newcode33 net/base/network_quality_estimator.cc:33: const size_t NetworkQualityEstimator::kMaximumNetworkQualityCacheSize = 10; Please add a comment ...

5 years, 6 months ago (2015-06-17 14:19:15 UTC) #34

https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quali...
File net/base/network_quality_estimator.cc (right):

https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quali...
net/base/network_quality_estimator.cc:33: const size_t
NetworkQualityEstimator::kMaximumNetworkQualityCacheSize = 10;
Please add a comment like this or preferably address the issue:

This limit should not be increased unless the logic for removing the oldest
cache entry is rewritten to use a doubly-linked-list LRU queue.

As it stands now, your logic to purge the oldest entry makes adding one entry to
the cache O(n) where n is kMaximumNetworkQualityCacheSize, and filling the cache
is O(n^2).
Here's a little example of a doubly-linked-list LRU queue and map:
http://stackoverflow.com/questions/2057424/lru-implementation-in-production-code
If you're going to add the comment, maybe adding a static_assert that checks
kMaximumNetworkQualityCacheSize <= 10 is appropriate next to the other
static_assert for kMaximumNetworkQualityCacheSize and putting the comment there
is a good idea.

https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quali...
net/base/network_quality_estimator.cc:318: switch (current_network_id_.type) {
This is racy.  You're using a potentially old value (current_network_id_.type)
and assuming it's current.  A similar race caused crashes in the
NetworkChangeNotifierAutoDetect, see crrev.com/844146e.
I don't know of a great solution that doesn't require a fair bit of work.
I'd prefer we changed GetCurrentNetworkName to GetCurrentNetworkID.  This will
avoid the split initialization on lines 45 and 54, and at least reduce the race
to one function making it easier to fix later.
You could do something really ugly like:
do {
  type = getCurrentType()
  if (type == wifi) id = GetWifiSSID();
} while (type != getCurrentType())

https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quali...
net/base/network_quality_estimator.cc:328: return std::string();
Does this mean every WiFi network will match every other WiFi network in the
cache?  Please fix this.

https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quali...
File net/base/network_quality_estimator.h (right):

https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quali...
net/base/network_quality_estimator.h:177: typedef std::map<NetworkID,
scoped_ptr<CachedNetworkQuality>>
I don't see an ordered traversal and assume we'll never need one, so please add
a comment or preferably use a hash table:

This does not use a unordered_map or hash_map for code simplicity (key just
implements operator<, rather than hash and equality) and because the map is
tiny.

https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quali...
net/base/network_quality_estimator.h:209: size_t
GetMaximumObservationBufferSizeForTests() const;
Can we remove this and have the tests compare to the internal sizes?

https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quali...
net/base/network_quality_estimator.h:213: bool VerifyBufferSizeForTests(size_t
expected_size) const;
Can we remove this and have the tests compare to the internal sizes?

https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quali...
net/base/network_quality_estimator.h:216: // WiFi SSID (if the user is connected
to a WiFi access point and the SSID
can you add " - " before "WiFi" on this line and before "the" on line 218 so
it's easier to read, and indent lines 217 and 219.

pauljensen

https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quality_estimator.h File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quality_estimator.h#newcode124 net/base/network_quality_estimator.h:124: base::TimeTicks last_update_time() const { return last_update_time_; } Slightly nicer ...

5 years, 6 months ago (2015-06-17 14:23:26 UTC) #35

pauljensen

https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quality_estimator.cc File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quality_estimator.cc#newcode33 net/base/network_quality_estimator.cc:33: const size_t NetworkQualityEstimator::kMaximumNetworkQualityCacheSize = 10; On 2015/06/17 14:19:15, pauljensen ...

5 years, 6 months ago (2015-06-17 14:32:45 UTC) #36

tbansal1

Thanks for the comments. ptal. Unfortunately, changing virtual std::string GetCurrentNetworkName() const; to virtual NetworkID GetCurrentNetworkID() ...

5 years, 6 months ago (2015-06-17 21:18:46 UTC) #37

Thanks for the comments. ptal. Unfortunately, changing
virtual std::string GetCurrentNetworkName() const;
to
virtual NetworkID GetCurrentNetworkID() const;
required making changes in several other places.

https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quali...
File net/base/network_quality_estimator.cc (right):

https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quali...
net/base/network_quality_estimator.cc:33: const size_t
NetworkQualityEstimator::kMaximumNetworkQualityCacheSize = 10;
On 2015/06/17 14:19:15, pauljensen wrote:
> Please add a comment like this or preferably address the issue:
> 
> This limit should not be increased unless the logic for removing the oldest
> cache entry is rewritten to use a doubly-linked-list LRU queue.
Done.
> 
> As it stands now, your logic to purge the oldest entry makes adding one entry
to
> the cache O(n) where n is kMaximumNetworkQualityCacheSize, and filling the
cache
> is O(n^2).
> Here's a little example of a doubly-linked-list LRU queue and map:
>
http://stackoverflow.com/questions/2057424/lru-implementation-in-production-code
Yes, I thought of using map + deque, but I suspected that code complexity is not
worth it (unless the combined data structure is already available somewhere in
Chromium). Further, this happens only on network change, which is rare and
already too slow (just something to note when considering trade-offs). Also,
IIUC, complexity of filling cache is not really important because it happens
over a long period of time (e.g, on average, device may take at least a few
hours to switch over 10 different networks).
> If you're going to add the comment, maybe adding a static_assert that checks
> kMaximumNetworkQualityCacheSize <= 10 is appropriate next to the other
> static_assert for kMaximumNetworkQualityCacheSize and putting the comment
there
> is a good idea.
Done

https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quali...
net/base/network_quality_estimator.cc:33: const size_t
NetworkQualityEstimator::kMaximumNetworkQualityCacheSize = 10;
On 2015/06/17 14:32:45, pauljensen wrote:
> On 2015/06/17 14:19:15, pauljensen wrote:
> > Please add a comment like this or preferably address the issue:
> > 
> > This limit should not be increased unless the logic for removing the oldest
> > cache entry is rewritten to use a doubly-linked-list LRU queue.
> > 
> > As it stands now, your logic to purge the oldest entry makes adding one
entry
> to
> > the cache O(n) where n is kMaximumNetworkQualityCacheSize, and filling the
> cache
> > is O(n^2).
> > Here's a little example of a doubly-linked-list LRU queue and map:
> >
>
http://stackoverflow.com/questions/2057424/lru-implementation-in-production-code
> > If you're going to add the comment, maybe adding a static_assert that checks
> > kMaximumNetworkQualityCacheSize <= 10 is appropriate next to the other
> > static_assert for kMaximumNetworkQualityCacheSize and putting the comment
> there
> > is a good idea.
> 
> Another benefit to switching to an LRU queue is that we can remove the
reliance
> on comparing TimeTicks::Now() values.  Matt mentioned horror stories about
> different processor cores exhibiting Now() values significantly out of skew
from
> other cores.  Probably doesn't come in to play for your code (single threaded,
> hopefully large Now() separation) but good future-proofing.

Thats a valid point but I think here for some reason, if a recent entry is
evicted (instead of the oldest one), it would still be okay. This cache is
already best effort and probably code simplicity trumps slightly more accuracy
that we can achieve by using double linked list + map. WDYT? I remember Ben once
suggesting that I should just randomly remove some entry and don't even keep
timestamps.

https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quali...
net/base/network_quality_estimator.cc:318: switch (current_network_id_.type) {
On 2015/06/17 14:19:15, pauljensen wrote:
> This is racy.  You're using a potentially old value (current_network_id_.type)
> and assuming it's current.  A similar race caused crashes in the
> NetworkChangeNotifierAutoDetect, see crrev.com/844146e.
> I don't know of a great solution that doesn't require a fair bit of work.
> I'd prefer we changed GetCurrentNetworkName to GetCurrentNetworkID.  This will
> avoid the split initialization on lines 45 and 54, and at least reduce the
race
> to one function making it easier to fix later.
> You could do something really ugly like:
> do {
>   type = getCurrentType()
>   if (type == wifi) id = GetWifiSSID();
> } while (type != getCurrentType())

Thanks for catching this. I made the change but this required making changes at
several other places. Done.

https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quali...
net/base/network_quality_estimator.cc:328: return std::string();
On 2015/06/17 14:19:15, pauljensen wrote:
> Does this mean every WiFi network will match every other WiFi network in the
> cache?  Please fix this.

Yes, it does mean that on unsupported OSes. My previous logic was to not cache
if the SSID is unavailable. However, we had a discussion where we weighed the
pros/cons: Argument was that the slightly inaccurate value is better than no
value at all. Slightly inaccurate because of the hypothesis that it is likely
that users are always on slow Wi-Fi networks or always on fast Wi-Fi networks.

https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quali...
File net/base/network_quality_estimator.h (right):

https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quali...
net/base/network_quality_estimator.h:124: base::TimeTicks last_update_time()
const { return last_update_time_; }
On 2015/06/17 14:23:26, pauljensen wrote:
> Slightly nicer encapsulation if we replaced this function with a "bool
> older_than(const CachedNetworkQuality&)" function.

Done.

https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quali...
net/base/network_quality_estimator.h:177: typedef std::map<NetworkID,
scoped_ptr<CachedNetworkQuality>>
On 2015/06/17 14:19:15, pauljensen wrote:
> I don't see an ordered traversal and assume we'll never need one, so please
add
> a comment or preferably use a hash table:
> 
> This does not use a unordered_map or hash_map for code simplicity (key just
> implements operator<, rather than hash and equality) and because the map is
> tiny.

Done.

https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quali...
net/base/network_quality_estimator.h:209: size_t
GetMaximumObservationBufferSizeForTests() const;
On 2015/06/17 14:19:15, pauljensen wrote:
> Can we remove this and have the tests compare to the internal sizes?

Right now kMaximumObservationsBufferSize is defined in the cc file (because its
type is size_t which can't be defined in .h). There is no way for the tests to
access it without going through the function. So, still keeping this function.

https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quali...
net/base/network_quality_estimator.h:213: bool VerifyBufferSizeForTests(size_t
expected_size) const;
On 2015/06/17 14:19:15, pauljensen wrote:
> Can we remove this and have the tests compare to the internal sizes?

Removed this function but had to add 2 more functions. It seems that I can't
call estimator.kbps_observations_.Size() in the test code directly. It gives
linking error on windows (it does work on all other platforms):
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp....
I fixed the linking error by
https://codereview.chromium.org/1164713004/diff2/380001:400001/net/base/netwo....

https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quali...
net/base/network_quality_estimator.h:216: // WiFi SSID (if the user is connected
to a WiFi access point and the SSID
On 2015/06/17 14:19:15, pauljensen wrote:
> can you add " - " before "WiFi" on this line and before "the" on line 218 so
> it's easier to read, and indent lines 217 and 219.

Done.

pauljensen

https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quality_estimator.cc File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quality_estimator.cc#newcode328 net/base/network_quality_estimator.cc:328: return std::string(); On 2015/06/17 21:18:46, tbansal1 wrote: > On ...

5 years, 6 months ago (2015-06-18 16:20:26 UTC) #38

tbansal1

ptal, thanks. https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quality_estimator.cc File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quality_estimator.cc#newcode328 net/base/network_quality_estimator.cc:328: return std::string(); On 2015/06/18 16:20:25, pauljensen wrote: ...

5 years, 6 months ago (2015-06-18 20:57:41 UTC) #41

ptal, thanks.

https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quali...
File net/base/network_quality_estimator.cc (right):

https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quali...
net/base/network_quality_estimator.cc:328: return std::string();
On 2015/06/18 16:20:25, pauljensen wrote:
> On 2015/06/17 21:18:46, tbansal1 wrote:
> > On 2015/06/17 14:19:15, pauljensen wrote:
> > > Does this mean every WiFi network will match every other WiFi network in
the
> > > cache?  Please fix this.
> > 
> > Yes, it does mean that on unsupported OSes. My previous logic was to not
cache
> > if the SSID is unavailable. However, we had a discussion where we weighed
the
> > pros/cons: Argument was that the slightly inaccurate value is better than no
> > value at all. Slightly inaccurate because of the hypothesis that it is
likely
> > that users are always on slow Wi-Fi networks or always on fast Wi-Fi
networks.
> 
> I don't think I agree with that.  We already have logic to indicate when there
> is no estimate available, I think we should use it in this case.  I don't
think
> one Wi-Fi's quality gives much information about another Wi-Fi's quality. 
When
> someone changes Wi-Fi networks, they generally do so because one is
> significantly better than the other, the O/S's Wi-Fi logic generally has high
> hysteresis for changing APs.
> If this becomes a big problem, it shouldn't be too hard to implement
> GetWifiSSID() for windows.

I don't think I feel strongly either way (especially since I do not have any
data to backup either approach). Removed read/write to cache if the network name
is unavailable. Also, added test.

https://codereview.chromium.org/1144163008/diff/340001/net/base/network_quali...
File net/base/network_quality_estimator.cc (right):

https://codereview.chromium.org/1144163008/diff/340001/net/base/network_quali...
net/base/network_quality_estimator.cc:355: // Due to race, it is possible that
the connection type changed between when
On 2015/06/18 16:20:26, pauljensen wrote:
> Remove "Due to race, "

Done.

https://codereview.chromium.org/1144163008/diff/340001/net/base/network_quali...
net/base/network_quality_estimator.cc:356: // GetCurrentConnectionType() was
read and when network name was read.
On 2015/06/18 16:20:25, pauljensen wrote:
> network->the network

Done.

https://codereview.chromium.org/1144163008/diff/340001/net/base/network_quali...
net/base/network_quality_estimator.cc:356: // GetCurrentConnectionType() was
read and when network name was read.
On 2015/06/18 16:20:26, pauljensen wrote:
> read->called

Done.

https://codereview.chromium.org/1144163008/diff/340001/net/base/network_quali...
net/base/network_quality_estimator.cc:357: // Check if that happened and retry.
On 2015/06/18 16:20:26, pauljensen wrote:
> Mention that this isn't perfect but should catch the majority of cases, and
this
> is just an estimate anyhow.

Done.

https://codereview.chromium.org/1144163008/diff/340001/net/base/network_quali...
net/base/network_quality_estimator.cc:359: return GetCurrentNetworkID();
On 2015/06/18 16:20:26, pauljensen wrote:
> Please use a loop rather than recursion.  Waiting a long time is better than
> crashing quickly from stack overflow.

Using while(true) now, which I found to be slightly cleaner -- I can define and
declare NetworkID inside the loop. Using do-while, I had to declare it before
the loop and update it inside the loop. WDYT?

https://codereview.chromium.org/1144163008/diff/340001/net/base/network_quali...
File net/base/network_quality_estimator.h (right):

https://codereview.chromium.org/1144163008/diff/340001/net/base/network_quali...
net/base/network_quality_estimator.h:55: protected:
On 2015/06/18 16:20:26, pauljensen wrote:
> Is this protected (rather than private) only for testing purposes? If so, add
a
> comment.

Yes, it is only for testing. Added comment. Done.

pauljensen

https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quality_estimator.h File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quality_estimator.h#newcode209 net/base/network_quality_estimator.h:209: size_t GetMaximumObservationBufferSizeForTests() const; On 2015/06/17 21:18:46, tbansal1 wrote: > ...

5 years, 6 months ago (2015-06-19 03:10:07 UTC) #42

tbansal1

Thanks for the comments. PTAL. https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quality_estimator.h File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quality_estimator.h#newcode209 net/base/network_quality_estimator.h:209: size_t GetMaximumObservationBufferSizeForTests() const; On ...

5 years, 6 months ago (2015-06-19 23:34:40 UTC) #43

Thanks for the comments. PTAL.

https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quali...
File net/base/network_quality_estimator.h (right):

https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quali...
net/base/network_quality_estimator.h:209: size_t
GetMaximumObservationBufferSizeForTests() const;
On 2015/06/19 03:10:07, pauljensen wrote:
> On 2015/06/17 21:18:46, tbansal1 wrote:
> > On 2015/06/17 14:19:15, pauljensen wrote:
> > > Can we remove this and have the tests compare to the internal sizes?
> > 
> > Right now kMaximumObservationsBufferSize is defined in the cc file (because
> its
> > type is size_t which can't be defined in .h). There is no way for the tests
to
> > access it without going through the function. So, still keeping this
function.
> 
> I don't understand.  Why can't we replace this function declaration with
"static
> const size_t kMaximumObservationsBufferSize = 500;" that the friended test
> classes can read?

For some reason, this gives the linking error both in nqe.cc and nqe_test.cc.
Typecasting it by size_t solves the linking error (this is what I am doing right
now) but I am not sure if that's the right approach. Seems that it is not
preferable to define static const size_t in header file. Another approach is to
simply use const size_t (e.g.,
https://code.google.com/p/chromium/codesearch#chromium/src/net/quic/quic_fram...).

So, there are 3 possible approaches
(1) Typecase size_t like what I am doing now
(2) Define the variables as const size_t (similar to quic code above)
(3) Declare and define in anonymous namespace in .cc file and have functions
that expose it to the tests.

Please let me know which approach you think is best.

https://codereview.chromium.org/1144163008/diff/340001/net/base/network_quali...
File net/base/network_quality_estimator.cc (right):

https://codereview.chromium.org/1144163008/diff/340001/net/base/network_quali...
net/base/network_quality_estimator.cc:396:
(it->second)->UpdateNetworkQuality(median_kbps, median_rtt);
On 2015/06/19 03:10:07, pauljensen wrote:
> This in-place update seems odd.  Why not just:
>   cached_network_qualities_[current_network_id_] = CachedNetworkQuality(...)
> and then you can remove UpdateNetworkQuality() and make the members of
> CachedNetworkQuality const.

For this, I will need to overload = operator for CachedNetworkQuality. That
prevents me from making members of CachedNetworkQuality const.

Other option is to reset the scoped ptr in the map by creating a new
CachedNetworkQuality oject. This will call the constructor and not the
assignment operator. That way I can make members of CachedNetworkQuality as
const and not have to overload = operator. Please let me know if that SGTY? My
only concern was that we would be calling destructor and constructor but I am
not sure if that's a valid concern. Thanks.

https://codereview.chromium.org/1144163008/diff/340001/net/base/network_quali...
File net/base/network_quality_estimator.h (right):

https://codereview.chromium.org/1144163008/diff/340001/net/base/network_quali...
net/base/network_quality_estimator.h:84: // - Wi-Fi SSID (if the user is
connected to a Wi-Fi access point and the
On 2015/06/19 03:10:07, pauljensen wrote:
> user->device
> remove ()
> This will make it like the following bullet point

Done.

https://codereview.chromium.org/1144163008/diff/400001/net/base/network_quali...
File net/base/network_quality_estimator.cc (right):

https://codereview.chromium.org/1144163008/diff/400001/net/base/network_quali...
net/base/network_quality_estimator.cc:327: // GetCurrentConnectionType() was
called and when the API to determine the
On 2015/06/19 03:10:07, pauljensen wrote:
> GetCurrent->Get

Thanks for the catch. Done.

pauljensen

https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quality_estimator.h File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quality_estimator.h#newcode209 net/base/network_quality_estimator.h:209: size_t GetMaximumObservationBufferSizeForTests() const; On 2015/06/19 23:34:40, tbansal1 wrote: > ...

5 years, 6 months ago (2015-06-20 01:30:46 UTC) #44

https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quali...
File net/base/network_quality_estimator.h (right):

https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quali...
net/base/network_quality_estimator.h:209: size_t
GetMaximumObservationBufferSizeForTests() const;
On 2015/06/19 23:34:40, tbansal1 wrote:
> On 2015/06/19 03:10:07, pauljensen wrote:
> > On 2015/06/17 21:18:46, tbansal1 wrote:
> > > On 2015/06/17 14:19:15, pauljensen wrote:
> > > > Can we remove this and have the tests compare to the internal sizes?
> > > 
> > > Right now kMaximumObservationsBufferSize is defined in the cc file
(because
> > its
> > > type is size_t which can't be defined in .h). There is no way for the
tests
> to
> > > access it without going through the function. So, still keeping this
> function.
> > 
> > I don't understand.  Why can't we replace this function declaration with
> "static
> > const size_t kMaximumObservationsBufferSize = 500;" that the friended test
> > classes can read?
> 
> For some reason, this gives the linking error both in nqe.cc and nqe_test.cc.
> Typecasting it by size_t solves the linking error (this is what I am doing
right
> now) but I am not sure if that's the right approach. Seems that it is not
> preferable to define static const size_t in header file. Another approach is
to
> simply use const size_t (e.g.,
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/quic/quic_fram...).
> 
> 
> So, there are 3 possible approaches
> (1) Typecase size_t like what I am doing now
> (2) Define the variables as const size_t (similar to quic code above)
> (3) Declare and define in anonymous namespace in .cc file and have functions
> that expose it to the tests.
> 
> Please let me know which approach you think is best.

What link error are you seeing?  There are plenty of places that define static
const size_t's in headers:
~/chrome/src$ git grep "  static const size_t" -- *.h | wc -l
181

https://codereview.chromium.org/1144163008/diff/340001/net/base/network_quali...
File net/base/network_quality_estimator.cc (right):

https://codereview.chromium.org/1144163008/diff/340001/net/base/network_quali...
net/base/network_quality_estimator.cc:396:
(it->second)->UpdateNetworkQuality(median_kbps, median_rtt);
On 2015/06/19 23:34:40, tbansal1 wrote:
> On 2015/06/19 03:10:07, pauljensen wrote:
> > This in-place update seems odd.  Why not just:
> >   cached_network_qualities_[current_network_id_] = CachedNetworkQuality(...)
> > and then you can remove UpdateNetworkQuality() and make the members of
> > CachedNetworkQuality const.
> 
> For this, I will need to overload = operator for CachedNetworkQuality.
SGTM

> That
> prevents me from making members of CachedNetworkQuality const.
Fine; they're already non-const.

> 
> Other option is to reset the scoped ptr in the map by creating a new
> CachedNetworkQuality oject. This will call the constructor and not the
> assignment operator. That way I can make members of CachedNetworkQuality as
> const and not have to overload = operator. Please let me know if that SGTY? My
> only concern was that we would be calling destructor and constructor but I am
> not sure if that's a valid concern. Thanks.

tbansal1

ptal. thanks. https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quality_estimator.h File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quality_estimator.h#newcode209 net/base/network_quality_estimator.h:209: size_t GetMaximumObservationBufferSizeForTests() const; On 2015/06/20 01:30:45, pauljensen ...

5 years, 6 months ago (2015-06-20 03:09:29 UTC) #45

ptal. thanks.

https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quali...
File net/base/network_quality_estimator.h (right):

https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quali...
net/base/network_quality_estimator.h:209: size_t
GetMaximumObservationBufferSizeForTests() const;
On 2015/06/20 01:30:45, pauljensen wrote:
> On 2015/06/19 23:34:40, tbansal1 wrote:
> > On 2015/06/19 03:10:07, pauljensen wrote:
> > > On 2015/06/17 21:18:46, tbansal1 wrote:
> > > > On 2015/06/17 14:19:15, pauljensen wrote:
> > > > > Can we remove this and have the tests compare to the internal sizes?
> > > > 
> > > > Right now kMaximumObservationsBufferSize is defined in the cc file
> (because
> > > its
> > > > type is size_t which can't be defined in .h). There is no way for the
> tests
> > to
> > > > access it without going through the function. So, still keeping this
> > function.
> > > 
> > > I don't understand.  Why can't we replace this function declaration with
> > "static
> > > const size_t kMaximumObservationsBufferSize = 500;" that the friended test
> > > classes can read?
> > 
> > For some reason, this gives the linking error both in nqe.cc and
nqe_test.cc.
> > Typecasting it by size_t solves the linking error (this is what I am doing
> right
> > now) but I am not sure if that's the right approach. Seems that it is not
> > preferable to define static const size_t in header file. Another approach is
> to
> > simply use const size_t (e.g.,
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/quic/quic_fram...).
> > 
> > 
> > So, there are 3 possible approaches
> > (1) Typecase size_t like what I am doing now
> > (2) Define the variables as const size_t (similar to quic code above)
> > (3) Declare and define in anonymous namespace in .cc file and have functions
> > that expose it to the tests.
> > 
> > Please let me know which approach you think is best.
> 
> What link error are you seeing?  There are plenty of places that define static
> const size_t's in headers:
> ~/chrome/src$ git grep "  static const size_t" -- *.h | wc -l
> 181

Forgot to mention that the linking error comes only in DCHECK or other macro
statements. It does not show up in non-MACRO code. Weird.

Here is the linking error: https://goto.google.com/linkingerror

It works if the static const is not a class member but not as a class member.
e.g., if you search for 
static\ const\ size_t.*_\ .* f:h$ -f:test -f:third_party
in cs.chromium.org, there are only 3 results. (This regex "tries" to look only
for static const class members that are defined in the header)

tbansal1

ptal. thanks. https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quality_estimator.h File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quality_estimator.h#newcode209 net/base/network_quality_estimator.h:209: size_t GetMaximumObservationBufferSizeForTests() const; On 2015/06/20 01:30:45, pauljensen ...

5 years, 6 months ago (2015-06-20 03:09:30 UTC) #46

ptal. thanks.

https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quali...
File net/base/network_quality_estimator.h (right):

https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quali...
net/base/network_quality_estimator.h:209: size_t
GetMaximumObservationBufferSizeForTests() const;
On 2015/06/20 01:30:45, pauljensen wrote:
> On 2015/06/19 23:34:40, tbansal1 wrote:
> > On 2015/06/19 03:10:07, pauljensen wrote:
> > > On 2015/06/17 21:18:46, tbansal1 wrote:
> > > > On 2015/06/17 14:19:15, pauljensen wrote:
> > > > > Can we remove this and have the tests compare to the internal sizes?
> > > > 
> > > > Right now kMaximumObservationsBufferSize is defined in the cc file
> (because
> > > its
> > > > type is size_t which can't be defined in .h). There is no way for the
> tests
> > to
> > > > access it without going through the function. So, still keeping this
> > function.
> > > 
> > > I don't understand.  Why can't we replace this function declaration with
> > "static
> > > const size_t kMaximumObservationsBufferSize = 500;" that the friended test
> > > classes can read?
> > 
> > For some reason, this gives the linking error both in nqe.cc and
nqe_test.cc.
> > Typecasting it by size_t solves the linking error (this is what I am doing
> right
> > now) but I am not sure if that's the right approach. Seems that it is not
> > preferable to define static const size_t in header file. Another approach is
> to
> > simply use const size_t (e.g.,
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/quic/quic_fram...).
> > 
> > 
> > So, there are 3 possible approaches
> > (1) Typecase size_t like what I am doing now
> > (2) Define the variables as const size_t (similar to quic code above)
> > (3) Declare and define in anonymous namespace in .cc file and have functions
> > that expose it to the tests.
> > 
> > Please let me know which approach you think is best.
> 
> What link error are you seeing?  There are plenty of places that define static
> const size_t's in headers:
> ~/chrome/src$ git grep "  static const size_t" -- *.h | wc -l
> 181

Forgot to mention that the linking error comes only in DCHECK or other macro
statements. It does not show up in non-MACRO code. Weird.

Here is the linking error: https://goto.google.com/linkingerror

It works if the static const is not a class member but not as a class member.
e.g., if you search for 
static\ const\ size_t.*_\ .* f:h$ -f:test -f:third_party
in cs.chromium.org, there are only 3 results. (This regex "tries" to look only
for static const class members that are defined in the header)

tbansal1

https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quality_estimator.h File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quality_estimator.h#newcode209 net/base/network_quality_estimator.h:209: size_t GetMaximumObservationBufferSizeForTests() const; On 2015/06/20 03:09:30, tbansal1 wrote: > ...

5 years, 6 months ago (2015-06-20 03:13:56 UTC) #47

https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quali...
File net/base/network_quality_estimator.h (right):

https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quali...
net/base/network_quality_estimator.h:209: size_t
GetMaximumObservationBufferSizeForTests() const;
On 2015/06/20 03:09:30, tbansal1 wrote:
> On 2015/06/20 01:30:45, pauljensen wrote:
> > On 2015/06/19 23:34:40, tbansal1 wrote:
> > > On 2015/06/19 03:10:07, pauljensen wrote:
> > > > On 2015/06/17 21:18:46, tbansal1 wrote:
> > > > > On 2015/06/17 14:19:15, pauljensen wrote:
> > > > > > Can we remove this and have the tests compare to the internal sizes?
> > > > > 
> > > > > Right now kMaximumObservationsBufferSize is defined in the cc file
> > (because
> > > > its
> > > > > type is size_t which can't be defined in .h). There is no way for the
> > tests
> > > to
> > > > > access it without going through the function. So, still keeping this
> > > function.
> > > > 
> > > > I don't understand.  Why can't we replace this function declaration with
> > > "static
> > > > const size_t kMaximumObservationsBufferSize = 500;" that the friended
test
> > > > classes can read?
> > > 
> > > For some reason, this gives the linking error both in nqe.cc and
> nqe_test.cc.
> > > Typecasting it by size_t solves the linking error (this is what I am doing
> > right
> > > now) but I am not sure if that's the right approach. Seems that it is not
> > > preferable to define static const size_t in header file. Another approach
is
> > to
> > > simply use const size_t (e.g.,
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/quic/quic_fram...).
> > > 
> > > 
> > > So, there are 3 possible approaches
> > > (1) Typecase size_t like what I am doing now
> > > (2) Define the variables as const size_t (similar to quic code above)
> > > (3) Declare and define in anonymous namespace in .cc file and have
functions
> > > that expose it to the tests.
> > > 
> > > Please let me know which approach you think is best.
> > 
> > What link error are you seeing?  There are plenty of places that define
static
> > const size_t's in headers:
> > ~/chrome/src$ git grep "  static const size_t" -- *.h | wc -l
> > 181
> 
> Forgot to mention that the linking error comes only in DCHECK or other macro
> statements. It does not show up in non-MACRO code. Weird.
> 
> Here is the linking error: https://goto.google.com/linkingerror
> 
> It works if the static const is not a class member but not as a class member.
> e.g., if you search for 
> static\ const\ size_t.*_\ .* f:h$ -f:test -f:third_party
> in http://cs.chromium.org, there are only 3 results. (This regex "tries" to
look only
> for static const class members that are defined in the header)

My regex was incorrect. It does not capture variable names that start with k.
Here is another one that captures that but does not
guarantee that the variable is member of class:
static\ const\ size_t\ k.*=.* f:h$ -f:test -f:third_party

tbansal1

https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quality_estimator.h File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quality_estimator.h#newcode209 net/base/network_quality_estimator.h:209: size_t GetMaximumObservationBufferSizeForTests() const; On 2015/06/20 03:13:56, tbansal1 wrote: > ...

5 years, 6 months ago (2015-06-20 03:21:17 UTC) #48

https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quali...
File net/base/network_quality_estimator.h (right):

https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quali...
net/base/network_quality_estimator.h:209: size_t
GetMaximumObservationBufferSizeForTests() const;
On 2015/06/20 03:13:56, tbansal1 wrote:
> On 2015/06/20 03:09:30, tbansal1 wrote:
> > On 2015/06/20 01:30:45, pauljensen wrote:
> > > On 2015/06/19 23:34:40, tbansal1 wrote:
> > > > On 2015/06/19 03:10:07, pauljensen wrote:
> > > > > On 2015/06/17 21:18:46, tbansal1 wrote:
> > > > > > On 2015/06/17 14:19:15, pauljensen wrote:
> > > > > > > Can we remove this and have the tests compare to the internal
sizes?
> > > > > > 
> > > > > > Right now kMaximumObservationsBufferSize is defined in the cc file
> > > (because
> > > > > its
> > > > > > type is size_t which can't be defined in .h). There is no way for
the
> > > tests
> > > > to
> > > > > > access it without going through the function. So, still keeping this
> > > > function.
> > > > > 
> > > > > I don't understand.  Why can't we replace this function declaration
with
> > > > "static
> > > > > const size_t kMaximumObservationsBufferSize = 500;" that the friended
> test
> > > > > classes can read?
> > > > 
> > > > For some reason, this gives the linking error both in nqe.cc and
> > nqe_test.cc.
> > > > Typecasting it by size_t solves the linking error (this is what I am
doing
> > > right
> > > > now) but I am not sure if that's the right approach. Seems that it is
not
> > > > preferable to define static const size_t in header file. Another
approach
> is
> > > to
> > > > simply use const size_t (e.g.,
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/quic/quic_fram...).
> > > > 
> > > > 
> > > > So, there are 3 possible approaches
> > > > (1) Typecase size_t like what I am doing now
> > > > (2) Define the variables as const size_t (similar to quic code above)
> > > > (3) Declare and define in anonymous namespace in .cc file and have
> functions
> > > > that expose it to the tests.
> > > > 
> > > > Please let me know which approach you think is best.
> > > 
> > > What link error are you seeing?  There are plenty of places that define
> static
> > > const size_t's in headers:
> > > ~/chrome/src$ git grep "  static const size_t" -- *.h | wc -l
> > > 181
> > 
> > Forgot to mention that the linking error comes only in DCHECK or other macro
> > statements. It does not show up in non-MACRO code. Weird.
> > 
> > Here is the linking error: https://goto.google.com/linkingerror
> > 
> > It works if the static const is not a class member but not as a class
member.
> > e.g., if you search for 
> > static\ const\ size_t.*_\ .* f:h$ -f:test -f:third_party
> > in http://cs.chromium.org, there are only 3 results. (This regex "tries" to
> look only
> > for static const class members that are defined in the header)
> 
> My regex was incorrect. It does not capture variable names that start with k.
> Here is another one that captures that but does not
> guarantee that the variable is member of class:
> static\ const\ size_t\ k.*=.* f:h$ -f:test -f:third_party

It seems like existing code also does typecast before putting it in  a macro
statement, see this e.g.:
https://code.google.com/p/chromium/codesearch#chromium/src/gpu/command_buffer...

Please let me know what you think is the best approach. I do not have much
Chromium experience but this typecasting approach does not look very great.
WDYT?

pauljensen

On 2015/06/20 03:21:17, tbansal1 wrote: > https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quality_estimator.h > File net/base/network_quality_estimator.h (right): > > https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quality_estimator.h#newcode209 > ...

5 years, 6 months ago (2015-06-22 11:11:03 UTC) #49

On 2015/06/20 03:21:17, tbansal1 wrote:
>
https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quali...
> File net/base/network_quality_estimator.h (right):
> 
>
https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quali...
> net/base/network_quality_estimator.h:209: size_t
> GetMaximumObservationBufferSizeForTests() const;
> On 2015/06/20 03:13:56, tbansal1 wrote:
> > On 2015/06/20 03:09:30, tbansal1 wrote:
> > > On 2015/06/20 01:30:45, pauljensen wrote:
> > > > On 2015/06/19 23:34:40, tbansal1 wrote:
> > > > > On 2015/06/19 03:10:07, pauljensen wrote:
> > > > > > On 2015/06/17 21:18:46, tbansal1 wrote:
> > > > > > > On 2015/06/17 14:19:15, pauljensen wrote:
> > > > > > > > Can we remove this and have the tests compare to the internal
> sizes?
> > > > > > > 
> > > > > > > Right now kMaximumObservationsBufferSize is defined in the cc file
> > > > (because
> > > > > > its
> > > > > > > type is size_t which can't be defined in .h). There is no way for
> the
> > > > tests
> > > > > to
> > > > > > > access it without going through the function. So, still keeping
this
> > > > > function.
> > > > > > 
> > > > > > I don't understand.  Why can't we replace this function declaration
> with
> > > > > "static
> > > > > > const size_t kMaximumObservationsBufferSize = 500;" that the
friended
> > test
> > > > > > classes can read?
> > > > > 
> > > > > For some reason, this gives the linking error both in nqe.cc and
> > > nqe_test.cc.
> > > > > Typecasting it by size_t solves the linking error (this is what I am
> doing
> > > > right
> > > > > now) but I am not sure if that's the right approach. Seems that it is
> not
> > > > > preferable to define static const size_t in header file. Another
> approach
> > is
> > > > to
> > > > > simply use const size_t (e.g.,
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/quic/quic_fram...).
> > > > > 
> > > > > 
> > > > > So, there are 3 possible approaches
> > > > > (1) Typecase size_t like what I am doing now
> > > > > (2) Define the variables as const size_t (similar to quic code above)
> > > > > (3) Declare and define in anonymous namespace in .cc file and have
> > functions
> > > > > that expose it to the tests.
> > > > > 
> > > > > Please let me know which approach you think is best.
> > > > 
> > > > What link error are you seeing?  There are plenty of places that define
> > static
> > > > const size_t's in headers:
> > > > ~/chrome/src$ git grep "  static const size_t" -- *.h | wc -l
> > > > 181
> > > 
> > > Forgot to mention that the linking error comes only in DCHECK or other
macro
> > > statements. It does not show up in non-MACRO code. Weird.
> > > 
> > > Here is the linking error: https://goto.google.com/linkingerror
> > > 
> > > It works if the static const is not a class member but not as a class
> member.
> > > e.g., if you search for 
> > > static\ const\ size_t.*_\ .* f:h$ -f:test -f:third_party
> > > in http://cs.chromium.org, there are only 3 results. (This regex "tries"
to
> > look only
> > > for static const class members that are defined in the header)
> > 
> > My regex was incorrect. It does not capture variable names that start with
k.
> > Here is another one that captures that but does not
> > guarantee that the variable is member of class:
> > static\ const\ size_t\ k.*=.* f:h$ -f:test -f:third_party
> 
> It seems like existing code also does typecast before putting it in  a macro
> statement, see this e.g.:
>
https://code.google.com/p/chromium/codesearch#chromium/src/gpu/command_buffer...
> 
> Please let me know what you think is the best approach. I do not have much
> Chromium experience but this typecasting approach does not look very great.
> WDYT?

I just want to keep the implementation simple and not complicated for testing,
so I'd like those accessor functions removed.
I wonder if this is related to whether the compiler outputs a reference for the
variable and whether a definition is output (i.e. whether the constant has an
address).
See
http://stackoverflow.com/questions/5391973/undefined-reference-to-static-cons...
You could investigate by dumping the generated object files to see if a
reference and definition are there.
I think the solution is to add a definition of the variable to a .cc file.
If that fails to work, I'm fine if you have to do "size_t var = kBlah" in tests.

tbansal1

ptal. thnaks. https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quality_estimator.h File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quality_estimator.h#newcode209 net/base/network_quality_estimator.h:209: size_t GetMaximumObservationBufferSizeForTests() const; On 2015/06/20 03:21:17, tbansal1 ...

5 years, 6 months ago (2015-06-22 17:51:22 UTC) #51

ptal. thnaks.

https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quali...
File net/base/network_quality_estimator.h (right):

https://codereview.chromium.org/1144163008/diff/320001/net/base/network_quali...
net/base/network_quality_estimator.h:209: size_t
GetMaximumObservationBufferSizeForTests() const;
On 2015/06/20 03:21:17, tbansal1 wrote:
> On 2015/06/20 03:13:56, tbansal1 wrote:
> > On 2015/06/20 03:09:30, tbansal1 wrote:
> > > On 2015/06/20 01:30:45, pauljensen wrote:
> > > > On 2015/06/19 23:34:40, tbansal1 wrote:
> > > > > On 2015/06/19 03:10:07, pauljensen wrote:
> > > > > > On 2015/06/17 21:18:46, tbansal1 wrote:
> > > > > > > On 2015/06/17 14:19:15, pauljensen wrote:
> > > > > > > > Can we remove this and have the tests compare to the internal
> sizes?
> > > > > > > 
> > > > > > > Right now kMaximumObservationsBufferSize is defined in the cc file
> > > > (because
> > > > > > its
> > > > > > > type is size_t which can't be defined in .h). There is no way for
> the
> > > > tests
> > > > > to
> > > > > > > access it without going through the function. So, still keeping
this
> > > > > function.
> > > > > > 
> > > > > > I don't understand.  Why can't we replace this function declaration
> with
> > > > > "static
> > > > > > const size_t kMaximumObservationsBufferSize = 500;" that the
friended
> > test
> > > > > > classes can read?
> > > > > 
> > > > > For some reason, this gives the linking error both in nqe.cc and
> > > nqe_test.cc.
> > > > > Typecasting it by size_t solves the linking error (this is what I am
> doing
> > > > right
> > > > > now) but I am not sure if that's the right approach. Seems that it is
> not
> > > > > preferable to define static const size_t in header file. Another
> approach
> > is
> > > > to
> > > > > simply use const size_t (e.g.,
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/quic/quic_fram...).
> > > > > 
> > > > > 
> > > > > So, there are 3 possible approaches
> > > > > (1) Typecase size_t like what I am doing now
> > > > > (2) Define the variables as const size_t (similar to quic code above)
> > > > > (3) Declare and define in anonymous namespace in .cc file and have
> > functions
> > > > > that expose it to the tests.
> > > > > 
> > > > > Please let me know which approach you think is best.
> > > > 
> > > > What link error are you seeing?  There are plenty of places that define
> > static
> > > > const size_t's in headers:
> > > > ~/chrome/src$ git grep "  static const size_t" -- *.h | wc -l
> > > > 181
> > > 
> > > Forgot to mention that the linking error comes only in DCHECK or other
macro
> > > statements. It does not show up in non-MACRO code. Weird.
> > > 
> > > Here is the linking error: https://goto.google.com/linkingerror
> > > 
> > > It works if the static const is not a class member but not as a class
> member.
> > > e.g., if you search for 
> > > static\ const\ size_t.*_\ .* f:h$ -f:test -f:third_party
> > > in http://cs.chromium.org, there are only 3 results. (This regex "tries"
to
> > look only
> > > for static const class members that are defined in the header)
> > 
> > My regex was incorrect. It does not capture variable names that start with
k.
> > Here is another one that captures that but does not
> > guarantee that the variable is member of class:
> > static\ const\ size_t\ k.*=.* f:h$ -f:test -f:third_party
> 
> It seems like existing code also does typecast before putting it in  a macro
> statement, see this e.g.:
>
https://code.google.com/p/chromium/codesearch#chromium/src/gpu/command_buffer...
> 
> Please let me know what you think is the best approach. I do not have much
> Chromium experience but this typecasting approach does not look very great.
> WDYT?

Done.

pauljensen

https://codereview.chromium.org/1144163008/diff/340001/net/base/network_quality_estimator.cc File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1144163008/diff/340001/net/base/network_quality_estimator.cc#newcode396 net/base/network_quality_estimator.cc:396: (it->second)->UpdateNetworkQuality(median_kbps, median_rtt); On 2015/06/20 01:30:45, pauljensen wrote: > On ...

5 years, 6 months ago (2015-06-22 18:39:53 UTC) #52

tbansal1

ptal. https://codereview.chromium.org/1144163008/diff/340001/net/base/network_quality_estimator.cc File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1144163008/diff/340001/net/base/network_quality_estimator.cc#newcode396 net/base/network_quality_estimator.cc:396: (it->second)->UpdateNetworkQuality(median_kbps, median_rtt); On 2015/06/22 18:39:52, pauljensen wrote: > ...

5 years, 6 months ago (2015-06-22 20:28:26 UTC) #55

pauljensen

https://codereview.chromium.org/1144163008/diff/460001/net/base/network_quality.h File net/base/network_quality.h (right): https://codereview.chromium.org/1144163008/diff/460001/net/base/network_quality.h#newcode23 net/base/network_quality.h:23: remove new line https://codereview.chromium.org/1144163008/diff/460001/net/base/network_quality.h#newcode25 net/base/network_quality.h:25: remove new line https://codereview.chromium.org/1144163008/diff/460001/net/base/network_quality_estimator.h ...

5 years, 6 months ago (2015-06-23 11:52:10 UTC) #56

tbansal1

On 2015/06/23 11:52:10, pauljensen wrote: > https://codereview.chromium.org/1144163008/diff/460001/net/base/network_quality.h > File net/base/network_quality.h (right): > > https://codereview.chromium.org/1144163008/diff/460001/net/base/network_quality.h#newcode23 > ...

5 years, 6 months ago (2015-06-23 14:32:58 UTC) #57

mmenke

mmenke@chromium.org changed reviewers: - mmenke@chromium.org

5 years, 5 months ago (2015-07-07 18:03:58 UTC) #58

tbansal1

pauljensen@: ptal. Lot of rebasing since you last had a look. https://codereview.chromium.org/1144163008/diff/460001/net/base/network_quality.h File net/base/network_quality.h (right): ...

5 years, 5 months ago (2015-07-13 21:21:26 UTC) #60

pauljensen

https://codereview.chromium.org/1144163008/diff/560001/net/base/network_quality_estimator.cc File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1144163008/diff/560001/net/base/network_quality_estimator.cc#newcode120 net/base/network_quality_estimator.cc:120: const size_t NetworkQualityEstimator::kMaximumNetworkQualityCacheSize = 10; how about initializing this ...

5 years, 5 months ago (2015-07-14 11:59:03 UTC) #61

tbansal1

ptal. thanks. https://codereview.chromium.org/1144163008/diff/560001/net/base/network_quality_estimator.cc File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1144163008/diff/560001/net/base/network_quality_estimator.cc#newcode120 net/base/network_quality_estimator.cc:120: const size_t NetworkQualityEstimator::kMaximumNetworkQualityCacheSize = 10; On 2015/07/14 ...

5 years, 5 months ago (2015-07-14 19:51:48 UTC) #62

pauljensen

https://codereview.chromium.org/1144163008/diff/560001/net/base/network_quality_estimator.cc File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1144163008/diff/560001/net/base/network_quality_estimator.cc#newcode120 net/base/network_quality_estimator.cc:120: const size_t NetworkQualityEstimator::kMaximumNetworkQualityCacheSize = 10; On 2015/07/14 19:51:48, tbansal1 ...

5 years, 5 months ago (2015-07-14 20:19:59 UTC) #63

tbansal1

On 2015/07/14 20:19:59, pauljensen wrote: > https://codereview.chromium.org/1144163008/diff/560001/net/base/network_quality_estimator.cc > File net/base/network_quality_estimator.cc (right): > > https://codereview.chromium.org/1144163008/diff/560001/net/base/network_quality_estimator.cc#newcode120 > ...

5 years, 5 months ago (2015-07-14 21:26:20 UTC) #64

pauljensen

On 2015/07/14 21:26:20, tbansal1 wrote: > On 2015/07/14 20:19:59, pauljensen wrote: > > > https://codereview.chromium.org/1144163008/diff/560001/net/base/network_quality_estimator.cc ...

5 years, 5 months ago (2015-07-15 15:12:02 UTC) #65

On 2015/07/14 21:26:20, tbansal1 wrote:
> On 2015/07/14 20:19:59, pauljensen wrote:
> >
>
https://codereview.chromium.org/1144163008/diff/560001/net/base/network_quali...
> > File net/base/network_quality_estimator.cc (right):
> > 
> >
>
https://codereview.chromium.org/1144163008/diff/560001/net/base/network_quali...
> > net/base/network_quality_estimator.cc:120: const size_t
> > NetworkQualityEstimator::kMaximumNetworkQualityCacheSize = 10;
> > On 2015/07/14 19:51:48, tbansal1 wrote:
> > > On 2015/07/14 11:59:03, pauljensen wrote:
> > > > how about initializing this in the header like the other constants
there?
> > > 
> > > I get linking error:
> > > ../../net/base/network_quality_estimator.cc:692: error: undefined
reference
> to
> > > 'net::NetworkQualityEstimator::kMaximumNetworkQualityCacheSize'
> > > 
> > > I believe it is because it is size_t. Changing it to int fixes the error.
> > 
> > Can you track this down further?  Use objdump or nm to figure out if the
> > definition is present or if the symbol names don't match or what is going on
> > here.  Is it related to NET_EXPORT_PRIVATE?
> > There are litterally hundreds of instances of this elsewhere in Chrome:
> >
>
https://code.google.com/p/chromium/codesearch#search/&q=%22static%20const%20s...
> 
> The linking error only comes when the static const is used in DCHECK macros.
> It seems typecasting is one way of fixing it:
>
https://code.google.com/p/chromium/codesearch#chromium/src/gpu/command_buffer...
> 
> I just used static_cast to fix this (ptal). Not sure why but I did not see the
> definition in the .o file.

Can you track this down further?  Use objdump or nm to figure out if the
definition is present or if the symbol names don't match or what is going on
here.
I see 5 instances of this work-around now.  I'd rather we knew the source of
this problem rather than proliferate a work-around.
At a minimum if we're going to accept the work-around we should file a new bug
about this marked with component Build-Toolchain so the infra folks can
investigate.

pauljensen

I think this is about ready. This is probably my last set of comments. https://codereview.chromium.org/1144163008/diff/600001/net/base/network_quality_estimator.cc ...

5 years, 5 months ago (2015-07-15 15:12:46 UTC) #66

tbansal1

PTAL. Filed Filed https://code.google.com/p/chromium/issues/detail?id=510498 for the linking error. The code looks much cleaner now without ...

5 years, 5 months ago (2015-07-15 17:33:48 UTC) #67

pauljensen

lgtm if "Ethernet" is removed https://codereview.chromium.org/1144163008/diff/600001/net/base/network_quality_estimator.cc File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1144163008/diff/600001/net/base/network_quality_estimator.cc#newcode633 net/base/network_quality_estimator.cc:633: network_id.id = "Ethernet"; On ...

5 years, 5 months ago (2015-07-15 17:44:05 UTC) #68

tbansal1

https://codereview.chromium.org/1144163008/diff/600001/net/base/network_quality_estimator.cc File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1144163008/diff/600001/net/base/network_quality_estimator.cc#newcode633 net/base/network_quality_estimator.cc:633: network_id.id = "Ethernet"; On 2015/07/15 17:44:05, pauljensen wrote: > ...

5 years, 5 months ago (2015-07-15 17:54:19 UTC) #69

tbansal1

The patchset sent to the CQ was uploaded after l-g-t-m from bengr@chromium.org, pauljensen@chromium.org Link to ...

5 years, 5 months ago (2015-07-15 17:54:28 UTC) #71