Add connection type to NCN::MaxBandwidthChanged

This CL adds ConnectionType to the MaxBandwidthObserver notification as a convenience for its only client (in the downstream CL), NetInfo.

This CL also ensures that MaxBandwidthObserver is called on every platform when the network type changes.

Upstream of https://codereview.chromium.org/1306423004/

BUG=412741
Committed: https://crrev.com/bce491860ced584a398c76dca134ed350e879259
Cr-Commit-Position: refs/heads/master@{#349673}

[jkarlin, 2015-09-04 18:51:24]

Patchset #1 (id:1) has been deleted

[jkarlin, 2015-09-08 12:01:50]

jkarlin@chromium.org changed reviewers:
+ pauljensen@chromium.org

[jkarlin, 2015-09-08 12:01:51]

Paul: PTAL, thanks!

[jkarlin, 2015-09-10 23:23:28]

jkarlin@chromium.org changed reviewers:
+ pneubeck@chromium.org

[jkarlin, 2015-09-10 23:23:29]

pneubeck@chromium.org: Please review changes in chromeos/, thanks!

[pneubeck (no reviews), 2015-09-11 07:48:20]

chromeos/network/network_change_notifier_chromeos.cc lgtm

https://codereview.chromium.org/1306653003/diff/80001/net/base/network_change...
File net/base/network_change_notifier.h (right):

https://codereview.chromium.org/1306653003/diff/80001/net/base/network_change...
net/base/network_change_notifier.h:240: // types). See the NetInfo spec for the
mapping of specific subtypes to
s/)//

[jkarlin, 2015-09-11 18:56:02]

On 2015/09/11 07:48:20, pneubeck wrote:
> chromeos/network/network_change_notifier_chromeos.cc lgtm
> 
>
https://codereview.chromium.org/1306653003/diff/80001/net/base/network_change...
> File net/base/network_change_notifier.h (right):
> 
>
https://codereview.chromium.org/1306653003/diff/80001/net/base/network_change...
> net/base/network_change_notifier.h:240: // types). See the NetInfo spec for
the
> mapping of specific subtypes to
> s/)//

Thanks pneubeck, can you look one more time at chromeos/? I had to change
GetCurrentMaxBandwidth to GetCurrentMaxBandwidthAndConncectionType. Thanks
again!

[jkarlin, 2015-09-11 18:56:11]

https://codereview.chromium.org/1306653003/diff/80001/net/base/network_change...
File net/base/network_change_notifier.h (right):

https://codereview.chromium.org/1306653003/diff/80001/net/base/network_change...
net/base/network_change_notifier.h:240: // types). See the NetInfo spec for the
mapping of specific subtypes to
On 2015/09/11 07:48:20, pneubeck wrote:
> s/)//

Done.

[pauljensen, 2015-09-11 18:59:02]

https://codereview.chromium.org/1306653003/diff/140001/net/base/network_chang...
File net/base/network_change_notifier.h (right):

https://codereview.chromium.org/1306653003/diff/140001/net/base/network_chang...
net/base/network_change_notifier.h:245: ConnectionType* connection_type);
Are you going to change this API to take pointers to both items as per our char
earlier?

https://codereview.chromium.org/1306653003/diff/140001/net/base/network_chang...
net/base/network_change_notifier.h:400: ConnectionType* connection_type) const;
ditto

[jkarlin, 2015-09-11 19:04:26]

https://codereview.chromium.org/1306653003/diff/140001/net/base/network_chang...
File net/base/network_change_notifier.h (right):

https://codereview.chromium.org/1306653003/diff/140001/net/base/network_chang...
net/base/network_change_notifier.h:245: ConnectionType* connection_type);
On 2015/09/11 18:59:02, pauljensen wrote:
> Are you going to change this API to take pointers to both items as per our
char
> earlier?

Ah, I took our convo. to mean that you were fine with either arrangement. I
chose this one form so that I could
DCHECK(GetMaxBandwidthAndConnectionType(nullptr) != ...) which is needed in one
of the files.

[pauljensen, 2015-09-11 19:18:59]

https://codereview.chromium.org/1306653003/diff/140001/net/base/network_chang...
File net/base/network_change_notifier.h (right):

https://codereview.chromium.org/1306653003/diff/140001/net/base/network_chang...
net/base/network_change_notifier.h:245: ConnectionType* connection_type);
On 2015/09/11 19:04:26, jkarlin wrote:
> On 2015/09/11 18:59:02, pauljensen wrote:
> > Are you going to change this API to take pointers to both items as per our
> char
> > earlier?
> 
> Ah, I took our convo. to mean that you were fine with either arrangement. I
> chose this one form so that I could
> DCHECK(GetMaxBandwidthAndConnectionType(nullptr) != ...) which is needed in
one
> of the files. 

Oh, no, I was trying to say I wanted it to be:
void GetMaxBandwidthAndConnectionType(double* max_bandwidth_mbps,
ConnectionType* connection_type);
Sorry.  It seems more consistent to me.

[jkarlin, 2015-09-11 20:07:24]

https://codereview.chromium.org/1306653003/diff/140001/net/base/network_chang...
File net/base/network_change_notifier.h (right):

https://codereview.chromium.org/1306653003/diff/140001/net/base/network_chang...
net/base/network_change_notifier.h:245: ConnectionType* connection_type);
On 2015/09/11 19:18:59, pauljensen wrote:
> On 2015/09/11 19:04:26, jkarlin wrote:
> > On 2015/09/11 18:59:02, pauljensen wrote:
> > > Are you going to change this API to take pointers to both items as per our
> > char
> > > earlier?
> > 
> > Ah, I took our convo. to mean that you were fine with either arrangement. I
> > chose this one form so that I could
> > DCHECK(GetMaxBandwidthAndConnectionType(nullptr) != ...) which is needed in
> one
> > of the files. 
> 
> Oh, no, I was trying to say I wanted it to be:
> void GetMaxBandwidthAndConnectionType(double* max_bandwidth_mbps,
> ConnectionType* connection_type);
> Sorry.  It seems more consistent to me.

Done.

https://codereview.chromium.org/1306653003/diff/140001/net/base/network_chang...
net/base/network_change_notifier.h:400: ConnectionType* connection_type) const;
On 2015/09/11 18:59:02, pauljensen wrote:
> ditto

Done.

[jkarlin, 2015-09-14 14:37:40]

PTAL, thanks!

[pneubeck (no reviews), 2015-09-14 16:23:52]

chromeos/network/network_change_notifier_chromeos.*
still lgtm

[pauljensen, 2015-09-15 18:17:40]

Can we give back more accurate max bandwidths given a particular connection
type?  e.g. NCNLinux knows about WiFi or Ethernet, if we know it's WiFi could we
give back the max  bandwidth for all WiFi types? or is this not a good idea as
it could be some futuristic WiFi connection that gets even better bandwidth?

https://codereview.chromium.org/1306653003/diff/200001/net/base/network_chang...
File net/base/network_change_notifier.cc (right):

https://codereview.chromium.org/1306653003/diff/200001/net/base/network_chang...
net/base/network_change_notifier.cc:552: 
so if !g_network_change_notifier we dereference it...  I think you need an else

https://codereview.chromium.org/1306653003/diff/200001/net/base/network_chang...
net/base/network_change_notifier.cc:803: double max_bandwidth) {
add _mbps suffix

https://codereview.chromium.org/1306653003/diff/200001/net/base/network_chang...
net/base/network_change_notifier.cc:804: if (g_network_change_notifier)
need curly braces when more than one line clause

https://codereview.chromium.org/1306653003/diff/200001/net/base/network_chang...
File net/base/network_change_notifier.h (right):

https://codereview.chromium.org/1306653003/diff/200001/net/base/network_chang...
net/base/network_change_notifier.h:192: // GetMaxBanwidth for what to expect for
the values of |max_bandwidth_mbps|.
GetMaxBandwidth->GetMaxBandwidthAndConnectionType

https://codereview.chromium.org/1306653003/diff/200001/net/base/network_chang...
net/base/network_change_notifier.h:238: // is a connection and 0.0 otherwise.
The circumstances in which a more
Um I think it does more than "Specifically, it returns +Infinity if there is a
connection and 0.0 otherwise."

https://codereview.chromium.org/1306653003/diff/200001/net/base/network_chang...
net/base/network_change_notifier.h:239: // specific value is given are:  Android
cellular types, Android wifi types,
wifi->WiFi

https://codereview.chromium.org/1306653003/diff/200001/net/base/network_chang...
net/base/network_change_notifier.h:239: // specific value is given are:  Android
cellular types, Android wifi types,
can we reword the "types" here to something like "when an Android device is
connected to a cellular or WiFi network"

https://codereview.chromium.org/1306653003/diff/200001/net/base/network_chang...
net/base/network_change_notifier.h:242: // |connection_type| is set to the
current ConnectionType.
ConnectionType->active default network's connection type

https://codereview.chromium.org/1306653003/diff/200001/net/base/network_chang...
File net/base/network_change_notifier_linux.cc (right):

https://codereview.chromium.org/1306653003/diff/200001/net/base/network_chang...
net/base/network_change_notifier_linux.cc:90: SUBTYPE_UNKNOWN);
how about ReallyLongFunctionName(conditon ? SUBTYPE_NONE : SUBTYPE_UNKNOWN)?
rather than duplicating ReallyLongFunctionName

https://codereview.chromium.org/1306653003/diff/200001/net/base/network_chang...
File net/base/network_change_notifier_mac.cc (right):

https://codereview.chromium.org/1306653003/diff/200001/net/base/network_chang...
net/base/network_change_notifier_mac.cc:264: ?
NetworkChangeNotifier::GetMaxBandwidthForConnectionSubtype(
ditto

https://codereview.chromium.org/1306653003/diff/200001/net/base/network_chang...
File net/base/network_change_notifier_win.cc (right):

https://codereview.chromium.org/1306653003/diff/200001/net/base/network_chang...
net/base/network_change_notifier_win.cc:325:
NotifyObserversOfMaxBandwidthChange(connection_type, max_bandwidth_mbps);
these two function have opposite argument ordering, can you fix that?

[jkarlin, 2015-09-16 12:12:22]

Thanks Paul! PTAL.

There are two issues with returning the max bandwidth for the type instead of
+Infinity:

1) As you said, the types might change over time.
2) If you do that, then you don't know if you're getting the max value because
that's the actual subtype or if it's because the browser doesn't know the exact
subtype.

https://codereview.chromium.org/1306653003/diff/200001/net/base/network_chang...
File net/base/network_change_notifier.cc (right):

https://codereview.chromium.org/1306653003/diff/200001/net/base/network_chang...
net/base/network_change_notifier.cc:552: 
On 2015/09/15 18:17:40, pauljensen wrote:
> so if !g_network_change_notifier we dereference it...  I think you need an
else

Meant to have an early return, good catch. Thanks.

https://codereview.chromium.org/1306653003/diff/200001/net/base/network_chang...
net/base/network_change_notifier.cc:803: double max_bandwidth) {
On 2015/09/15 18:17:40, pauljensen wrote:
> add _mbps suffix

Done.

https://codereview.chromium.org/1306653003/diff/200001/net/base/network_chang...
net/base/network_change_notifier.cc:804: if (g_network_change_notifier)
On 2015/09/15 18:17:40, pauljensen wrote:
> need curly braces when more than one line clause

Done.

https://codereview.chromium.org/1306653003/diff/200001/net/base/network_chang...
File net/base/network_change_notifier.h (right):

https://codereview.chromium.org/1306653003/diff/200001/net/base/network_chang...
net/base/network_change_notifier.h:192: // GetMaxBanwidth for what to expect for
the values of |max_bandwidth_mbps|.
On 2015/09/15 18:17:40, pauljensen wrote:
> GetMaxBandwidth->GetMaxBandwidthAndConnectionType

Done.

https://codereview.chromium.org/1306653003/diff/200001/net/base/network_chang...
net/base/network_change_notifier.h:238: // is a connection and 0.0 otherwise.
The circumstances in which a more
On 2015/09/15 18:17:40, pauljensen wrote:
> Um I think it does more than "Specifically, it returns +Infinity if there is a
> connection and 0.0 otherwise."

Done.

https://codereview.chromium.org/1306653003/diff/200001/net/base/network_chang...
net/base/network_change_notifier.h:239: // specific value is given are:  Android
cellular types, Android wifi types,
On 2015/09/15 18:17:40, pauljensen wrote:
> can we reword the "types" here to something like "when an Android device is
> connected to a cellular or WiFi network"

Done.

https://codereview.chromium.org/1306653003/diff/200001/net/base/network_chang...
net/base/network_change_notifier.h:239: // specific value is given are:  Android
cellular types, Android wifi types,
On 2015/09/15 18:17:40, pauljensen wrote:
> wifi->WiFi

Done.

https://codereview.chromium.org/1306653003/diff/200001/net/base/network_chang...
net/base/network_change_notifier.h:242: // |connection_type| is set to the
current ConnectionType.
On 2015/09/15 18:17:40, pauljensen wrote:
> ConnectionType->active default network's connection type

Done.

https://codereview.chromium.org/1306653003/diff/200001/net/base/network_chang...
File net/base/network_change_notifier_linux.cc (right):

https://codereview.chromium.org/1306653003/diff/200001/net/base/network_chang...
net/base/network_change_notifier_linux.cc:90: SUBTYPE_UNKNOWN);
On 2015/09/15 18:17:40, pauljensen wrote:
> how about ReallyLongFunctionName(conditon ? SUBTYPE_NONE : SUBTYPE_UNKNOWN)?
> rather than duplicating ReallyLongFunctionName

Good idea. Done.

https://codereview.chromium.org/1306653003/diff/200001/net/base/network_chang...
File net/base/network_change_notifier_mac.cc (right):

https://codereview.chromium.org/1306653003/diff/200001/net/base/network_chang...
net/base/network_change_notifier_mac.cc:264: ?
NetworkChangeNotifier::GetMaxBandwidthForConnectionSubtype(
On 2015/09/15 18:17:40, pauljensen wrote:
> ditto

Done.

[jkarlin, 2015-09-17 14:07:11]

Paul: PTAL, this is the last one needed to get downlinkMax working :)

[pauljensen, 2015-09-18 13:56:09]

lgtm

[jkarlin, 2015-09-18 14:05:51]

The CQ bit was checked by jkarlin@chromium.org

[jkarlin, 2015-09-18 14:05:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
pneubeck@chromium.org
Link to the patchset: https://codereview.chromium.org/1306653003/#ps260001
(title: "Remove extra space in comment")

[commit-bot: I haz the power, 2015-09-18 14:06:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1306653003/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1306653003/260001

[commit-bot: I haz the power, 2015-09-18 15:14:14]

Committed patchset #13 (id:260001)

[commit-bot: I haz the power, 2015-09-18 15:14:58]

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