Non-functional simplification of NetworkChangeNotifierAutoDetect code

Non-functional simplification of NetworkChangeNotifierAutoDetect code

Turns out NetworkState was only needed for the current default network,
so simplify unnecessary pieces.  This is in preparation for adding
WiFi SSID to NetworkState, so I wanted to reduce the dependence on
NetworkState so the code wouldn't be required to fetch the Wifi
SSID as much.

BUG=647691
R=xunjieli

Committed: https://crrev.com/0e30b8a04dde1f68f8a03f915bf448e65bbd7a19
Cr-Commit-Position: refs/heads/master@{#420653}

[pauljensen, 2016-09-22 15:16:55]

Helen, PTAL, thank you!  This is the first in the breakup of the bigger NCN
speed-up CL.

[xunjieli, 2016-09-22 17:44:08]

Lgtm mod one comment below.

https://codereview.chromium.org/2361563003/diff/1/net/android/java/src/org/ch...
File net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java
(right):

https://codereview.chromium.org/2361563003/diff/1/net/android/java/src/org/ch...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:136:
return getConnectionType(networkInfo);
Can we inline the private getConnectionType() ? It doesn't add extra lines.
Having similarly named methods requires more mental effort from the reader of
code :\

int getConnectionType(Network network) {
   NetworkInfo networkInfo = getNetworkInfo(network);
   if (networkInfo != null && networkInfo.getType() == TYPE_VPN) {
      networkInfo = mConnectivityManager.getActiveNetworkInfo();
   }
   if (networkInfo != null && networkInfo.isConnected()) {
      return networkInfo.getType();
   }
   return  -1;
}

[pauljensen, 2016-09-22 18:27:59]

https://codereview.chromium.org/2361563003/diff/1/net/android/java/src/org/ch...
File net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java
(right):

https://codereview.chromium.org/2361563003/diff/1/net/android/java/src/org/ch...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:136:
return getConnectionType(networkInfo);
On 2016/09/22 17:44:08, xunjieli wrote:
> Can we inline the private getConnectionType() ? It doesn't add extra lines.
> Having similarly named methods requires more mental effort from the reader of
> code :\
> 
> int getConnectionType(Network network) {
>    NetworkInfo networkInfo = getNetworkInfo(network);
>    if (networkInfo != null && networkInfo.getType() == TYPE_VPN) {
>       networkInfo = mConnectivityManager.getActiveNetworkInfo();
>    }
>    if (networkInfo != null && networkInfo.isConnected()) {
>       return networkInfo.getType();
>    }
>    return  -1;
> }

Done.

[xunjieli, 2016-09-22 18:44:47]

lgtm

[pauljensen, 2016-09-23 02:08:01]

The CQ bit was checked by pauljensen@chromium.org

[commit-bot: I haz the power, 2016-09-23 02:08:26]

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

[pauljensen, 2016-09-23 02:51:52]

The CQ bit was unchecked by pauljensen@chromium.org

[pauljensen, 2016-09-23 13:13:49]

Helen, turns out getConnectionType() needs to return a ConnectionType, not a
ConnectivityManager connection type value.  I updated the code to correct this,
and added some @IntDef markers to avoid this in the future.  PTAL, thank you!
(might only be worth looking at diffs between patch sets 2 and 3).

[xunjieli, 2016-09-23 14:03:42]

The different types are indeed very tricky!

https://codereview.chromium.org/2361563003/diff/40001/net/android/java/src/or...
File net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java
(right):

https://codereview.chromium.org/2361563003/diff/40001/net/android/java/src/or...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:454:
final int newConnectionType =
getCurrentConnectionType(getCurrentNetworkState());
Can we use NetworkInfo directly to avoid constructing a NetworkState?
We might need to add a method to the ConnectivityManagerDelegate to
getNetworkInfo().

If we query directly from NetworkInfo, we  can remove
getCurrentConnectionType(NetworkState networkState) and only have one
getCurrentConnectionType(NetworkInfo networkInfo) method.  Seems slightly
cleaner that way .

https://codereview.chromium.org/2361563003/diff/40001/net/android/java/src/or...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:766:
private static int getCurrentConnectionType(int type, int subtype) {
The "Current" part in the method name makes me think that this is a member
method. How about renaming this method?

something like:
convertToConnectionType() ?

Fine with other names that do not have the word, "current."

[pauljensen, 2016-09-23 14:53:21]

https://codereview.chromium.org/2361563003/diff/40001/net/android/java/src/or...
File net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java
(right):

https://codereview.chromium.org/2361563003/diff/40001/net/android/java/src/or...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:454:
final int newConnectionType =
getCurrentConnectionType(getCurrentNetworkState());
On 2016/09/23 14:03:42, xunjieli wrote:
> Can we use NetworkInfo directly to avoid constructing a NetworkState?
> We might need to add a method to the ConnectivityManagerDelegate to
> getNetworkInfo().
> 
> If we query directly from NetworkInfo, we  can remove
> getCurrentConnectionType(NetworkState networkState) and only have one
> getCurrentConnectionType(NetworkInfo networkInfo) method.  Seems slightly
> cleaner that way . 

That won't work with the dependent CLs where I add WiFi SSID to the
NetworkState.  We'd have to pass around NetworkInfo and WiFi SSID in several
places, like when I initially proposed passing around NetworkState and WifiInfo,
but which we agreed was cleaner to pass around the SSID in the NetworkState.
This would also make testing harder because NetworkInfo has no visible
constructors, though I suppose some reflection could work around this.

I think the cost of constructing a NetworkState class should be trivial as it's
tiny.

https://codereview.chromium.org/2361563003/diff/40001/net/android/java/src/or...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:766:
private static int getCurrentConnectionType(int type, int subtype) {
On 2016/09/23 14:03:42, xunjieli wrote:
> The "Current" part in the method name makes me think that this is a member
> method. How about renaming this method?
> 
> something like:
> convertToConnectionType() ?
> 
> Fine with other names that do not have the word, "current."

Done.

[xunjieli, 2016-09-23 14:55:12]

lgtm

https://codereview.chromium.org/2361563003/diff/40001/net/android/java/src/or...
File net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java
(right):

https://codereview.chromium.org/2361563003/diff/40001/net/android/java/src/or...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:454:
final int newConnectionType =
getCurrentConnectionType(getCurrentNetworkState());
On 2016/09/23 14:53:21, pauljensen wrote:
> On 2016/09/23 14:03:42, xunjieli wrote:
> > Can we use NetworkInfo directly to avoid constructing a NetworkState?
> > We might need to add a method to the ConnectivityManagerDelegate to
> > getNetworkInfo().
> > 
> > If we query directly from NetworkInfo, we  can remove
> > getCurrentConnectionType(NetworkState networkState) and only have one
> > getCurrentConnectionType(NetworkInfo networkInfo) method.  Seems slightly
> > cleaner that way . 
> 
> That won't work with the dependent CLs where I add WiFi SSID to the
> NetworkState.  We'd have to pass around NetworkInfo and WiFi SSID in several
> places, like when I initially proposed passing around NetworkState and
WifiInfo,
> but which we agreed was cleaner to pass around the SSID in the NetworkState.
> This would also make testing harder because NetworkInfo has no visible
> constructors, though I suppose some reflection could work around this.
> 
> I think the cost of constructing a NetworkState class should be trivial as
it's
> tiny.

Acknowledged.

[pauljensen, 2016-09-23 15:02:18]

The CQ bit was checked by pauljensen@chromium.org

[commit-bot: I haz the power, 2016-09-23 15:02:28]

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

[commit-bot: I haz the power, 2016-09-23 15:20:41]

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

[commit-bot: I haz the power, 2016-09-23 15:20:42]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[pauljensen, 2016-09-23 16:17:10]

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

[pauljensen, 2016-09-23 16:17:13]

Josh, PTAL @ BackgroundSyncNetworkObserver.java

[jkarlin, 2016-09-23 16:28:18]

BackgroundSyncNetworkObserver.java lgtm

[pauljensen, 2016-09-23 16:29:48]

The CQ bit was checked by pauljensen@chromium.org

[pauljensen, 2016-09-23 16:29:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
xunjieli@chromium.org
Link to the patchset: https://codereview.chromium.org/2361563003/#ps80001
(title: "fix additional reference")

[commit-bot: I haz the power, 2016-09-23 16:30:16]

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

[commit-bot: I haz the power, 2016-09-23 17:51:03]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-23 17:54:32]

Description was changed from

==========
Non-functional simplification of NetworkChangeNotifierAutoDetect code

Turns out NetworkState was only needed for the current default network,
so simplify unnecessary pieces.  This is in preparation for adding
WiFi SSID to NetworkState, so I wanted to reduce the dependence on
NetworkState so the code wouldn't be required to fetch the Wifi
SSID as much.

BUG=647691
R=xunjieli
==========

to

==========
Non-functional simplification of NetworkChangeNotifierAutoDetect code

Turns out NetworkState was only needed for the current default network,
so simplify unnecessary pieces.  This is in preparation for adding
WiFi SSID to NetworkState, so I wanted to reduce the dependence on
NetworkState so the code wouldn't be required to fetch the Wifi
SSID as much.

BUG=647691
R=xunjieli

Committed: https://crrev.com/0e30b8a04dde1f68f8a03f915bf448e65bbd7a19
Cr-Commit-Position: refs/heads/master@{#420653}
==========

[commit-bot: I haz the power, 2016-09-23 17:54:33]

Patchset 5 (id:??) landed as
https://crrev.com/0e30b8a04dde1f68f8a03f915bf448e65bbd7a19
Cr-Commit-Position: refs/heads/master@{#420653}