Optimize NetworkChangeNotifierAutoDetect construction

Optimize NetworkChangeNotifierAutoDetect construction

1) Avoid doing duplicated work, like fetching Wifi SSID twice.
2) When app has ACCESS_WIFI_STATE permission, call WifiManager
   directly to get SSID as this is faster than calling
   AndroidNetworkLibrary.getWifiSSID().

BUG=647691

[pauljensen, 2016-09-16 14:37:48]

Description was changed from

==========
Optimize NetworkChangeNotifierAutoDetect construction

1) Avoid doing duplicated work, like fetching Wifi SSID twice.
2) When app has ACCESS_WIFI_STATE permission, call WifiManager
   directly to get SSID as this is faster than calling
   AndroidNetworkLibrary.getWifiSSID().
==========

to

==========
Optimize NetworkChangeNotifierAutoDetect construction

1) Avoid doing duplicated work, like fetching Wifi SSID twice.
2) When app has ACCESS_WIFI_STATE permission, call WifiManager
   directly to get SSID as this is faster than calling
   AndroidNetworkLibrary.getWifiSSID().

BUG=647691
==========

[pauljensen, 2016-09-16 14:40:11]

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

[pauljensen, 2016-09-16 14:40:11]

Helen, PTAL.  I think this should greatly improve startup time.  This is an
attempt to grab some low-hanging-fruit, without complicating things too much. 
I'm hesitant to move initialization off the UI thread as it could introduce
races.

[xunjieli, 2016-09-16 17:22:03]

This is a great improvement! I just have one small request. It seems that there
are two optimizations. One is to avoid getWifiSSID() duplicated calls and the
second is to avoid signalling connection changes unnecessarily. 

How about splitting this into two CLs? The caching wifi SSID part is easier to
review. The second part (adding mIgnoreNextBroadcast and mDontSignal), I need a
bit more time to understand the code. It might be good to separate them so I can
understand the change better.

Thanks!

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

https://codereview.chromium.org/2319753002/diff/60001/net/android/java/src/or...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:240:
static class WifiManagerDelegate {
Passing |wifiInfo| around in arguments can get a bit tedious if this file grows
in size. How about making WifiManagerDelegate cache the last getSSID() result?
so we can encapsulate the "don't query unless necessary" logic? 

Something like:

clss WifiManagerDelegate {
   private WifiInfo mWifiInfo;
   WifiManagerDelegate(Context context) {
      updateWifiInfo();
   }
   // This should be called during delegate initialization or
   // when connection changed.
   void updateWifiInfo () {
      // Queuries the platform to fetch current wifiInfo
      mWifiInfo = newWifiInfo;
   }
 
   String getWifiSSID() {
      return mWifiInfo.getSSID();
   }

   WifiInfo getWifiInfo() {
      return mWifiInfo;
   }

   ...
}

[pauljensen, 2016-09-19 13:55:42]

I'm happy to split into multiple CLs.

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

https://codereview.chromium.org/2319753002/diff/60001/net/android/java/src/or...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:240:
static class WifiManagerDelegate {
On 2016/09/16 17:22:03, xunjieli wrote:
> Passing |wifiInfo| around in arguments can get a bit tedious if this file
grows
> in size. How about making WifiManagerDelegate cache the last getSSID() result?
> so we can encapsulate the "don't query unless necessary" logic? 
> 
> Something like:
> 
> clss WifiManagerDelegate {
>    private WifiInfo mWifiInfo;
>    WifiManagerDelegate(Context context) {
>       updateWifiInfo();
>    }
>    // This should be called during delegate initialization or
>    // when connection changed.
>    void updateWifiInfo () {
>       // Queuries the platform to fetch current wifiInfo
>       mWifiInfo = newWifiInfo;
>    }
>  
>    String getWifiSSID() {
>       return mWifiInfo.getSSID();
>    }
> 
>    WifiInfo getWifiInfo() {
>       return mWifiInfo;
>    }
> 
>    ...
> }

I agree it's quite tedious.  How about I make it part of NetworkState?

[xunjieli, 2016-09-19 14:11:46]

> How about I make it part of NetworkState?
NetworkState does seem to be a better place for the cached result of WifiInfo!
SGTM

[pauljensen, 2016-09-22 15:15:10]

Ok, I've split this into five different CLs that is each much more palatable. 
This also allowed for some optimization I didn't think of previously.  One of
the CLs I can also send to someone else for review to better distribute the
review load.  I'm abandoning and closing this original one.