[NetInfo] Add MaxBandwidthChanged notification and implement on Android

[NetInfo] Add a MaxBandwidthChangeObserver and Android implementation

* Add a MaxBandwidthChangeObserver to NetworkChangeNotifier
* Android notifies of max bandwidth changes at two points:
 1. On connection type change (if the bandwidth changes)
 2. On wifi signal strength change

BUG=433370
Committed: https://crrev.com/7b25e6c3b62270e30e92ff19a5fa915d5b6213f9
Cr-Commit-Position: refs/heads/master@{#308647}

[jkarlin, 2014-12-05 17:49:03]

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

[jkarlin, 2014-12-05 17:49:04]

thanks!

[pauljensen, 2014-12-05 19:27:34]

https://codereview.chromium.org/780293003/diff/20001/net/android/java/src/org...
File net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java
(right):

https://codereview.chromium.org/780293003/diff/20001/net/android/java/src/org...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:384:
addAction(WifiManager.RSSI_CHANGED_ACTION);
We should not be registering for another broadcast when we don't have
ACCESS_WIFI_STATE permissions, otherwise we'll be needlessly waking up our app
every few seconds for no purpose.

[jkarlin, 2014-12-08 16:58:44]

https://codereview.chromium.org/780293003/diff/20001/net/android/java/src/org...
File net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java
(right):

https://codereview.chromium.org/780293003/diff/20001/net/android/java/src/org...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:384:
addAction(WifiManager.RSSI_CHANGED_ACTION);
On 2014/12/05 19:27:34, pauljensen wrote:
> We should not be registering for another broadcast when we don't have
> ACCESS_WIFI_STATE permissions, otherwise we'll be needlessly waking up our app
> every few seconds for no purpose.

Done.

[jkarlin, 2014-12-09 20:17:26]

On 2014/12/08 16:58:44, jkarlin wrote:
>
https://codereview.chromium.org/780293003/diff/20001/net/android/java/src/org...
> File
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java
> (right):
> 
>
https://codereview.chromium.org/780293003/diff/20001/net/android/java/src/org...
>
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:384:
> addAction(WifiManager.RSSI_CHANGED_ACTION);
> On 2014/12/05 19:27:34, pauljensen wrote:
> > We should not be registering for another broadcast when we don't have
> > ACCESS_WIFI_STATE permissions, otherwise we'll be needlessly waking up our
app
> > every few seconds for no purpose.
> 
> Done.

PTAL.

[pauljensen, 2014-12-09 20:29:00]

I'm a little overloaded as a deadline was moved to Thursday so it may take me a
couple days to review.

[pauljensen, 2014-12-11 16:13:57]

https://codereview.chromium.org/780293003/diff/60001/net/android/java/src/org...
File net/android/java/src/org/chromium/net/NetworkChangeNotifier.java (right):

https://codereview.chromium.org/780293003/diff/60001/net/android/java/src/org...
net/android/java/src/org/chromium/net/NetworkChangeNotifier.java:76: public
double getCurrentMaxBandwidth() {
Can we add the InMbps suffix to this method similar to its AutoDetect
counterpart?

https://codereview.chromium.org/780293003/diff/60001/net/android/java/src/org...
File net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java
(right):

https://codereview.chromium.org/780293003/diff/60001/net/android/java/src/org...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:275:
if (getCurrentConnectionType() == ConnectionType.CONNECTION_WIFI) {
There are several IPCs over to ConnectivityService in this file that I worry may
be quite slow.  I think we can eliminate most of them by simply making
maxBandwidthChanged(), connectionTypeChanged(), getCurrentMaxBandwidthInMbps(),
getCurrentConnectionSubtype() and getCurrentWifiSSID() all take a NetworkState
as their argument.  I think this may also make the behavior of this code less
wonky (for example if the call to getCurrentConnectionType() on line 275 returns
non-WiFi but then getCurrentConnectionSubtype() on line 283 is told we are on
WiFi you'll get SUBTYPE_UNKNOWN somewhat unfortunately) and less prone to races
(see why I added NetworkState in crrev.com/750563003).

https://codereview.chromium.org/780293003/diff/60001/net/android/java/src/org...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:371:
return;
This "return" should probably be on the previous line or should have curly
braces.  Either way it stands out as very inconsistent with your line 390.  Feel
free to move line 390 onto the previous line and remove the curly braces if you
like also.

https://codereview.chromium.org/780293003/diff/60001/net/android/network_chan...
File net/android/network_change_notifier_android.cc (right):

https://codereview.chromium.org/780293003/diff/60001/net/android/network_chan...
net/android/network_change_notifier_android.cc:100: static void
NotifyMaxBandwidthChangedObservers(double max_bandwidth_mbps) {
Can we remove this function?  I don't see a reason for it.  Can we just move the
contents into the one caller?  I think NotifyNetworkChangeNotifierObservers()
only exists because it's one caller isn't static so it cannot be merged into it.

https://codereview.chromium.org/780293003/diff/60001/net/android/network_chan...
File net/android/network_change_notifier_delegate_android.cc (right):

https://codereview.chromium.org/780293003/diff/60001/net/android/network_chan...
net/android/network_change_notifier_delegate_android.cc:113:
DCHECK(new_max_bandwidth != connection_max_bandwidth_);
I think this is missing the lock; shouldn't it call GetCurrentMaxBandwidth()?

https://codereview.chromium.org/780293003/diff/60001/net/base/network_change_...
File net/base/network_change_notifier.h (right):

https://codereview.chromium.org/780293003/diff/60001/net/base/network_change_...
net/base/network_change_notifier.h:177: class NET_EXPORT
MaxBandwidthChangeObserver {
Can we remove the "Change" from this name...and everything else?  Since
"MaxBandwidth" is exactly what is being "Observed" I don't think we need the
word "Change" in there.  "NetworkChangeObserver" is different because
"Oberserving" a "Network" is a very ambiguous action, whereas "Observing"
"MaxBandwidth" is not ambiguous.

[jkarlin, 2014-12-15 19:43:55]

Thanks Paul!

https://codereview.chromium.org/780293003/diff/60001/net/android/java/src/org...
File net/android/java/src/org/chromium/net/NetworkChangeNotifier.java (right):

https://codereview.chromium.org/780293003/diff/60001/net/android/java/src/org...
net/android/java/src/org/chromium/net/NetworkChangeNotifier.java:76: public
double getCurrentMaxBandwidth() {
On 2014/12/11 16:13:56, pauljensen wrote:
> Can we add the InMbps suffix to this method similar to its AutoDetect
> counterpart?

Done.

https://codereview.chromium.org/780293003/diff/60001/net/android/java/src/org...
File net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java
(right):

https://codereview.chromium.org/780293003/diff/60001/net/android/java/src/org...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:275:
if (getCurrentConnectionType() == ConnectionType.CONNECTION_WIFI) {
On 2014/12/11 16:13:56, pauljensen wrote:
> There are several IPCs over to ConnectivityService in this file that I worry
may
> be quite slow.  I think we can eliminate most of them by simply making
> maxBandwidthChanged(), connectionTypeChanged(),
getCurrentMaxBandwidthInMbps(),
> getCurrentConnectionSubtype() and getCurrentWifiSSID() all take a NetworkState
> as their argument.  I think this may also make the behavior of this code less
> wonky (for example if the call to getCurrentConnectionType() on line 275
returns
> non-WiFi but then getCurrentConnectionSubtype() on line 283 is told we are on
> WiFi you'll get SUBTYPE_UNKNOWN somewhat unfortunately) and less prone to
races
> (see why I added NetworkState in crrev.com/750563003).

Done.

https://codereview.chromium.org/780293003/diff/60001/net/android/java/src/org...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:371:
return;
On 2014/12/11 16:13:56, pauljensen wrote:
> This "return" should probably be on the previous line or should have curly
> braces.  Either way it stands out as very inconsistent with your line 390. 
Feel
> free to move line 390 onto the previous line and remove the curly braces if
you
> like also.

Done.

https://codereview.chromium.org/780293003/diff/60001/net/android/network_chan...
File net/android/network_change_notifier_android.cc (right):

https://codereview.chromium.org/780293003/diff/60001/net/android/network_chan...
net/android/network_change_notifier_android.cc:100: static void
NotifyMaxBandwidthChangedObservers(double max_bandwidth_mbps) {
On 2014/12/11 16:13:56, pauljensen wrote:
> Can we remove this function?  I don't see a reason for it.  Can we just move
the
> contents into the one caller?  I think NotifyNetworkChangeNotifierObservers()
> only exists because it's one caller isn't static so it cannot be merged into
it.

Done.

https://codereview.chromium.org/780293003/diff/60001/net/android/network_chan...
File net/android/network_change_notifier_delegate_android.cc (right):

https://codereview.chromium.org/780293003/diff/60001/net/android/network_chan...
net/android/network_change_notifier_delegate_android.cc:113:
DCHECK(new_max_bandwidth != connection_max_bandwidth_);
On 2014/12/11 16:13:57, pauljensen wrote:
> I think this is missing the lock; shouldn't it call GetCurrentMaxBandwidth()?

Done. Thanks!

https://codereview.chromium.org/780293003/diff/60001/net/base/network_change_...
File net/base/network_change_notifier.h (right):

https://codereview.chromium.org/780293003/diff/60001/net/base/network_change_...
net/base/network_change_notifier.h:177: class NET_EXPORT
MaxBandwidthChangeObserver {
On 2014/12/11 16:13:57, pauljensen wrote:
> Can we remove the "Change" from this name...and everything else?  Since
> "MaxBandwidth" is exactly what is being "Observed" I don't think we need the
> word "Change" in there.  "NetworkChangeObserver" is different because
> "Oberserving" a "Network" is a very ambiguous action, whereas "Observing"
> "MaxBandwidth" is not ambiguous.

I've renamed the observer, but not "NotifyMaxBandwidthChanged" or
"OnMaxBandwidthChanged".

[pauljensen, 2014-12-16 18:44:18]

lgtm

[jkarlin, 2014-12-16 18:49:43]

The CQ bit was checked by jkarlin@chromium.org

[commit-bot: I haz the power, 2014-12-16 18:50:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/780293003/100001

[commit-bot: I haz the power, 2014-12-16 20:00:17]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-12-16 20:00:53]

Patchset 6 (id:??) landed as
https://crrev.com/7b25e6c3b62270e30e92ff19a5fa915d5b6213f9
Cr-Commit-Position: refs/heads/master@{#308647}