NCN: Get the current network's info

NCN: Get the current network's info

Get the current network's info in NetworkChangeNotifier by calling
getActiveNetworkInfo and unblocking the network if possible.

BUG=677365
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2601893003
Cr-Commit-Position: refs/heads/master@{#442676}
Committed: https://chromium.googlesource.com/chromium/src/+/d7aa1b1772ad6c55b15706f65904875743ec689b

[tbansal1, 2016-12-28 23:55:31]

Patchset #1 (id:1) has been deleted

[tbansal1, 2016-12-29 01:33:05]

Patchset #1 (id:20001) has been deleted

[tbansal1, 2017-01-06 03:24:35]

Description was changed from

==========
NetworkChangeNotifier logging

BUG=
==========

to

==========
Get the current network's info from the list of all networks

BUG=677365
==========

[tbansal1, 2017-01-06 03:24:40]

Patchset #1 (id:40001) has been deleted

[tbansal1, 2017-01-06 03:26:19]

Description was changed from

==========
Get the current network's info from the list of all networks

BUG=677365
==========

to

==========
Get the current network's info from the list of all networks

Get the current network's info from the list of all networks if the
active network is marked as BLOCKED.

BUG=677365
==========

[tbansal1, 2017-01-06 03:26:47]

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

[tbansal1, 2017-01-06 03:26:48]

pauljensen: thoughts? Thanks for the suggestions on how to fix this.

[tbansal1, 2017-01-06 03:28:21]

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

https://codereview.chromium.org/2601893003/diff/60001/net/android/java/src/or...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:104:
@TargetApi(Build.VERSION_CODES.LOLLIPOP)
pauljensen@ qq for you: Should this function have TargetAPI annotation?

[tbansal1, 2017-01-06 04:41:18]

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

[commit-bot: I haz the power, 2017-01-06 04:41:26]

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-01-06 05:54:03]

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

[commit-bot: I haz the power, 2017-01-06 05:54:04]

Dry run: This issue passed the CQ dry run.

[pauljensen, 2017-01-06 12:58:00]

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

https://codereview.chromium.org/2601893003/diff/60001/net/android/java/src/or...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:104:
@TargetApi(Build.VERSION_CODES.LOLLIPOP)
On 2017/01/06 03:28:21, tbansal1 wrote:
> pauljensen@ qq for you: Should this function have TargetAPI annotation?

Yes, it calls methods only available in Lollipop but our app is targeted at
JellyBean and newer.

https://codereview.chromium.org/2601893003/diff/60001/net/android/java/src/or...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:127:
if (activeNetworkDetailedState == null
detailed state is never null

https://codereview.chromium.org/2601893003/diff/60001/net/android/java/src/or...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:132:
}
Move this check up as it's fast.

https://codereview.chromium.org/2601893003/diff/60001/net/android/java/src/or...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:134:
if (Build.VERSION.SDK_INT < Build.VERSION_CODES.LOLLIPOP) {
Check M not LOLLIPOP, as this race involves doze mode which is only in M.

https://codereview.chromium.org/2601893003/diff/60001/net/android/java/src/or...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:139:
}
Move this check up (to just below the isConnected() check) as it's quick.

https://codereview.chromium.org/2601893003/diff/60001/net/android/java/src/or...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:146:
networkInfo = mConnectivityManager.getNetworkInfo(networks[0]);
This won't be accurate in cases where the network is in a state other than
CONNECTED.  Hmm.  I don't know of a way we can determine if the network is
connected or not...  I think most of the time networks are connected, so I
suppose we can just assume it's connected if we got a NetworkInfo for it. 
Actually I don't know if using the Network type gets us any more information. 
Perhaps we should just remove this getAllNetworksFiltered() code and use the
NetworkInfo that getActiveNetworkInfo gave us.

https://codereview.chromium.org/2601893003/diff/60001/net/android/java/src/or...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:160:
}
I don't think we need to check capabilities as getAllNetworksFiltered should
have done that for us.

[tbansal1, 2017-01-06 18:26:44]

Patchset #2 (id:80001) has been deleted

[tbansal1, 2017-01-06 18:29:22]

pauljensen: ptal. thanks.

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

https://codereview.chromium.org/2601893003/diff/60001/net/android/java/src/or...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:104:
@TargetApi(Build.VERSION_CODES.LOLLIPOP)
On 2017/01/06 12:58:00, pauljensen wrote:
> On 2017/01/06 03:28:21, tbansal1 wrote:
> > pauljensen@ qq for you: Should this function have TargetAPI annotation?
> 
> Yes, it calls methods only available in Lollipop but our app is targeted at
> JellyBean and newer.

Acknowledged.

https://codereview.chromium.org/2601893003/diff/60001/net/android/java/src/or...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:127:
if (activeNetworkDetailedState == null
On 2017/01/06 12:58:00, pauljensen wrote:
> detailed state is never null

Done.

https://codereview.chromium.org/2601893003/diff/60001/net/android/java/src/or...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:132:
}
On 2017/01/06 12:58:00, pauljensen wrote:
> Move this check up as it's fast.

Done.

https://codereview.chromium.org/2601893003/diff/60001/net/android/java/src/or...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:134:
if (Build.VERSION.SDK_INT < Build.VERSION_CODES.LOLLIPOP) {
On 2017/01/06 12:58:00, pauljensen wrote:
> Check M not LOLLIPOP, as this race involves doze mode which is only in M.

Done. I thought L has a user-setting that prevents background apps from
accessing the data
(http://teckfront.com/how-to-restrict-background-data-in-android-5-0-2-lollipo...),
so they probably have the same bug if the setting was enabled manually by the
user. But I can't seem to find that setting in the L devices that I have, so I
have changed it to M.

https://codereview.chromium.org/2601893003/diff/60001/net/android/java/src/or...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:139:
}
On 2017/01/06 12:58:00, pauljensen wrote:
> Move this check up (to just below the isConnected() check) as it's quick.

Done.

https://codereview.chromium.org/2601893003/diff/60001/net/android/java/src/or...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:146:
networkInfo = mConnectivityManager.getNetworkInfo(networks[0]);
On 2017/01/06 12:58:00, pauljensen wrote:
> This won't be accurate in cases where the network is in a state other than
> CONNECTED.  Hmm.  I don't know of a way we can determine if the network is
> connected or not...  I think most of the time networks are connected, so I
> suppose we can just assume it's connected if we got a NetworkInfo for it. 
If the network state is BLOCKED, I thought it means that network is available in
general, but it is not available to this specific app. If the network was
disconnected, I thought the state would be set to DISCONNECTED or CONNECTING?

Now, currently the only reason network may not be available to this  specific
app is because the app is in background, and background data has been restricted
(either automatically through doze mode, or via user configured setting). Since
we have checked that app is in foreground, so the network must be connected and
available.

Currently, Android does not provide a way (automatically or via user-settings)
to block network usage for a foreground app. In future, it is possible that this
might break. e.g., if new ways are introduced for blocking data access to a
specific app.
> Actually I don't know if using the Network type gets us any more information. 
> Perhaps we should just remove this getAllNetworksFiltered() code and use the
> NetworkInfo that getActiveNetworkInfo gave us.
It gives us at least one extra bit of information: We know that the network has
INTERNET capability.

https://codereview.chromium.org/2601893003/diff/60001/net/android/java/src/or...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:160:
}
On 2017/01/06 12:58:00, pauljensen wrote:
> I don't think we need to check capabilities as getAllNetworksFiltered should
> have done that for us.

Done. Replaced by assert.

[tbansal1, 2017-01-06 18:31:44]

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

[tbansal1, 2017-01-06 18:32:14]

Description was changed from

==========
Get the current network's info from the list of all networks

Get the current network's info from the list of all networks if the
active network is marked as BLOCKED.

BUG=677365
==========

to

==========
Get the current network's info from the list of all networks

Get the current network's info from the list of all networks if the
active network is marked as BLOCKED.

BUG=677365
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[tbansal1, 2017-01-06 18:32:19]

The CQ bit was unchecked by tbansal@chromium.org

[tbansal1, 2017-01-06 18:32:24]

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

[commit-bot: I haz the power, 2017-01-06 18:32:32]

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

[pauljensen, 2017-01-06 19:02:42]

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

https://codereview.chromium.org/2601893003/diff/60001/net/android/java/src/or...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:134:
if (Build.VERSION.SDK_INT < Build.VERSION_CODES.LOLLIPOP) {
On 2017/01/06 18:29:22, tbansal1 wrote:
> On 2017/01/06 12:58:00, pauljensen wrote:
> > Check M not LOLLIPOP, as this race involves doze mode which is only in M.
> 
> Done. I thought L has a user-setting that prevents background apps from
> accessing the data
>
(http://teckfront.com/how-to-restrict-background-data-in-android-5-0-2-lollipo...),
> so they probably have the same bug if the setting was enabled manually by the
> user. But I can't seem to find that setting in the L devices that I have, so I
> have changed it to M.

The UMA should tell us.  Does the UMA indicate it is specific to L & M or just
M?

https://codereview.chromium.org/2601893003/diff/60001/net/android/java/src/or...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:146:
networkInfo = mConnectivityManager.getNetworkInfo(networks[0]);
On 2017/01/06 18:29:22, tbansal1 wrote:
> On 2017/01/06 12:58:00, pauljensen wrote:
> > This won't be accurate in cases where the network is in a state other than
> > CONNECTED.  Hmm.  I don't know of a way we can determine if the network is
> > connected or not...  I think most of the time networks are connected, so I
> > suppose we can just assume it's connected if we got a NetworkInfo for it. 
> If the network state is BLOCKED, I thought it means that network is available
in
> general, but it is not available to this specific app. If the network was
> disconnected, I thought the state would be set to DISCONNECTED or CONNECTING?
No, BLOCKED just means the app isn't supposed to hear about networks.

> 
> Now, currently the only reason network may not be available to this  specific
> app is because the app is in background, and background data has been
restricted
> (either automatically through doze mode, or via user configured setting).
Since
> we have checked that app is in foreground, so the network must be connected
and
> available.
> 
> Currently, Android does not provide a way (automatically or via user-settings)
> to block network usage for a foreground app. In future, it is possible that
this
> might break. e.g., if new ways are introduced for blocking data access to a
> specific app.
> > Actually I don't know if using the Network type gets us any more
information. 
> > Perhaps we should just remove this getAllNetworksFiltered() code and use the
> > NetworkInfo that getActiveNetworkInfo gave us.
> It gives us at least one extra bit of information: We know that the network
has
> INTERNET capability.

The active network is always a data networks (i.e. has INTERNET capability).

https://codereview.chromium.org/2601893003/diff/100001/net/android/java/src/o...
File net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java
(right):

https://codereview.chromium.org/2601893003/diff/100001/net/android/java/src/o...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:149:
assert mConnectivityManager.getNetworkCapabilities(networks[0])
Let's not do an IPC just for the sake of an assert, please remove.

https://codereview.chromium.org/2601893003/diff/100001/net/android/java/src/o...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:153:
// networks[0] is BLOCKED which implies that it is connected but access to it is
BLOCKED doesn't mean it's connected, it could be in any of the other states.

[commit-bot: I haz the power, 2017-01-06 19:28:55]

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

[commit-bot: I haz the power, 2017-01-06 19:28:56]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2017-01-06 22:53:53]

ptal. Thanks.

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

https://codereview.chromium.org/2601893003/diff/60001/net/android/java/src/or...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:134:
if (Build.VERSION.SDK_INT < Build.VERSION_CODES.LOLLIPOP) {
On 2017/01/06 19:02:41, pauljensen wrote:
> On 2017/01/06 18:29:22, tbansal1 wrote:
> > On 2017/01/06 12:58:00, pauljensen wrote:
> > > Check M not LOLLIPOP, as this race involves doze mode which is only in M.
> > 
> > Done. I thought L has a user-setting that prevents background apps from
> > accessing the data
> >
>
(http://teckfront.com/how-to-restrict-background-data-in-android-5-0-2-lollipo...),
> > so they probably have the same bug if the setting was enabled manually by
the
> > user. But I can't seem to find that setting in the L devices that I have, so
I
> > have changed it to M.
> 
> The UMA should tell us.  Does the UMA indicate it is specific to L & M or just
> M?

UMA indicates jump in L too. But, I am not sure why.

https://codereview.chromium.org/2601893003/diff/60001/net/android/java/src/or...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:146:
networkInfo = mConnectivityManager.getNetworkInfo(networks[0]);
On 2017/01/06 19:02:41, pauljensen wrote:
> On 2017/01/06 18:29:22, tbansal1 wrote:
> > On 2017/01/06 12:58:00, pauljensen wrote:
> > > This won't be accurate in cases where the network is in a state other than
> > > CONNECTED.  Hmm.  I don't know of a way we can determine if the network is
> > > connected or not...  I think most of the time networks are connected, so I
> > > suppose we can just assume it's connected if we got a NetworkInfo for it. 
> > If the network state is BLOCKED, I thought it means that network is
available
> in
> > general, but it is not available to this specific app. If the network was
> > disconnected, I thought the state would be set to DISCONNECTED or
CONNECTING?
> No, BLOCKED just means the app isn't supposed to hear about networks.
> 
> > 
> > Now, currently the only reason network may not be available to this 
specific
> > app is because the app is in background, and background data has been
> restricted
> > (either automatically through doze mode, or via user configured setting).
> Since
> > we have checked that app is in foreground, so the network must be connected
> and
> > available.
> > 
> > Currently, Android does not provide a way (automatically or via
user-settings)
> > to block network usage for a foreground app. In future, it is possible that
> this
> > might break. e.g., if new ways are introduced for blocking data access to a
> > specific app.
> > > Actually I don't know if using the Network type gets us any more
> information. 
> > > Perhaps we should just remove this getAllNetworksFiltered() code and use
the
> > > NetworkInfo that getActiveNetworkInfo gave us.
> > It gives us at least one extra bit of information: We know that the network
> has
> > INTERNET capability.
> 
> The active network is always a data networks (i.e. has INTERNET capability).

Done.

https://codereview.chromium.org/2601893003/diff/100001/net/android/java/src/o...
File net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java
(right):

https://codereview.chromium.org/2601893003/diff/100001/net/android/java/src/o...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:149:
assert mConnectivityManager.getNetworkCapabilities(networks[0])
On 2017/01/06 19:02:42, pauljensen wrote:
> Let's not do an IPC just for the sake of an assert, please remove.

Done.

https://codereview.chromium.org/2601893003/diff/100001/net/android/java/src/o...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:153:
// networks[0] is BLOCKED which implies that it is connected but access to it is
On 2017/01/06 19:02:42, pauljensen wrote:
> BLOCKED doesn't mean it's connected, it could be in any of the other states.

Done.

[tbansal1, 2017-01-06 22:53:59]

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

[commit-bot: I haz the power, 2017-01-06 22:54:29]

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-01-06 23:46:44]

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

[commit-bot: I haz the power, 2017-01-06 23:46:46]

Dry run: This issue passed the CQ dry run.

[pauljensen, 2017-01-07 02:32:38]

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

https://codereview.chromium.org/2601893003/diff/60001/net/android/java/src/or...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:134:
if (Build.VERSION.SDK_INT < Build.VERSION_CODES.LOLLIPOP) {
On 2017/01/06 22:53:53, tbansal1 wrote:
> On 2017/01/06 19:02:41, pauljensen wrote:
> > On 2017/01/06 18:29:22, tbansal1 wrote:
> > > On 2017/01/06 12:58:00, pauljensen wrote:
> > > > Check M not LOLLIPOP, as this race involves doze mode which is only in
M.
> > > 
> > > Done. I thought L has a user-setting that prevents background apps from
> > > accessing the data
> > >
> >
>
(http://teckfront.com/how-to-restrict-background-data-in-android-5-0-2-lollipo...),
> > > so they probably have the same bug if the setting was enabled manually by
> the
> > > user. But I can't seem to find that setting in the L devices that I have,
so
> I
> > > have changed it to M.
> > 
> > The UMA should tell us.  Does the UMA indicate it is specific to L & M or
just
> > M?
> 
> UMA indicates jump in L too. But, I am not sure why.

Let's enable this fix for L also then.

https://codereview.chromium.org/2601893003/diff/120001/net/android/java/src/o...
File net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java
(right):

https://codereview.chromium.org/2601893003/diff/120001/net/android/java/src/o...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:115:
// Current active network is not connected. Check if the network connectivity is
blocked
Can we combine and simplify this comment and the one on line 137 into just one
comment like:
If |networkInfo| is BLOCKED, but the app is in the foreground, then it's likely
Android hasn't finished updating the network access permissions as BLOCKED is
only meant for apps in the background.  See https://crbug.com/677365 for more
details.

[tbansal1, 2017-01-09 17:54:59]

Patchset #4 (id:140001) has been deleted

[tbansal1, 2017-01-09 18:02:28]

Patchset #4 (id:160001) has been deleted

[tbansal1, 2017-01-09 18:04:19]

Patchset #4 (id:180001) has been deleted

[tbansal1, 2017-01-09 18:11:03]

Patchset #4 (id:200001) has been deleted

[tbansal1, 2017-01-09 18:11:14]

pauljensen: ptal. Thanks.

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

https://codereview.chromium.org/2601893003/diff/60001/net/android/java/src/or...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:134:
if (Build.VERSION.SDK_INT < Build.VERSION_CODES.LOLLIPOP) {
On 2017/01/07 02:32:38, pauljensen wrote:
> On 2017/01/06 22:53:53, tbansal1 wrote:
> > On 2017/01/06 19:02:41, pauljensen wrote:
> > > On 2017/01/06 18:29:22, tbansal1 wrote:
> > > > On 2017/01/06 12:58:00, pauljensen wrote:
> > > > > Check M not LOLLIPOP, as this race involves doze mode which is only in
> M.
> > > > 
> > > > Done. I thought L has a user-setting that prevents background apps from
> > > > accessing the data
> > > >
> > >
> >
>
(http://teckfront.com/how-to-restrict-background-data-in-android-5-0-2-lollipo...),
> > > > so they probably have the same bug if the setting was enabled manually
by
> > the
> > > > user. But I can't seem to find that setting in the L devices that I
have,
> so
> > I
> > > > have changed it to M.
> > > 
> > > The UMA should tell us.  Does the UMA indicate it is specific to L & M or
> just
> > > M?
> > 
> > UMA indicates jump in L too. But, I am not sure why.
> 
> Let's enable this fix for L also then.

Done.

https://codereview.chromium.org/2601893003/diff/120001/net/android/java/src/o...
File net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java
(right):

https://codereview.chromium.org/2601893003/diff/120001/net/android/java/src/o...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:115:
// Current active network is not connected. Check if the network connectivity is
blocked
On 2017/01/07 02:32:38, pauljensen wrote:
> Can we combine and simplify this comment and the one on line 137 into just one
> comment like:
> If |networkInfo| is BLOCKED, but the app is in the foreground, then it's
likely
> Android hasn't finished updating the network access permissions as BLOCKED is
> only meant for apps in the background.  See https://crbug.com/677365 for more
> details.

Done.

[tbansal1, 2017-01-09 18:43:38]

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

[commit-bot: I haz the power, 2017-01-09 18:44:25]

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

[pauljensen, 2017-01-09 19:08:18]

looks good, let's add some UMA and land

https://codereview.chromium.org/2601893003/diff/220001/net/android/java/src/o...
File net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java
(right):

https://codereview.chromium.org/2601893003/diff/220001/net/android/java/src/o...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:105:
NetworkInfo getNetworkInfo() {
Could we add some UMA to measure how well this is working?  Perhaps
RecordHistogram.recordEnumeratedHistogram() with an enum like:

DISCONNECTED (recorded on line 108)
CONNECTED (recorded on line 112)
TOO_OLD_FOR_UNBLOCKING (recorded on line 120)
NOT_BLOCKED (recorded on line 125)
NOT_FOREGROUND (recorded on line 132)
UNBLOCKED (recorded on line 134)

[commit-bot: I haz the power, 2017-01-09 19:27:41]

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

[commit-bot: I haz the power, 2017-01-09 19:27:42]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2017-01-09 20:03:01]

Description was changed from

==========
Get the current network's info from the list of all networks

Get the current network's info from the list of all networks if the
active network is marked as BLOCKED.

BUG=677365
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
NCN: Get the current network's info

Get the current network's info in NetworkChangeNotifier by calling
getActiveNetworkInfo and unblocking the network if possible.

BUG=677365
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[tbansal1, 2017-01-09 20:03:25]

Patchset #5 (id:240001) has been deleted

[tbansal1, 2017-01-09 20:04:22]

pauljensen: ptal. Thanks.

https://codereview.chromium.org/2601893003/diff/220001/net/android/java/src/o...
File net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java
(right):

https://codereview.chromium.org/2601893003/diff/220001/net/android/java/src/o...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:105:
NetworkInfo getNetworkInfo() {
On 2017/01/09 19:08:18, pauljensen wrote:
> Could we add some UMA to measure how well this is working?  Perhaps
> RecordHistogram.recordEnumeratedHistogram() with an enum like:
> 
> DISCONNECTED (recorded on line 108)
> CONNECTED (recorded on line 112)
> TOO_OLD_FOR_UNBLOCKING (recorded on line 120)
> NOT_BLOCKED (recorded on line 125)
> NOT_FOREGROUND (recorded on line 132)
> UNBLOCKED (recorded on line 134)

Done.

[tbansal1, 2017-01-09 21:42:36]

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

[commit-bot: I haz the power, 2017-01-09 21:43:16]

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-01-09 23:06:56]

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

[commit-bot: I haz the power, 2017-01-09 23:06:56]

Dry run: This issue passed the CQ dry run.

[pauljensen, 2017-01-10 13:01:02]

lgtm modulo comments

https://codereview.chromium.org/2601893003/diff/260001/net/android/java/src/o...
File net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java
(right):

https://codereview.chromium.org/2601893003/diff/260001/net/android/java/src/o...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:88:
private static final int NETWORK_INFO_DISCONNECTED = 0;
can we make this an actual enum? like so:
private enum NetworkInfoState {
 DISCONNECTED(0),
 CONNECTED(1),
 ANDROID_TOO_OLD_FOR_UNBLOCKING(2),
 NOT_BLOCKED(3),
 APP_BACKGROUNDED(4),
 UNBLOCKED(5),
 NUM_STATES(6);

 public final int value;

 private NetworkInfoState(int value) {
    this.value = value;
 }
}

https://codereview.chromium.org/2601893003/diff/260001/net/android/java/src/o...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:113:
NetworkInfo getActiveNetworkInfo() {
private

[tbansal1, 2017-01-10 14:11:01]

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

[commit-bot: I haz the power, 2017-01-10 14:11:19]

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

[tbansal1, 2017-01-10 14:12:24]

tbansal@chromium.org changed reviewers:
+ rkaplow@chromium.org

[tbansal1, 2017-01-10 14:12:25]

pauljensen: I have not made the suggested enum change.

rkaplow: ptal at histograms.xml.

https://codereview.chromium.org/2601893003/diff/260001/net/android/java/src/o...
File net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java
(right):

https://codereview.chromium.org/2601893003/diff/260001/net/android/java/src/o...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:88:
private static final int NETWORK_INFO_DISCONNECTED = 0;
On 2017/01/10 13:01:02, pauljensen wrote:
> can we make this an actual enum? like so:
> private enum NetworkInfoState {
>  DISCONNECTED(0),
>  CONNECTED(1),
>  ANDROID_TOO_OLD_FOR_UNBLOCKING(2),
>  NOT_BLOCKED(3),
>  APP_BACKGROUNDED(4),
>  UNBLOCKED(5),
>  NUM_STATES(6);
> 
>  public final int value;
> 
>  private NetworkInfoState(int value) {
>     this.value = value;
>  }
> }

NOT DONE.
I thought use of enums in Java was discouraged. http://shortn/_roLTlxhg7z

Let me know if that discussion is no longer valid.

https://codereview.chromium.org/2601893003/diff/260001/net/android/java/src/o...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:113:
NetworkInfo getActiveNetworkInfo() {
On 2017/01/10 13:01:02, pauljensen wrote:
> private

Done.

[pauljensen, 2017-01-10 14:14:43]

https://codereview.chromium.org/2601893003/diff/260001/net/android/java/src/o...
File net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java
(right):

https://codereview.chromium.org/2601893003/diff/260001/net/android/java/src/o...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:88:
private static final int NETWORK_INFO_DISCONNECTED = 0;
On 2017/01/10 14:12:24, tbansal1 wrote:
> On 2017/01/10 13:01:02, pauljensen wrote:
> > can we make this an actual enum? like so:
> > private enum NetworkInfoState {
> >  DISCONNECTED(0),
> >  CONNECTED(1),
> >  ANDROID_TOO_OLD_FOR_UNBLOCKING(2),
> >  NOT_BLOCKED(3),
> >  APP_BACKGROUNDED(4),
> >  UNBLOCKED(5),
> >  NUM_STATES(6);
> > 
> >  public final int value;
> > 
> >  private NetworkInfoState(int value) {
> >     this.value = value;
> >  }
> > }
> 
> NOT DONE.
> I thought use of enums in Java was discouraged. http://shortn/_roLTlxhg7z
> 
> Let me know if that discussion is no longer valid.

I disagree with the ban on enums but I'm fine leaving it the way it is.

[commit-bot: I haz the power, 2017-01-10 16:37:30]

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

[commit-bot: I haz the power, 2017-01-10 16:37:32]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2017-01-10 17:18:05]

https://codereview.chromium.org/2601893003/diff/260001/net/android/java/src/o...
File net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java
(right):

https://codereview.chromium.org/2601893003/diff/260001/net/android/java/src/o...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:88:
private static final int NETWORK_INFO_DISCONNECTED = 0;
On 2017/01/10 14:14:43, pauljensen wrote:
> On 2017/01/10 14:12:24, tbansal1 wrote:
> > On 2017/01/10 13:01:02, pauljensen wrote:
> > > can we make this an actual enum? like so:
> > > private enum NetworkInfoState {
> > >  DISCONNECTED(0),
> > >  CONNECTED(1),
> > >  ANDROID_TOO_OLD_FOR_UNBLOCKING(2),
> > >  NOT_BLOCKED(3),
> > >  APP_BACKGROUNDED(4),
> > >  UNBLOCKED(5),
> > >  NUM_STATES(6);
> > > 
> > >  public final int value;
> > > 
> > >  private NetworkInfoState(int value) {
> > >     this.value = value;
> > >  }
> > > }
> > 
> > NOT DONE.
> > I thought use of enums in Java was discouraged. http://shortn/_roLTlxhg7z
> > 
> > Let me know if that discussion is no longer valid.
> 
> I disagree with the ban on enums but I'm fine leaving it the way it is.

Acknowledged.

[rkaplow, 2017-01-10 18:19:22]

lgtm

https://codereview.chromium.org/2601893003/diff/280001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2601893003/diff/280001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:29550: +<histogram
name="NCN.getActiveNetworkInfoResult"
minor nit, Get should be capitalized (reminder to change the code as well)

[tbansal1, 2017-01-10 18:32:12]

https://codereview.chromium.org/2601893003/diff/280001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2601893003/diff/280001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:29550: +<histogram
name="NCN.getActiveNetworkInfoResult"
On 2017/01/10 18:19:22, rkaplow wrote:
> minor nit, Get should be capitalized (reminder to change the code as well)

Done. I had used lower-case letter to be consistent with other histograms in
this file
(https://cs.chromium.org/search/?q=f:network.*change+recordhistogram&sq=packag...).
I still think it is better for new histograms to use upper case letter.

[tbansal1, 2017-01-10 18:32:16]

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

[commit-bot: I haz the power, 2017-01-10 18:32:53]

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-01-10 20:04:30]

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

[commit-bot: I haz the power, 2017-01-10 20:04:31]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2017-01-10 20:29:27]

The CQ bit was checked by tbansal@chromium.org

[tbansal1, 2017-01-10 20:29:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
pauljensen@chromium.org, rkaplow@chromium.org
Link to the patchset: https://codereview.chromium.org/2601893003/#ps300001
(title: "rkaplow nit")

[commit-bot: I haz the power, 2017-01-10 20:30:20]

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-01-10 20:37:10]

CQ is committing da patch.

Bot data: {"patchset_id": 300001, "attempt_start_ts": 1484080167717690,
"parent_rev": "ec78ec0763a819fc5d5f346bef3538ff090368a5", "commit_rev":
"d7aa1b1772ad6c55b15706f65904875743ec689b"}

[commit-bot: I haz the power, 2017-01-10 20:37:47]

Description was changed from

==========
NCN: Get the current network's info

Get the current network's info in NetworkChangeNotifier by calling
getActiveNetworkInfo and unblocking the network if possible.

BUG=677365
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
NCN: Get the current network's info

Get the current network's info in NetworkChangeNotifier by calling
getActiveNetworkInfo and unblocking the network if possible.

BUG=677365
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2601893003
Cr-Commit-Position: refs/heads/master@{#442676}
Committed:
https://chromium.googlesource.com/chromium/src/+/d7aa1b1772ad6c55b15706f65904...
==========

[commit-bot: I haz the power, 2017-01-10 20:37:54]

Committed patchset #7 (id:300001) as
https://chromium.googlesource.com/chromium/src/+/d7aa1b1772ad6c55b15706f65904...