From d1e449267a68220e84c45065ed0d6eb8fe393ed0 Mon Sep 17 00:00:00 2001 From: Sundeep Ghuman Date: Mon, 7 Aug 2017 11:21:38 -0700 Subject: [PATCH] Refactor WifiTracker sStaleScanResults. In ag/2580164, in order to minimize changes, we used mutable static state in WifiTracker to control cache eviction. This breaks a certain flow in SetupWizard. This change reverts sStaleScanResults to an instance member and then pipes the value into the AccessPoint update call to control cache eviction instead of relying on error prone use of mutable static state. Bug: 63479352 Test: runtest --path frameworks/base/packages/SettingsLib/tests/integ/src/com/android/settingslib/wifi/AccessPointTest.java && runtest --path frameworks/base/packages/SettingsLib/tests/integ/src/com/android/settingslib/wifi/WifiTrackerTest.java Manual: See bug Change-Id: Ia6d79c1904294da69841cbdf6eafbf42fa70f2d0 --- .../android/settingslib/wifi/AccessPoint.java | 30 +++++++++---------- .../android/settingslib/wifi/WifiTracker.java | 22 +++++++------- .../settingslib/wifi/AccessPointTest.java | 4 +-- .../settingslib/wifi/WifiTrackerTest.java | 4 --- 4 files changed, 28 insertions(+), 32 deletions(-) diff --git a/packages/SettingsLib/src/com/android/settingslib/wifi/AccessPoint.java b/packages/SettingsLib/src/com/android/settingslib/wifi/AccessPoint.java index 33af403904b86..b8c5aca4b946f 100644 --- a/packages/SettingsLib/src/com/android/settingslib/wifi/AccessPoint.java +++ b/packages/SettingsLib/src/com/android/settingslib/wifi/AccessPoint.java @@ -254,6 +254,8 @@ public class AccessPoint implements Comparable { mCarrierName = savedState.getString(KEY_CARRIER_NAME); } update(mConfig, mInfo, mNetworkInfo); + + // Do not evict old scan results on initial creation updateRssi(); updateSeen(); mId = sLastId.incrementAndGet(); @@ -495,10 +497,6 @@ public class AccessPoint implements Comparable { } private void evictOldScanResults() { - if (WifiTracker.sStaleScanResults) { - // Do not evict old scan results unless we are scanning and have fresh results. - return; - } long nowMs = SystemClock.elapsedRealtime(); for (Iterator iter = mScanResultCache.values().iterator(); iter.hasNext(); ) { ScanResult result = iter.next(); @@ -562,12 +560,8 @@ public class AccessPoint implements Comparable { * results, returning the best RSSI for all matching AccessPoints averaged with the previous * value. If the access point is not connected and there are no scan results, the rssi will be * set to {@link #UNREACHABLE_RSSI}. - * - *

Old scan results will be evicted from the cache when this method is invoked. */ private void updateRssi() { - evictOldScanResults(); - if (this.isActive()) { return; } @@ -586,14 +580,8 @@ public class AccessPoint implements Comparable { } } - /** - * Updates {@link #mSeen} based on the scan result cache. - * - *

Old scan results will be evicted from the cache when this method is invoked. - */ + /** Updates {@link #mSeen} based on the scan result cache. */ private void updateSeen() { - evictOldScanResults(); - // TODO(sghuman): Set to now if connected long seen = 0; @@ -1097,12 +1085,22 @@ public class AccessPoint implements Comparable { mAccessPointListener = listener; } - boolean update(ScanResult result) { + /** + * Update the AP with the given scan result. + * + * @param result the ScanResult to add to the AccessPoint scan cache + * @param evictOldScanResults whether stale scan results should be removed + * from the cache during this update process + * @return true if the scan result update caused a change in state which would impact ranking + * or AccessPoint rendering (e.g. wifi level, security) + */ + boolean update(ScanResult result, boolean evictOldScanResults) { if (matches(result)) { int oldLevel = getLevel(); /* Add or update the scan result for the BSSID */ mScanResultCache.put(result.BSSID, result); + if (evictOldScanResults) evictOldScanResults(); updateSeen(); updateRssi(); int newLevel = getLevel(); diff --git a/packages/SettingsLib/src/com/android/settingslib/wifi/WifiTracker.java b/packages/SettingsLib/src/com/android/settingslib/wifi/WifiTracker.java index 45032ce428829..a242570d6930f 100644 --- a/packages/SettingsLib/src/com/android/settingslib/wifi/WifiTracker.java +++ b/packages/SettingsLib/src/com/android/settingslib/wifi/WifiTracker.java @@ -147,7 +147,7 @@ public class WifiTracker { Scanner mScanner; @GuardedBy("mLock") - static boolean sStaleScanResults = true; + private boolean mStaleScanResults = true; public WifiTracker(Context context, WifiListener wifiListener, boolean includeSaved, boolean includeScans) { @@ -356,7 +356,7 @@ public class WifiTracker { *

This should always be called when done with a WifiTracker (if startTracking was called) to * ensure proper cleanup and prevent any further callbacks from occurring. * - *

Calling this method will set the {@link #sStaleScanResults} bit, which prevents + *

Calling this method will set the {@link #mStaleScanResults} bit, which prevents * {@link WifiListener#onAccessPointsChanged()} callbacks from being invoked (until the bit * is unset on the next SCAN_RESULTS_AVAILABLE_ACTION). */ @@ -374,7 +374,7 @@ public class WifiTracker { mWorkHandler.removePendingMessages(); mMainHandler.removePendingMessages(); - sStaleScanResults = true; + mStaleScanResults = true; } } @@ -480,7 +480,7 @@ public class WifiTracker { /** * Safely modify {@link #mInternalAccessPoints} by acquiring {@link #mLock} first. * - *

Will not perform the update if {@link #sStaleScanResults} is true + *

Will not perform the update if {@link #mStaleScanResults} is true */ private void updateAccessPoints() { List configs = mWifiManager.getConfiguredNetworks(); @@ -490,7 +490,7 @@ public class WifiTracker { } synchronized (mLock) { - if(!sStaleScanResults) { + if(!mStaleScanResults) { updateAccessPointsLocked(newScanResults, configs); } } @@ -500,7 +500,7 @@ public class WifiTracker { * Update the internal list of access points. * *

Do not called directly (except for forceUpdate), use {@link #updateAccessPoints()} which - * respects {@link #sStaleScanResults}. + * respects {@link #mStaleScanResults}. */ @GuardedBy("mLock") private void updateAccessPointsLocked(final List newScanResults, @@ -574,7 +574,8 @@ public class WifiTracker { boolean found = false; for (AccessPoint accessPoint : apMap.getAll(result.SSID)) { - if (accessPoint.update(result)) { + // We want to evict old scan results if are current results are not stale + if (accessPoint.update(result, !mStaleScanResults)) { found = true; break; } @@ -647,7 +648,8 @@ public class WifiTracker { for (int i = 0; i < N; i++) { if (cache.get(i).matches(result)) { AccessPoint ret = cache.remove(i); - ret.update(result); + // evict old scan results only if we have fresh results + ret.update(result, !mStaleScanResults); return ret; } } @@ -847,7 +849,7 @@ public class WifiTracker { // Only notify listeners of changes if we have fresh scan results, otherwise the // UI will be updated with stale results. We want to copy the APs regardless, // for instances where forceUpdate was invoked by the caller. - if (sStaleScanResults) { + if (mStaleScanResults) { copyAndNotifyListeners(false /*notifyListeners*/); } else { copyAndNotifyListeners(true /*notifyListeners*/); @@ -902,7 +904,7 @@ public class WifiTracker { switch (msg.what) { case MSG_UPDATE_ACCESS_POINTS: if (msg.arg1 == CLEAR_STALE_SCAN_RESULTS) { - sStaleScanResults = false; + mStaleScanResults = false; } updateAccessPoints(); break; diff --git a/packages/SettingsLib/tests/integ/src/com/android/settingslib/wifi/AccessPointTest.java b/packages/SettingsLib/tests/integ/src/com/android/settingslib/wifi/AccessPointTest.java index 24f0c7a9af1b2..ae59d37814300 100644 --- a/packages/SettingsLib/tests/integ/src/com/android/settingslib/wifi/AccessPointTest.java +++ b/packages/SettingsLib/tests/integ/src/com/android/settingslib/wifi/AccessPointTest.java @@ -258,7 +258,7 @@ public class AccessPointTest { scanResult.BSSID = "bssid"; scanResult.timestamp = SystemClock.elapsedRealtime() * 1000; scanResult.capabilities = ""; - assertThat(ap.update(scanResult)).isTrue(); + assertThat(ap.update(scanResult, true /* evict old scan results */)).isTrue(); assertThat(ap.getRssi()).isEqualTo(expectedRssi); } @@ -554,7 +554,7 @@ public class AccessPointTest { scanResult.isCarrierAp = true; scanResult.carrierApEapType = carrierApEapType; scanResult.carrierName = carrierName; - assertThat(ap.update(scanResult)).isTrue(); + assertThat(ap.update(scanResult, true /* evictOldScanresults */)).isTrue(); assertThat(ap.isCarrierAp()).isEqualTo(true); assertThat(ap.getCarrierApEapType()).isEqualTo(carrierApEapType); diff --git a/packages/SettingsLib/tests/integ/src/com/android/settingslib/wifi/WifiTrackerTest.java b/packages/SettingsLib/tests/integ/src/com/android/settingslib/wifi/WifiTrackerTest.java index 76d9823c6ab6a..df6587e5042f8 100644 --- a/packages/SettingsLib/tests/integ/src/com/android/settingslib/wifi/WifiTrackerTest.java +++ b/packages/SettingsLib/tests/integ/src/com/android/settingslib/wifi/WifiTrackerTest.java @@ -137,7 +137,6 @@ public class WifiTrackerTest { private Looper mMainLooper; private int mOriginalScoringUiSettingValue; - private boolean mOriginalStaleScanResultsValue; @Before public void setUp() { @@ -213,7 +212,6 @@ public class WifiTrackerTest { Settings.Global.NETWORK_SCORING_UI_ENABLED, 1 /* enabled */); - mOriginalStaleScanResultsValue = WifiTracker.sStaleScanResults; } @After @@ -222,8 +220,6 @@ public class WifiTrackerTest { InstrumentationRegistry.getTargetContext().getContentResolver(), Settings.Global.NETWORK_SCORING_UI_ENABLED, mOriginalScoringUiSettingValue); - - WifiTracker.sStaleScanResults = mOriginalStaleScanResultsValue; } private static ScanResult buildScanResult1() {