Network notifications: revamp keying scheme

This patch changes the (tag: String, id: Int) keying scheme for network
notifications so that TRON notification counters can count network
related notifications unambiguously.

TRON instruments all notifications shown for package "android" as well
as user interactions with these Notifications. These counters are
grouped by id. However the network notifications ("no internet" dialog,
"captive portal sign in" dialog, ...) use a static tag and a dynamic id
for keying notifications, preventing the counters to correctly
aggregate. In addition there is also the risk of collision with other
SystemUi notification ids not managed by NetworkNotificationManager.

In order to make the TRON counters useful for network notifications,
the id is now encoding the network notification type in a stable way
while the tag is used to uniquely identify network notifications.

Test: change covered by previously added new unit tests.
Bug: 32198726
Bug: 33030620
Change-Id: I1201d1d4fab8183dd83449c2afb2c35cfece883f

services/core/java/com/android/server/connectivity/NetworkNotificationManager.java

@@ -19,7 +19,6 @@ package com.android.server.connectivity;
 import android.app.Notification;
 import android.app.NotificationManager;
 import android.app.PendingIntent;
-import android.widget.Toast;
 import android.content.Context;
 import android.content.Intent;
 import android.content.res.Resources;
@@ -27,18 +26,40 @@ import android.net.NetworkCapabilities;
 import android.os.UserHandle;
 import android.telephony.TelephonyManager;
 import android.util.Slog;
-import com.android.internal.annotations.VisibleForTesting;
+import android.util.SparseArray;
+import android.util.SparseIntArray;
+import android.widget.Toast;
 import com.android.internal.R;
+import com.android.internal.annotations.VisibleForTesting;
+import com.android.internal.logging.nano.MetricsProto.MetricsEvent;
 
-import static android.net.NetworkCapabilities.*;
+import static android.net.NetworkCapabilities.NET_CAPABILITY_INTERNET;
-
+import static android.net.NetworkCapabilities.TRANSPORT_CELLULAR;
+import static android.net.NetworkCapabilities.TRANSPORT_WIFI;
 
 public class NetworkNotificationManager {
 
-    public static enum NotificationType { SIGN_IN, NO_INTERNET, LOST_INTERNET, NETWORK_SWITCH };
+    public static enum NotificationType {
+        LOST_INTERNET(MetricsEvent.NOTIFICATION_NETWORK_LOST_INTERNET),
+        NETWORK_SWITCH(MetricsEvent.NOTIFICATION_NETWORK_SWITCH),
+        NO_INTERNET(MetricsEvent.NOTIFICATION_NETWORK_NO_INTERNET),
+        SIGN_IN(MetricsEvent.NOTIFICATION_NETWORK_SIGN_IN);
 
-    @VisibleForTesting
+        public final int eventId;
-    static final String NOTIFICATION_ID = "Connectivity.Notification";
+
+        NotificationType(int eventId) {
+            this.eventId = eventId;
+            Holder.sIdToTypeMap.put(eventId, this);
+        }
+
+        private static class Holder {
+            private static SparseArray<NotificationType> sIdToTypeMap = new SparseArray<>();
+        }
+
+        public static NotificationType getFromId(int id) {
+            return Holder.sIdToTypeMap.get(id);
+        }
+    };
 
     private static final String TAG = NetworkNotificationManager.class.getSimpleName();
     private static final boolean DBG = true;
@@ -47,11 +68,14 @@ public class NetworkNotificationManager {
     private final Context mContext;
     private final TelephonyManager mTelephonyManager;
     private final NotificationManager mNotificationManager;
+    // Tracks the types of notifications managed by this instance, from creation to cancellation.
+    private final SparseIntArray mNotificationTypeMap;
 
     public NetworkNotificationManager(Context c, TelephonyManager t, NotificationManager n) {
         mContext = c;
         mTelephonyManager = t;
         mNotificationManager = n;
+        mNotificationTypeMap = new SparseIntArray();
     }
 
     // TODO: deal more gracefully with multi-transport networks.
@@ -101,8 +125,10 @@ public class NetworkNotificationManager {
      */
     public void showNotification(int id, NotificationType notifyType, NetworkAgentInfo nai,
             NetworkAgentInfo switchToNai, PendingIntent intent, boolean highPriority) {
-        int transportType;
+        final String tag = tagFor(id);
-        String extraInfo;
+        final int eventId = notifyType.eventId;
+        final int transportType;
+        final String extraInfo;
         if (nai != null) {
             transportType = getFirstTransportType(nai);
             extraInfo = nai.networkInfo.getExtraInfo();
@@ -115,9 +141,9 @@ public class NetworkNotificationManager {
         }
 
         if (DBG) {
-            Slog.d(TAG, "showNotification id=" + id + " " + notifyType
+            Slog.d(TAG, String.format(
-                    + " transportType=" + getTransportName(transportType)
+                    "showNotification tag=%s event=%s transport=%s extraInfo=%d highPrioriy=%s",
-                    + " extraInfo=" + extraInfo + " highPriority=" + highPriority);
+                    tag, nameOf(eventId), getTransportName(transportType), extraInfo, highPriority));
         }
 
         Resources r = Resources.getSystem();
@@ -185,22 +211,31 @@ public class NetworkNotificationManager {
 
         Notification notification = builder.build();
 
+        mNotificationTypeMap.put(id, eventId);
         try {
-            mNotificationManager.notifyAsUser(NOTIFICATION_ID, id, notification, UserHandle.ALL);
+            mNotificationManager.notifyAsUser(tag, eventId, notification, UserHandle.ALL);
         } catch (NullPointerException npe) {
             Slog.d(TAG, "setNotificationVisible: visible notificationManager error", npe);
         }
     }
 
     public void clearNotification(int id) {
+        final String tag = tagFor(id);
+        if (mNotificationTypeMap.indexOfKey(id) < 0) {
+            Slog.e(TAG, "cannot clear unknown notification with tag=" + tag);
+            return;
+        }
+        final int eventId = mNotificationTypeMap.get(id);
         if (DBG) {
-            Slog.d(TAG, "clearNotification id=" + id);
+            Slog.d(TAG, String.format("clearing notification tag=%s event=", tag, nameOf(eventId)));
         }
         try {
-            mNotificationManager.cancelAsUser(NOTIFICATION_ID, id, UserHandle.ALL);
+            mNotificationManager.cancelAsUser(tag, eventId, UserHandle.ALL);
         } catch (NullPointerException npe) {
-            Slog.d(TAG, "setNotificationVisible: cancel notificationManager error", npe);
+            Slog.d(TAG, String.format(
+                    "failed to clear notification tag=%s event=", tag, nameOf(eventId)), npe);
         }
+        mNotificationTypeMap.delete(id);
     }
 
     /**
@@ -223,4 +258,15 @@ public class NetworkNotificationManager {
                 R.string.network_switch_metered_toast, fromTransport, toTransport);
         Toast.makeText(mContext, text, Toast.LENGTH_LONG).show();
     }
+
+    @VisibleForTesting
+    static String tagFor(int id) {
+        return String.format("ConnectivityNotification:%d", id);
+    }
+
+    @VisibleForTesting
+    static String nameOf(int eventId) {
+        NotificationType t = NotificationType.getFromId(eventId);
+        return (t != null) ? t.name() : "UNKNOWN";
+    }
 }

tests/net/java/com/android/server/connectivity/NetworkNotificationManagerTest.java

@@ -48,8 +48,6 @@ import static org.mockito.Mockito.when;
 
 public class NetworkNotificationManagerTest extends TestCase {
 
-    static final String NOTIFICATION_ID = NetworkNotificationManager.NOTIFICATION_ID;
-
     static final NetworkCapabilities CELL_CAPABILITIES = new NetworkCapabilities();
     static final NetworkCapabilities WIFI_CAPABILITIES = new NetworkCapabilities();
     static {
@@ -108,11 +106,11 @@ public class NetworkNotificationManagerTest extends TestCase {
         }
 
         for (int i = 0; i < ids.size(); i++) {
-            final int expectedId = NETWORK_ID_BASE + i;
+            final int id = ids.get(i);
-            verify(mNotificationManager, times(1))
+            final int eventId = types.get(i).eventId;
-                    .notifyAsUser(eq(NOTIFICATION_ID), eq(expectedId), any(), any());
+            final String tag = NetworkNotificationManager.tagFor(id);
-            verify(mNotificationManager, times(1))
+            verify(mNotificationManager, times(1)).notifyAsUser(eq(tag), eq(eventId), any(), any());
-                    .cancelAsUser(eq(NOTIFICATION_ID), eq(expectedId), any());
+            verify(mNotificationManager, times(1)).cancelAsUser(eq(tag), eq(eventId), any());
         }
     }
 
@@ -125,8 +123,9 @@ public class NetworkNotificationManagerTest extends TestCase {
 
         mManager.showNotification(102, NO_INTERNET, mWifiNai, mCellNai, null, false);
 
-        verify(mNotificationManager, times(1))
+        final int eventId = NO_INTERNET.eventId;
-                .notifyAsUser(eq(NOTIFICATION_ID), eq(102), any(), any());
+        final String tag = NetworkNotificationManager.tagFor(102);
+        verify(mNotificationManager, times(1)).notifyAsUser(eq(tag), eq(eventId), any(), any());
     }
 
     @SmallTest