Non-functional improvement to reduce querying of WiFi SSID upon startup

Non-functional improvement to reduce querying of WiFi SSID upon startup

Include WiFi SSID in NetworkState so it can be reused without re-querying.
Also improve how we fetch WiFi SSID if needed:
1. On Android 4.2+ devices, SSID can be retreived from NetworkInfo which
   NetworkChangeNotifierAutoDetect already queried
2. When an app has the ACCESS_WIFI_STATE permission, the SSID can be
   queried quickly directly from WifiManager.

BUG=647691
R=xunjieli

Committed: https://crrev.com/336a75e2356486da08bd04996affa8a43aa3056e
Cr-Commit-Position: refs/heads/master@{#422128}

[pauljensen, 2016-09-22 15:20:32]

Helen, PTAL, thank you!  This is the third in the breakup of the bigger NCN
speed-up CL. (The second I sent to Josh for review)

[xunjieli, 2016-09-22 21:06:34]

Looks great. Just a couple of nits below.

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

https://codereview.chromium.org/2360303002/diff/1/net/android/java/src/org/ch...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:50:
public NetworkState(boolean connected, int type, int subtype, String wifiSsid) {
nit: maybe add another constructor that doesn't have |wifiSsid| as an argument
and initializes it to "" there. 
Might be clearer this way than passing nulls at the call place.

https://codereview.chromium.org/2360303002/diff/1/net/android/java/src/org/ch...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:105:
} else {
Could you add a comment on the else case?

optional nit: get rid of } else { } and just do return.

https://codereview.chromium.org/2360303002/diff/1/net/android/java/src/org/ch...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:262:
private boolean mHasWifiPermission; // Only valid when
mHasWifiPermissionComputed is set
nit: Could you move comments above the variable? It's a bit long to read when
comment is on the same line as the variable.

https://codereview.chromium.org/2360303002/diff/1/net/android/java/src/org/ch...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:271:
private synchronized boolean havePermission() {
Could you add a doc why this is an synchronized method?
nit: maybe hasPermission() instead of havePermission()? since I am assuming
whoever has the permission is a single entity.

https://codereview.chromium.org/2360303002/diff/1/net/android/java/src/org/ch...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:285:
WifiManagerDelegate() {
nit: Could you group the constructors together? Easier to read that way.

https://codereview.chromium.org/2360303002/diff/1/net/android/java/src/org/ch...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:298:
}
optional nit: Maybe use an early return and return an empty string.
if (hasPermission()) {
  WifiInfo wifiInfo = getWifiInfo();
  if (wifiInfo) {
    return wifiInfo.getSSID();
  }
  return "";
}
return AndroidNetworkLibrary.getWifiSSID(mContext);

[pauljensen, 2016-09-23 14:12:18]

Thanks for the useful tweaks! PTAL.

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

https://codereview.chromium.org/2360303002/diff/1/net/android/java/src/org/ch...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:50:
public NetworkState(boolean connected, int type, int subtype, String wifiSsid) {
On 2016/09/22 21:06:34, xunjieli wrote:
> nit: maybe add another constructor that doesn't have |wifiSsid| as an argument
> and initializes it to "" there. 
> Might be clearer this way than passing nulls at the call place.

I'd rather not, as I really want to avoid callers creating NetworkState's for
WiFi but without SSID.

https://codereview.chromium.org/2360303002/diff/1/net/android/java/src/org/ch...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:105:
} else {
On 2016/09/22 21:06:34, xunjieli wrote:
> Could you add a comment on the else case?
> 
> optional nit: get rid of } else { } and just do return.

Done.

https://codereview.chromium.org/2360303002/diff/1/net/android/java/src/org/ch...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:262:
private boolean mHasWifiPermission; // Only valid when
mHasWifiPermissionComputed is set
On 2016/09/22 21:06:34, xunjieli wrote:
> nit: Could you move comments above the variable? It's a bit long to read when
> comment is on the same line as the variable.

Done.

https://codereview.chromium.org/2360303002/diff/1/net/android/java/src/org/ch...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:271:
private synchronized boolean havePermission() {
On 2016/09/22 21:06:34, xunjieli wrote:
> Could you add a doc why this is an synchronized method?
> nit: maybe hasPermission() instead of havePermission()? since I am assuming
> whoever has the permission is a single entity.

Done.

https://codereview.chromium.org/2360303002/diff/1/net/android/java/src/org/ch...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:285:
WifiManagerDelegate() {
On 2016/09/22 21:06:34, xunjieli wrote:
> nit: Could you group the constructors together? Easier to read that way.

Done.

https://codereview.chromium.org/2360303002/diff/1/net/android/java/src/org/ch...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:298:
}
On 2016/09/22 21:06:34, xunjieli wrote:
> optional nit: Maybe use an early return and return an empty string.
> if (hasPermission()) {
>   WifiInfo wifiInfo = getWifiInfo();
>   if (wifiInfo) {
>     return wifiInfo.getSSID();
>   }
>   return "";
> }
> return AndroidNetworkLibrary.getWifiSSID(mContext);

Done.

[xunjieli, 2016-09-23 18:23:11]

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

https://codereview.chromium.org/2360303002/diff/20001/net/android/java/src/or...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:48:
private final String mWifiSsid; // always non-null to facilitate .equals()
optional nit: maybe consider moving comment above the variable.
Something like:
WIFI SSID of the connection. If the connection is not WIFI, this is an empty
string to facilitate .equals().

https://codereview.chromium.org/2360303002/diff/20001/net/android/java/src/or...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:50:
public NetworkState(boolean connected, int type, int subtype, String wifiSsid) {
Maybe add an assert to check wifiSsid is only non-null when type == WIFI?
If I understand correctly from your last comment, wifiSsid *must* be non-null
when type == WIFI, can we also check that?

The assertions do not do much but it makes the assumptions/invariants clearer.
In case someone break it, they will know right away.

https://codereview.chromium.org/2360303002/diff/20001/net/android/java/src/or...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:269:
mHasWifiPermissionComputed = false;
nit: either leave out this initialization or also initialize the other boolean
(mHasWifiPermssion).

https://codereview.chromium.org/2360303002/diff/20001/net/android/java/src/or...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:279:
// Synchronized because it is called on multiple threads and is otherwise racy.
What are these "multiple threads"? UI thread and the network thread ? or network
threads?
Could you clarify a bit in the comment?
I know that there are probably multiple threads involved as you choose to use
"synchronized." but without digging into the code, I am as confused as I was
before reading the doc.

[pauljensen, 2016-09-26 15:12:22]

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

https://codereview.chromium.org/2360303002/diff/20001/net/android/java/src/or...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:48:
private final String mWifiSsid; // always non-null to facilitate .equals()
On 2016/09/23 18:23:11, xunjieli wrote:
> optional nit: maybe consider moving comment above the variable.
> Something like:
> WIFI SSID of the connection. If the connection is not WIFI, this is an empty
> string to facilitate .equals().

Done, though reworded to take into account that in weird cases (like races with
WiFi disconnecting) it could be an empty string even if on WiFi.

https://codereview.chromium.org/2360303002/diff/20001/net/android/java/src/or...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:50:
public NetworkState(boolean connected, int type, int subtype, String wifiSsid) {
On 2016/09/23 18:23:11, xunjieli wrote:
> Maybe add an assert to check wifiSsid is only non-null when type == WIFI?

Done.

> If I understand correctly from your last comment, wifiSsid *must* be non-null
> when type == WIFI, can we also check that?

In odd cases, like when WiFi is in the process of disconnecting, wifiSsid may be
null or empty so I cannot add an assert for that.

https://codereview.chromium.org/2360303002/diff/20001/net/android/java/src/or...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:269:
mHasWifiPermissionComputed = false;
On 2016/09/23 18:23:11, xunjieli wrote:
> nit: either leave out this initialization or also initialize the other boolean
> (mHasWifiPermssion).

Done.

https://codereview.chromium.org/2360303002/diff/20001/net/android/java/src/or...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:279:
// Synchronized because it is called on multiple threads and is otherwise racy.
On 2016/09/23 18:23:11, xunjieli wrote:
> What are these "multiple threads"? UI thread and the network thread ? or
network
> threads?
> Could you clarify a bit in the comment?
> I know that there are probably multiple threads involved as you choose to use
> "synchronized." but without digging into the code, I am as confused as I was
> before reading the doc.

Done.

[xunjieli, 2016-09-26 16:30:04]

lgtm

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

https://codereview.chromium.org/2360303002/diff/20001/net/android/java/src/or...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:50:
public NetworkState(boolean connected, int type, int subtype, String wifiSsid) {
On 2016/09/26 15:12:21, pauljensen wrote:
> On 2016/09/23 18:23:11, xunjieli wrote:
> > Maybe add an assert to check wifiSsid is only non-null when type == WIFI?
> 
> Done.
> 
> > If I understand correctly from your last comment, wifiSsid *must* be
non-null
> > when type == WIFI, can we also check that?
> 
> In odd cases, like when WiFi is in the process of disconnecting, wifiSsid may
be
> null or empty so I cannot add an assert for that.

Acknowledged.

https://codereview.chromium.org/2360303002/diff/60001/net/android/javatests/s...
File net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java
(right):

https://codereview.chromium.org/2360303002/diff/60001/net/android/javatests/s...
net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java:157:
return new NetworkState(mActiveNetworkExists, mNetworkType, mNetworkSubtype,
null);
null -> wifiManagerDelegate.getWifiSsid() ?

[pauljensen, 2016-09-26 18:35:38]

https://codereview.chromium.org/2360303002/diff/60001/net/android/javatests/s...
File net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java
(right):

https://codereview.chromium.org/2360303002/diff/60001/net/android/javatests/s...
net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java:157:
return new NetworkState(mActiveNetworkExists, mNetworkType, mNetworkSubtype,
null);
On 2016/09/26 16:30:04, xunjieli wrote:
> null -> wifiManagerDelegate.getWifiSsid() ?

Done, when mNetworkType is TYPE_WIFI.

[xunjieli, 2016-09-26 18:56:56]

lgtm

[pauljensen, 2016-09-26 19:42:49]

The CQ bit was checked by pauljensen@chromium.org

[commit-bot: I haz the power, 2016-09-26 19:43:30]

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-26 20:41:27]

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

[commit-bot: I haz the power, 2016-09-26 20:41:28]

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

[pauljensen, 2016-09-27 11:24:55]

The CQ bit was checked by pauljensen@chromium.org

[pauljensen, 2016-09-27 11:24:55]

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/2360303002/#ps120001
(title: "fix clang warning")

[commit-bot: I haz the power, 2016-09-27 11:25:14]

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-27 12:23:54]

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

[commit-bot: I haz the power, 2016-09-27 12:23:56]

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

[xunjieli, 2016-09-27 17:08:53]

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

https://codereview.chromium.org/2360303002/diff/120001/net/android/java/src/o...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:262:
// Lock all members below.
"all members" also include |mWifiManager| ?

https://codereview.chromium.org/2360303002/diff/120001/net/android/java/src/o...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:265:
private boolean mHasWifiPermissionComputed;
Maybe add @GuardedBy("mLock") to these two booleans.

https://codereview.chromium.org/2360303002/diff/120001/net/android/java/src/o...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:282:
private synchronized boolean hasPermission() {
"synchronized" not needed any more.

[pauljensen, 2016-09-30 02:53:28]

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

https://codereview.chromium.org/2360303002/diff/120001/net/android/java/src/o...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:262:
// Lock all members below.
On 2016/09/27 17:08:53, xunjieli wrote:
> "all members" also include |mWifiManager| ?

Yes, I reworked it to enforce this.

https://codereview.chromium.org/2360303002/diff/120001/net/android/java/src/o...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:265:
private boolean mHasWifiPermissionComputed;
On 2016/09/27 17:08:53, xunjieli wrote:
> Maybe add @GuardedBy("mLock") to these two booleans.

Done.

https://codereview.chromium.org/2360303002/diff/120001/net/android/java/src/o...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:282:
private synchronized boolean hasPermission() {
On 2016/09/27 17:08:53, xunjieli wrote:
> "synchronized" not needed any more.

Done.

[xunjieli, 2016-09-30 13:07:30]

lgtm

[pauljensen, 2016-09-30 14:27:56]

The CQ bit was checked by pauljensen@chromium.org

[commit-bot: I haz the power, 2016-09-30 14:28: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-30 15:49:36]

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

[commit-bot: I haz the power, 2016-09-30 15:49:37]

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

[pauljensen, 2016-09-30 15:57:32]

The CQ bit was checked by pauljensen@chromium.org

[commit-bot: I haz the power, 2016-09-30 15:57:47]

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-30 16:29:45]

Committed patchset #8 (id:140001)

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

Description was changed from

==========
Non-functional improvement to reduce querying of WiFi SSID upon startup

Include WiFi SSID in NetworkState so it can be reused without re-querying.
Also improve how we fetch WiFi SSID if needed:
1. On Android 4.2+ devices, SSID can be retreived from NetworkInfo which
   NetworkChangeNotifierAutoDetect already queried
2. When an app has the ACCESS_WIFI_STATE permission, the SSID can be
   queried quickly directly from WifiManager.

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

to

==========
Non-functional improvement to reduce querying of WiFi SSID upon startup

Include WiFi SSID in NetworkState so it can be reused without re-querying.
Also improve how we fetch WiFi SSID if needed:
1. On Android 4.2+ devices, SSID can be retreived from NetworkInfo which
   NetworkChangeNotifierAutoDetect already queried
2. When an app has the ACCESS_WIFI_STATE permission, the SSID can be
   queried quickly directly from WifiManager.

BUG=647691
R=xunjieli

Committed: https://crrev.com/336a75e2356486da08bd04996affa8a43aa3056e
Cr-Commit-Position: refs/heads/master@{#422128}
==========

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

Patchset 8 (id:??) landed as
https://crrev.com/336a75e2356486da08bd04996affa8a43aa3056e
Cr-Commit-Position: refs/heads/master@{#422128}