Address aosp/1199568 leftover comments

Bug: 139269711
Test: atest NetworkStackTests NetworkStackNextTests
Change-Id: I6e4f16e70b290ce6a3566b92e5946c65075971a1
Merged-In: I6e4f16e70b290ce6a3566b92e5946c65075971a1
(cherry picked from aosp/1240746)

5 files changed, 55 insertions, 110 deletions

diff --git a/apishim/29/com/android/networkstack/apishim/api29/NetworkInformationShimImpl.java b/apishim/29/com/android/networkstack/apishim/api29/NetworkInformationShimImpl.java
index b24b473..2a22108 100644
--- a/apishim/29/com/android/networkstack/apishim/api29/NetworkInformationShimImpl.java
+++ b/apishim/29/com/android/networkstack/apishim/api29/NetworkInformationShimImpl.java
@@ -74,7 +74,7 @@ public class NetworkInformationShimImpl implements NetworkInformationShim {
 
     @Nullable
     @Override
-    public String getSSID(@Nullable NetworkCapabilities nc) {
+    public String getSsid(@Nullable NetworkCapabilities nc) {
         // Not supported on this API level
         return null;
     }
diff --git a/apishim/30/com/android/networkstack/apishim/NetworkInformationShimImpl.java b/apishim/30/com/android/networkstack/apishim/NetworkInformationShimImpl.java
index 6466662..a533027 100644
--- a/apishim/30/com/android/networkstack/apishim/NetworkInformationShimImpl.java
+++ b/apishim/30/com/android/networkstack/apishim/NetworkInformationShimImpl.java
@@ -71,7 +71,7 @@ public class NetworkInformationShimImpl extends
 
     @Nullable
     @Override
-    public String getSSID(@Nullable NetworkCapabilities nc) {
+    public String getSsid(@Nullable NetworkCapabilities nc) {
         if (nc == null) return null;
         return nc.getSSID();
     }
diff --git a/apishim/common/com/android/networkstack/apishim/NetworkInformationShim.java b/apishim/common/com/android/networkstack/apishim/NetworkInformationShim.java
index c266043..de26c77 100644
--- a/apishim/common/com/android/networkstack/apishim/NetworkInformationShim.java
+++ b/apishim/common/com/android/networkstack/apishim/NetworkInformationShim.java
@@ -49,7 +49,7 @@ public interface NetworkInformationShim {
      * @see NetworkCapabilities#getSSID()
      */
     @Nullable
-    String getSSID(@Nullable NetworkCapabilities nc);
+    String getSsid(@Nullable NetworkCapabilities nc);
 
     /**
      * @see LinkProperties#makeSensitiveFieldsParcelingCopy()
diff --git a/src/com/android/networkstack/NetworkStackNotifier.java b/src/com/android/networkstack/NetworkStackNotifier.java
index 98caa76..d86e03b 100644
--- a/src/com/android/networkstack/NetworkStackNotifier.java
+++ b/src/com/android/networkstack/NetworkStackNotifier.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2019 The Android Open Source Project
+ * Copyright (C) 2020 The Android Open Source Project
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -31,7 +31,6 @@ import android.net.NetworkCapabilities;
 import android.net.NetworkRequest;
 import android.os.Handler;
 import android.os.Looper;
-import android.os.Message;
 import android.os.UserHandle;
 import android.provider.Settings;
 import android.text.TextUtils;
@@ -52,9 +51,6 @@ import java.util.function.Consumer;
  * Displays notification related to connected networks.
  */
 public class NetworkStackNotifier {
-    @VisibleForTesting
-    protected static final int MSG_DISMISS_CONNECTED = 1;
-
     private final Context mContext;
     private final Handler mHandler;
     private final NotificationManager mNotificationManager;
@@ -65,9 +61,14 @@ public class NetworkStackNotifier {
     @Nullable
     private Network mDefaultNetwork;
     @NonNull
-    private static final NetworkInformationShim NETWORK_INFO_SHIM =
-            NetworkInformationShimImpl.newInstance();
+    private final NetworkInformationShim mInfoShim = NetworkInformationShimImpl.newInstance();
 
+    /**
+     * The TrackedNetworkStatus object is a data class that keeps track of the relevant state of the
+     * various networks on the device. For efficiency the members are mutable, which means any
+     * instance of this object should only ever be accessed on the looper thread passed in the
+     * constructor. Any access (read or write) from any other thread would be incorrect.
+     */
     private static class TrackedNetworkStatus {
         private boolean mValidatedNotificationPending;
         private int mShownNotification = NOTE_NONE;
@@ -78,20 +79,6 @@ public class NetworkStackNotifier {
             if (mNetworkCapabilities == null) return false;
             return mNetworkCapabilities.hasCapability(NetworkCapabilities.NET_CAPABILITY_VALIDATED);
         }
-
-        private boolean isWifiNetwork() {
-            if (mNetworkCapabilities == null) return false;
-            return mNetworkCapabilities.hasTransport(NetworkCapabilities.TRANSPORT_WIFI);
-        }
-
-        @Nullable
-        private CaptivePortalDataShim getCaptivePortalData() {
-            return NETWORK_INFO_SHIM.getCaptivePortalData(mLinkProperties);
-        }
-
-        private String getSSID() {
-            return NETWORK_INFO_SHIM.getSSID(mNetworkCapabilities);
-        }
     }
 
     @VisibleForTesting
@@ -105,7 +92,8 @@ public class NetworkStackNotifier {
 
     private static final int NOTE_ID_NETWORK_INFO = 1;
 
-    private static final int CONNECTED_NOTIFICATION_TIMEOUT_MS = 20 * 1000;
+    @VisibleForTesting
+    protected static final long CONNECTED_NOTIFICATION_TIMEOUT_MS = 20_000L;
 
     protected static class Dependencies {
         public PendingIntent getActivityPendingIntent(Context context, Intent intent, int flags) {
@@ -120,14 +108,15 @@ public class NetworkStackNotifier {
     protected NetworkStackNotifier(@NonNull Context context, @NonNull Looper looper,
             @NonNull Dependencies dependencies) {
         mContext = context;
-        mHandler = new NotifierHandler(looper);
+        mHandler = new Handler(looper);
         mDependencies = dependencies;
         mNotificationManager = getContextAsUser(mContext, UserHandle.ALL)
                 .getSystemService(NotificationManager.class);
         final ConnectivityManager cm = context.getSystemService(ConnectivityManager.class);
         cm.registerDefaultNetworkCallback(new DefaultNetworkCallback(), mHandler);
         cm.registerNetworkCallback(
-                new NetworkRequest.Builder().build(),
+                new NetworkRequest.Builder()
+                        .addTransportType(NetworkCapabilities.TRANSPORT_WIFI).build(),
                 new AllNetworksCallback(),
                 mHandler);
 
@@ -165,28 +154,20 @@ public class NetworkStackNotifier {
         mHandler.post(() -> setCaptivePortalValidationPending(network));
     }
 
-    private class NotifierHandler extends Handler {
-        NotifierHandler(Looper looper) {
-            super(looper);
-        }
+    private void setCaptivePortalValidationPending(@NonNull Network network) {
+        updateNetworkStatus(network, status -> {
+            status.mValidatedNotificationPending = true;
+            status.mShownNotification = NOTE_NONE;
+        });
+    }
 
-        @Override
-        public void handleMessage(Message msg) {
-            switch(msg.what) {
-                case MSG_DISMISS_CONNECTED:
-                    final Network network = (Network) msg.obj;
-                    final TrackedNetworkStatus networkStatus = mNetworkStatus.get(network);
-                    if (networkStatus != null
-                            && networkStatus.mShownNotification == NOTE_CONNECTED) {
-                        dismissNotification(getNotificationTag(network), networkStatus);
-                    }
-                    break;
-            }
-        }
+    @Nullable
+    private CaptivePortalDataShim getCaptivePortalData(@NonNull TrackedNetworkStatus status) {
+        return mInfoShim.getCaptivePortalData(status.mLinkProperties);
     }
 
-    private void setCaptivePortalValidationPending(@NonNull Network network) {
-        updateNetworkStatus(network, status -> status.mValidatedNotificationPending = true);
+    private String getSsid(@NonNull TrackedNetworkStatus status) {
+        return mInfoShim.getSsid(status.mNetworkCapabilities);
     }
 
     private void updateNetworkStatus(@NonNull Network network,
@@ -201,7 +182,7 @@ public class NetworkStackNotifier {
         // The required network attributes callbacks were not fired yet for this network
         if (networkStatus == null) return;
 
-        final CaptivePortalDataShim capportData = networkStatus.getCaptivePortalData();
+        final CaptivePortalDataShim capportData = getCaptivePortalData(networkStatus);
         final boolean showVenueInfo = capportData != null && capportData.getVenueInfoUrl() != null
                 // Only show venue info on validated networks, to prevent misuse of the notification
                 // as an alternate login flow that uses the default browser (which would be broken
@@ -245,13 +226,12 @@ public class NetworkStackNotifier {
         } else if (showValidated) {
             if (networkStatus.mShownNotification == NOTE_CONNECTED) return;
 
-            builder = getNotificationBuilder(CHANNEL_CONNECTED, networkStatus, res);
-            if (networkStatus.isWifiNetwork()) {
-                builder.setContentIntent(mDependencies.getActivityPendingIntent(
-                        getContextAsUser(mContext, UserHandle.CURRENT),
-                        new Intent(Settings.ACTION_WIFI_SETTINGS),
-                        PendingIntent.FLAG_UPDATE_CURRENT));
-            }
+            builder = getNotificationBuilder(CHANNEL_CONNECTED, networkStatus, res)
+                    .setTimeoutAfter(CONNECTED_NOTIFICATION_TIMEOUT_MS)
+                    .setContentIntent(mDependencies.getActivityPendingIntent(
+                            getContextAsUser(mContext, UserHandle.CURRENT),
+                            new Intent(Settings.ACTION_WIFI_SETTINGS),
+                            PendingIntent.FLAG_UPDATE_CURRENT));
 
             networkStatus.mShownNotification = NOTE_CONNECTED;
         } else {
@@ -268,11 +248,6 @@ public class NetworkStackNotifier {
             networkStatus.mValidatedNotificationPending = false;
         }
         mNotificationManager.notify(notificationTag, NOTE_ID_NETWORK_INFO, builder.build());
-        mHandler.removeMessages(MSG_DISMISS_CONNECTED, network);
-        if (networkStatus.mShownNotification == NOTE_CONNECTED) {
-            mHandler.sendMessageDelayed(mHandler.obtainMessage(MSG_DISMISS_CONNECTED, network),
-                    CONNECTED_NOTIFICATION_TIMEOUT_MS);
-        }
     }
 
     private void dismissNotification(@NonNull String tag, @NonNull TrackedNetworkStatus status) {
@@ -304,9 +279,9 @@ public class NetworkStackNotifier {
         return mNotificationManager.getNotificationChannel(CHANNEL_VENUE_INFO) != null;
     }
 
-    private static String getConnectedNotificationTitle(@NonNull Resources res,
+    private String getConnectedNotificationTitle(@NonNull Resources res,
             @NonNull TrackedNetworkStatus status) {
-        final String ssid = status.getSSID();
+        final String ssid = getSsid(status);
         if (TextUtils.isEmpty(ssid)) {
             return res.getString(R.string.connected);
         }
diff --git a/tests/unit/src/com/android/networkstack/NetworkStackNotifierTest.kt b/tests/unit/src/com/android/networkstack/NetworkStackNotifierTest.kt
index 352d185..39d53af 100644
--- a/tests/unit/src/com/android/networkstack/NetworkStackNotifierTest.kt
+++ b/tests/unit/src/com/android/networkstack/NetworkStackNotifierTest.kt
@@ -33,7 +33,6 @@ import android.net.LinkProperties
 import android.net.Network
 import android.net.NetworkCapabilities
 import android.net.NetworkCapabilities.NET_CAPABILITY_VALIDATED
-import android.net.NetworkCapabilities.TRANSPORT_CELLULAR
 import android.net.NetworkCapabilities.TRANSPORT_WIFI
 import android.net.Uri
 import android.os.Handler
@@ -47,8 +46,8 @@ import androidx.test.platform.app.InstrumentationRegistry
 import com.android.dx.mockito.inline.extended.ExtendedMockito.verify
 import com.android.networkstack.NetworkStackNotifier.CHANNEL_CONNECTED
 import com.android.networkstack.NetworkStackNotifier.CHANNEL_VENUE_INFO
+import com.android.networkstack.NetworkStackNotifier.CONNECTED_NOTIFICATION_TIMEOUT_MS
 import com.android.networkstack.NetworkStackNotifier.Dependencies
-import com.android.networkstack.NetworkStackNotifier.MSG_DISMISS_CONNECTED
 import com.android.networkstack.apishim.NetworkInformationShimImpl
 import org.junit.Assume.assumeTrue
 import org.junit.Before
@@ -65,8 +64,6 @@ import org.mockito.Mockito.never
 import org.mockito.MockitoAnnotations
 import kotlin.reflect.KClass
 import kotlin.test.assertEquals
-import kotlin.test.assertNull
-import kotlin.test.assertTrue
 
 @RunWith(AndroidTestingRunner::class)
 @SmallTest
@@ -176,22 +173,26 @@ class NetworkStackNotifierTest {
         verify(mNm, never()).notify(any(), anyInt(), any())
     }
 
-    private fun verifyConnectedNotification() {
+    private fun verifyConnectedNotification(timeout: Long = CONNECTED_NOTIFICATION_TIMEOUT_MS) {
         verify(mNm).notify(eq(TEST_NETWORK_TAG), mNoteIdCaptor.capture(), mNoteCaptor.capture())
         val note = mNoteCaptor.value
         assertEquals(mPendingIntent, note.contentIntent)
         assertEquals(CHANNEL_CONNECTED, note.channelId)
+        assertEquals(timeout, note.timeoutAfter)
         verify(mDependencies).getActivityPendingIntent(
                 eq(mCurrentUserContext), mIntentCaptor.capture(), eq(FLAG_UPDATE_CURRENT))
     }
 
-    private fun verifyDismissConnectedNotification(noteId: Int) {
-        assertTrue(mHandler.hasMessages(MSG_DISMISS_CONNECTED, TEST_NETWORK))
-        // Execute dismiss message now
-        mHandler.sendMessageAtFrontOfQueue(
-                mHandler.obtainMessage(MSG_DISMISS_CONNECTED, TEST_NETWORK))
-        mLooper.processMessages(1)
-        verify(mNm).cancel(TEST_NETWORK_TAG, noteId)
+    private fun verifyCanceledNotificationAfterNetworkLost() {
+        onLost(TEST_NETWORK)
+        mLooper.processAllMessages()
+        verify(mNm).cancel(TEST_NETWORK_TAG, mNoteIdCaptor.value)
+    }
+
+    private fun verifyCanceledNotificationAfterDefaultNetworkLost() {
+        onDefaultNetworkLost(TEST_NETWORK)
+        mLooper.processAllMessages()
+        verify(mNm).cancel(TEST_NETWORK_TAG, mNoteIdCaptor.value)
     }
 
     @Test
@@ -204,7 +205,7 @@ class NetworkStackNotifierTest {
         verifyConnectedNotification()
         verify(mResources).getString(R.string.connected)
         verifyWifiSettingsIntent(mIntentCaptor.value)
-        verifyDismissConnectedNotification(mNoteIdCaptor.value)
+        verifyCanceledNotificationAfterNetworkLost()
     }
 
     @Test
@@ -222,29 +223,7 @@ class NetworkStackNotifierTest {
         verifyConnectedNotification()
         verify(mResources).getString(R.string.connected_to_ssid_param1, TEST_SSID)
         verifyWifiSettingsIntent(mIntentCaptor.value)
-        verifyDismissConnectedNotification(mNoteIdCaptor.value)
-    }
-
-    @Test
-    fun testConnectedNotification_WithNonWifiNetwork() {
-        // NetworkCapabilities#getSSID is not available for API <= Q
-        assumeTrue(NetworkInformationShimImpl.useApiAboveQ())
-        val capabilities = NetworkCapabilities()
-                .addTransportType(TRANSPORT_CELLULAR)
-                .addCapability(NET_CAPABILITY_VALIDATED)
-                .setSSID(TEST_SSID)
-
-        onCapabilitiesChanged(EMPTY_CAPABILITIES)
-        mNotifier.notifyCaptivePortalValidationPending(TEST_NETWORK)
-        onCapabilitiesChanged(capabilities)
-        mLooper.processAllMessages()
-
-        verify(mNm).notify(eq(TEST_NETWORK_TAG), mNoteIdCaptor.capture(), mNoteCaptor.capture())
-        val note = mNoteCaptor.value
-        assertNull(note.contentIntent)
-        assertEquals(CHANNEL_CONNECTED, note.channelId)
-        verify(mResources).getString(R.string.connected_to_ssid_param1, TEST_SSID)
-        verifyDismissConnectedNotification(mNoteIdCaptor.value)
+        verifyCanceledNotificationAfterNetworkLost()
     }
 
     @Test
@@ -258,16 +237,10 @@ class NetworkStackNotifierTest {
 
         mLooper.processAllMessages()
 
-        verify(mNm).notify(eq(TEST_NETWORK_TAG), mNoteIdCaptor.capture(), mNoteCaptor.capture())
-
-        verifyConnectedNotification()
+        verifyConnectedNotification(timeout = 0)
         verifyVenueInfoIntent(mIntentCaptor.value)
         verify(mResources).getString(R.string.tap_for_info)
-
-        onDefaultNetworkLost(TEST_NETWORK)
-        mLooper.processAllMessages()
-        // Notification only shown on default network
-        verify(mNm).cancel(TEST_NETWORK_TAG, mNoteIdCaptor.value)
+        verifyCanceledNotificationAfterDefaultNetworkLost()
     }
 
     @Test
@@ -284,7 +257,7 @@ class NetworkStackNotifierTest {
         verifyConnectedNotification()
         verifyWifiSettingsIntent(mIntentCaptor.value)
         verify(mResources, never()).getString(R.string.tap_for_info)
-        verifyDismissConnectedNotification(mNoteIdCaptor.value)
+        verifyCanceledNotificationAfterNetworkLost()
     }
 
     @Test
@@ -300,10 +273,7 @@ class NetworkStackNotifierTest {
         verify(mDependencies).getActivityPendingIntent(
                 eq(mCurrentUserContext), mIntentCaptor.capture(), eq(FLAG_UPDATE_CURRENT))
         verifyVenueInfoIntent(mIntentCaptor.value)
-
-        onLost(TEST_NETWORK)
-        mLooper.processAllMessages()
-        verify(mNm).cancel(TEST_NETWORK_TAG, mNoteIdCaptor.value)
+        verifyCanceledNotificationAfterDefaultNetworkLost()
     }
 
     @Test