Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit d1e44926 authored by Sundeep Ghuman's avatar Sundeep Ghuman
Browse files

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
parent c209e50d
Loading
Loading
Loading
Loading
+14 −16
Original line number Diff line number Diff line
@@ -254,6 +254,8 @@ public class AccessPoint implements Comparable<AccessPoint> {
            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<AccessPoint> {
    }

    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<ScanResult> iter = mScanResultCache.values().iterator(); iter.hasNext(); ) {
            ScanResult result = iter.next();
@@ -562,12 +560,8 @@ public class AccessPoint implements Comparable<AccessPoint> {
     * 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}.
     *
     * <p>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<AccessPoint> {
        }
    }

    /**
     * Updates {@link #mSeen} based on the scan result cache.
     *
     * <p>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<AccessPoint> {
        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();
+12 −10
Original line number Diff line number Diff line
@@ -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 {
     * <p>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.
     *
     * <p>Calling this method will set the {@link #sStaleScanResults} bit, which prevents
     * <p>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.
     *
     * <p>Will not perform the update if {@link #sStaleScanResults} is true
     * <p>Will not perform the update if {@link #mStaleScanResults} is true
     */
    private void updateAccessPoints() {
        List<WifiConfiguration> 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.
     *
     * <p>Do not called directly (except for forceUpdate), use {@link #updateAccessPoints()} which
     * respects {@link #sStaleScanResults}.
     * respects {@link #mStaleScanResults}.
     */
    @GuardedBy("mLock")
    private void updateAccessPointsLocked(final List<ScanResult> 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;
+2 −2
Original line number Diff line number Diff line
@@ -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);
+0 −4
Original line number Diff line number Diff line
@@ -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() {