Refactor NetworkChangeNotifierAndroid

Refactor NetworkChangeNotifierAndroid

Cleanup and simplify NCNAndroid in a few ways:
1. Move convertToConnectionType() and convertToConnectionSubtype() from NCNAutoDetect to NetworkState accessors as this is where they make the most sense.
2. Pass around connection type and subtype, instead of connection bandwidth which is just computed from the subtype.  This reduces the number of JNI calls to convert between the subtype and bandwidth.
3. Instead of keeping four different state variables in NCNAutoDetect, just keep one, the NetworkState, which contains the different state characteristics.
These cleanups have the convenient byproduct of avoiding all JNI calls from NCNAndroid until there are incoming JNI calls to register native observers; this ensures NCNAndroid initialization can be done without JNI, which facilitates crrev.com/2863093003.

Review-Url: https://codereview.chromium.org/2866253002
Cr-Commit-Position: refs/heads/master@{#471599}
Committed: https://chromium.googlesource.com/chromium/src/+/82afc22a4146e58caddfb33496c1ff3e15307854

[pauljensen, 2017-05-09 14:30:31]

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

[commit-bot: I haz the power, 2017-05-09 14:31:05]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-09 14:59:02]

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

[commit-bot: I haz the power, 2017-05-09 14:59:02]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[pauljensen, 2017-05-10 01:07:28]

Description was changed from

==========
Refactor NetworkChangeNotifierAndroid
==========

to

==========
Refactor NetworkChangeNotifierAndroid

Cleanup and simplify NCNAndroid in a few ways:
1. Move convertToConnectionType() and convertToConnectionSubtype() from
NCNAutoDetect to NetworkState accessors as this is where they make the most
sense.
2. Pass around connection type and subtype, instead of connection bandwidth
which is just computed from the subtype.  This reduces the number of JNI calls
to convert between the subtype and bandwidth.
3. Instead of keeping four different state variables in NCNAutoDetect, just keep
one, the NetworkState, which contains the different state characteristics.
These cleanups have the convenient byproduct of avoiding all JNI calls from
NCNAndroid until there are incoming JNI calls to register native observers; this
ensures NCNAndroid initialization can be done without JNI, which facilitates
crrev.com/2863093003.
==========

[pauljensen, 2017-05-10 01:10:03]

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

[pauljensen, 2017-05-10 01:10:04]

Helen, can you review this CL please?

[xunjieli, 2017-05-10 14:20:10]

nice clean. one question below.

https://codereview.chromium.org/2866253002/diff/160001/net/android/java/src/o...
File net/android/java/src/org/chromium/net/NetworkChangeNotifier.java (right):

https://codereview.chromium.org/2866253002/diff/160001/net/android/java/src/o...
net/android/java/src/org/chromium/net/NetworkChangeNotifier.java:43: private int
mMaxBandwidthConnectionSubType = ConnectionSubtype.SUBTYPE_UNKNOWN;
Can we have a comment here on how mCurrentConnectionType is different from
mMaxBandwidthConnectionType/SubType?

https://codereview.chromium.org/2866253002/diff/160001/net/android/java/src/o...
net/android/java/src/org/chromium/net/NetworkChangeNotifier.java:85: return
mMaxBandwidthConnectionSubType;
Should we rename the method or the variable?

https://codereview.chromium.org/2866253002/diff/160001/net/android/java/src/o...
net/android/java/src/org/chromium/net/NetworkChangeNotifier.java:212:
updateCurrentConnectionType(networkState.getConnectionType());
This method still can call into native code through
notifyObserversOfConnectionTypeChange, right?
Doesn't that depend on JNI registration and the native library being loaded?

In your second CL crrev.com/2863093003, registerToReceiveNotificationsAlways()
can be called before library is loaded.

[pauljensen, 2017-05-10 16:54:53]

I also deleted some more code in the latest patch set.

https://codereview.chromium.org/2866253002/diff/160001/net/android/java/src/o...
File net/android/java/src/org/chromium/net/NetworkChangeNotifier.java (right):

https://codereview.chromium.org/2866253002/diff/160001/net/android/java/src/o...
net/android/java/src/org/chromium/net/NetworkChangeNotifier.java:43: private int
mMaxBandwidthConnectionSubType = ConnectionSubtype.SUBTYPE_UNKNOWN;
On 2017/05/10 14:20:10, xunjieli wrote:
> Can we have a comment here on how mCurrentConnectionType is different from
> mMaxBandwidthConnectionType/SubType?

Done.

https://codereview.chromium.org/2866253002/diff/160001/net/android/java/src/o...
net/android/java/src/org/chromium/net/NetworkChangeNotifier.java:85: return
mMaxBandwidthConnectionSubType;
On 2017/05/10 14:20:10, xunjieli wrote:
> Should we rename the method or the variable?

I reverted this change.  I realized mMaxBandwidthConnectionSubType is probably
out of date as broadcast Intents are only really sent out when the connection
type changes, not when the connection subtype changes.

https://codereview.chromium.org/2866253002/diff/160001/net/android/java/src/o...
net/android/java/src/org/chromium/net/NetworkChangeNotifier.java:212:
updateCurrentConnectionType(networkState.getConnectionType());
On 2017/05/10 14:20:10, xunjieli wrote:
> This method still can call into native code through
> notifyObserversOfConnectionTypeChange, right?
> Doesn't that depend on JNI registration and the native library being loaded?
> 
> In your second CL crrev.com/2863093003, registerToReceiveNotificationsAlways()
> can be called before library is loaded.

All native methods are now notifying methods that are only called after a native
class has registered.  Native classes cannot register until after the native
library is fully registered.

Another way to look at it is, all native methods take a "nativePtr" which can
only come from native code, so there is no risk of calling these functions
before native code first calls this class.

[xunjieli, 2017-05-10 18:12:15]

If we have no user of onMaxBandwidthChanged() callback, can we remove it? 

This is a nice cleanup, but it's a bit odd that onMaxBandwidthChanged() now
reports the connection types instead of a double (i.e. the maxBandwidthMbps
value).

To replace onMaxBandwidthChanged(), we can have a onConnectionSubTypeChanged()
callback, so an observer has the choice to query for the maxBandwidthMbps if
they would like to do so. 

What do you think?

https://codereview.chromium.org/2866253002/diff/160001/net/android/java/src/o...
File net/android/java/src/org/chromium/net/NetworkChangeNotifier.java (right):

https://codereview.chromium.org/2866253002/diff/160001/net/android/java/src/o...
net/android/java/src/org/chromium/net/NetworkChangeNotifier.java:212:
updateCurrentConnectionType(networkState.getConnectionType());
On 2017/05/10 16:54:53, pauljensen wrote:
> On 2017/05/10 14:20:10, xunjieli wrote:
> > This method still can call into native code through
> > notifyObserversOfConnectionTypeChange, right?
> > Doesn't that depend on JNI registration and the native library being loaded?
> > 
> > In your second CL crrev.com/2863093003,
registerToReceiveNotificationsAlways()
> > can be called before library is loaded.
> 
> All native methods are now notifying methods that are only called after a
native
> class has registered.  Native classes cannot register until after the native
> library is fully registered.
> 
> Another way to look at it is, all native methods take a "nativePtr" which can
> only come from native code, so there is no risk of calling these functions
> before native code first calls this class.

Acknowledged. Ah, got it. If we ever break this contract, I guess the java tests
will catch it?

[pauljensen, 2017-05-10 18:46:43]

On 2017/05/10 18:12:15, xunjieli wrote:
> If we have no user of onMaxBandwidthChanged() callback, can we remove it?

There is a user:
https://cs.chromium.org/chromium/src/content/browser/net/browser_online_state...
It gets passed along to the NetInfo API AFAIK.

[xunjieli, 2017-05-10 19:16:33]

On 2017/05/10 18:46:43, pauljensen wrote:
> On 2017/05/10 18:12:15, xunjieli wrote:
> > If we have no user of onMaxBandwidthChanged() callback, can we remove it?
> 
> There is a user:
>
https://cs.chromium.org/chromium/src/content/browser/net/browser_online_state...
> It gets passed along to the NetInfo API AFAIK.

Thanks for the reference! I still find the new API
NCNAndroidAutoDetect.Observer#onMaxBandwidthChanged(@ConnectionType int
newConnectionType, int newConnectionSubtype) doesn't offer a java-side user what
they want (right now we have 0 java-side user, but we might new users in the
future if we keep the observer callback around). The previous method signature
(double maxBandwidthMbps) is more intuitive.
I am fine with either of the two following approaches.
(1). Rename NCNAndroidAutoDetect.Observer#onMaxBandwidthChanged() to
onConnectionSubtypeChanged(). A benefit of this is that we can also get rid of
mMaxBandwidthConnectionType. 
(2). Have a Java-side mapping for subtype to bandwidth speed, so we can keep the
original method signature of double maxBandwidthMbps. One downside is that we
have a duplicate mapping.

[pauljensen, 2017-05-11 13:02:17]

On 2017/05/10 19:16:33, xunjieli wrote:
> On 2017/05/10 18:46:43, pauljensen wrote:
> > On 2017/05/10 18:12:15, xunjieli wrote:
> > > If we have no user of onMaxBandwidthChanged() callback, can we remove it?
> > 
> > There is a user:
> >
>
https://cs.chromium.org/chromium/src/content/browser/net/browser_online_state...
> > It gets passed along to the NetInfo API AFAIK.
> 
> Thanks for the reference! I still find the new API
> NCNAndroidAutoDetect.Observer#onMaxBandwidthChanged(@ConnectionType int
> newConnectionType, int newConnectionSubtype) doesn't offer a java-side user
what
> they want (right now we have 0 java-side user, but we might new users in the
> future if we keep the observer callback around). The previous method signature
> (double maxBandwidthMbps) is more intuitive.
> I am fine with either of the two following approaches.
> (1). Rename NCNAndroidAutoDetect.Observer#onMaxBandwidthChanged() to
> onConnectionSubtypeChanged(). 

SGTM, done.

> A benefit of this is that we can also get rid of mMaxBandwidthConnectionType. 

Actually there is no reason for NetworkChangeNotifier.java to dedup a signal
that NetworkChangeNotifierAutoDetect also dedups, so mMaxBandwidth* can be
removed, done.

> (2). Have a Java-side mapping for subtype to bandwidth speed, so we can keep
the
> original method signature of double maxBandwidthMbps. One downside is that we
> have a duplicate mapping.

[xunjieli, 2017-05-11 13:16:37]

On 2017/05/11 13:02:17, pauljensen wrote:
> On 2017/05/10 19:16:33, xunjieli wrote:
> > On 2017/05/10 18:46:43, pauljensen wrote:
> > > On 2017/05/10 18:12:15, xunjieli wrote:
> > > > If we have no user of onMaxBandwidthChanged() callback, can we remove
it?
> > > 
> > > There is a user:
> > >
> >
>
https://cs.chromium.org/chromium/src/content/browser/net/browser_online_state...
> > > It gets passed along to the NetInfo API AFAIK.
> > 
> > Thanks for the reference! I still find the new API
> > NCNAndroidAutoDetect.Observer#onMaxBandwidthChanged(@ConnectionType int
> > newConnectionType, int newConnectionSubtype) doesn't offer a java-side user
> what
> > they want (right now we have 0 java-side user, but we might new users in the
> > future if we keep the observer callback around). The previous method
signature
> > (double maxBandwidthMbps) is more intuitive.
> > I am fine with either of the two following approaches.
> > (1). Rename NCNAndroidAutoDetect.Observer#onMaxBandwidthChanged() to
> > onConnectionSubtypeChanged(). 
> 
> SGTM, done.
> 
> > A benefit of this is that we can also get rid of
mMaxBandwidthConnectionType. 
> 
> Actually there is no reason for NetworkChangeNotifier.java to dedup a signal
> that NetworkChangeNotifierAutoDetect also dedups, so mMaxBandwidth* can be
> removed, done.
> 
> > (2). Have a Java-side mapping for subtype to bandwidth speed, so we can keep
> the
> > original method signature of double maxBandwidthMbps. One downside is that
we
> > have a duplicate mapping.

lgtm!

[pauljensen, 2017-05-11 16:08:11]

On 2017/05/10 18:12:15, xunjieli wrote:
> On 2017/05/10 16:54:53, pauljensen wrote:
> > On 2017/05/10 14:20:10, xunjieli wrote:
> > > This method still can call into native code through
> > > notifyObserversOfConnectionTypeChange, right?
> > > Doesn't that depend on JNI registration and the native library being
loaded?
> > > 
> > > In your second CL crrev.com/2863093003,
> registerToReceiveNotificationsAlways()
> > > can be called before library is loaded.
> > 
> > All native methods are now notifying methods that are only called after a
> native
> > class has registered.  Native classes cannot register until after the native
> > library is fully registered.
> > 
> > Another way to look at it is, all native methods take a "nativePtr" which
can
> > only come from native code, so there is no risk of calling these functions
> > before native code first calls this class.
> 
> Acknowledged. Ah, got it. If we ever break this contract, I guess the java
tests
> will catch it?

Our current tests don't catch this as they always load the native library in
setUp().  I added a test and verified it fails prior to this patch.  Can you
take a look at my little test:
net/android/javatests/src/org/chromium/net/NetworkChangeNotifierNoNativeTest.java

[xunjieli, 2017-05-11 16:17:51]

https://codereview.chromium.org/2866253002/diff/220001/net/android/javatests/...
File
net/android/javatests/src/org/chromium/net/NetworkChangeNotifierNoNativeTest.java
(right):

https://codereview.chromium.org/2866253002/diff/220001/net/android/javatests/...
net/android/javatests/src/org/chromium/net/NetworkChangeNotifierNoNativeTest.java:20:
@RunWith(BaseJUnit4ClassRunner.class)
Nice! LGTM. Thanks for adding the test.

[pauljensen, 2017-05-11 20:02:59]

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

[pauljensen, 2017-05-11 20:03:00]

Josh, PTAL @
content/public/android/java/src/org/chromium/content/browser/BackgroundSyncNetworkObserver.java
Min, PTAL @
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadManagerService.java

[qinmin, 2017-05-11 21:54:41]

lgtm

[jkarlin, 2017-05-12 15:19:51]

BackgroundSyncNetworkObserver.java lgtm

[pauljensen, 2017-05-13 17:25:52]

The CQ bit was checked by pauljensen@chromium.org

[commit-bot: I haz the power, 2017-05-13 17:26:03]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-13 18:23:33]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1494696352945380,
"parent_rev": "9d5bcf58d9932d151f12231a6544b67903780fed", "commit_rev":
"82afc22a4146e58caddfb33496c1ff3e15307854"}

[commit-bot: I haz the power, 2017-05-13 18:23:48]

Description was changed from

==========
Refactor NetworkChangeNotifierAndroid

Cleanup and simplify NCNAndroid in a few ways:
1. Move convertToConnectionType() and convertToConnectionSubtype() from
NCNAutoDetect to NetworkState accessors as this is where they make the most
sense.
2. Pass around connection type and subtype, instead of connection bandwidth
which is just computed from the subtype.  This reduces the number of JNI calls
to convert between the subtype and bandwidth.
3. Instead of keeping four different state variables in NCNAutoDetect, just keep
one, the NetworkState, which contains the different state characteristics.
These cleanups have the convenient byproduct of avoiding all JNI calls from
NCNAndroid until there are incoming JNI calls to register native observers; this
ensures NCNAndroid initialization can be done without JNI, which facilitates
crrev.com/2863093003.
==========

to

==========
Refactor NetworkChangeNotifierAndroid

Cleanup and simplify NCNAndroid in a few ways:
1. Move convertToConnectionType() and convertToConnectionSubtype() from
NCNAutoDetect to NetworkState accessors as this is where they make the most
sense.
2. Pass around connection type and subtype, instead of connection bandwidth
which is just computed from the subtype.  This reduces the number of JNI calls
to convert between the subtype and bandwidth.
3. Instead of keeping four different state variables in NCNAutoDetect, just keep
one, the NetworkState, which contains the different state characteristics.
These cleanups have the convenient byproduct of avoiding all JNI calls from
NCNAndroid until there are incoming JNI calls to register native observers; this
ensures NCNAndroid initialization can be done without JNI, which facilitates
crrev.com/2863093003.

Review-Url: https://codereview.chromium.org/2866253002
Cr-Commit-Position: refs/heads/master@{#471599}
Committed:
https://chromium.googlesource.com/chromium/src/+/82afc22a4146e58caddfb33496c1...
==========

[commit-bot: I haz the power, 2017-05-13 18:23:49]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/82afc22a4146e58caddfb33496c1...