From e9c9d4bf0cb0275dd2b27ce2990fb27fe59650b3 Mon Sep 17 00:00:00 2001 From: Hugo Benichi Date: Thu, 8 Dec 2016 09:36:52 +0900 Subject: [PATCH] DO NOT MERGE 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 (cherry picked from commit fb2609d3eee1c7a4dda889c000f32183a044978a) Change-Id: Iadf7f15da38de28587090ed0395f15c24d4ad442 --- .../NetworkNotificationManager.java | 78 +++++++++++++++---- .../NetworkNotificationManagerTest.java | 17 ++-- 2 files changed, 70 insertions(+), 25 deletions(-) diff --git a/services/core/java/com/android/server/connectivity/NetworkNotificationManager.java b/services/core/java/com/android/server/connectivity/NetworkNotificationManager.java index 9ffa40b752..c051642a21 100644 --- a/services/core/java/com/android/server/connectivity/NetworkNotificationManager.java +++ b/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.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 - static final String NOTIFICATION_ID = "Connectivity.Notification"; + public final int eventId; + + NotificationType(int eventId) { + this.eventId = eventId; + Holder.sIdToTypeMap.put(eventId, this); + } + + private static class Holder { + private static SparseArray 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; - String extraInfo; + final String tag = tagFor(id); + 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 - + " transportType=" + getTransportName(transportType) - + " extraInfo=" + extraInfo + " highPriority=" + highPriority); + Slog.d(TAG, String.format( + "showNotification tag=%s event=%s transport=%s extraInfo=%d highPrioriy=%s", + 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"; + } } diff --git a/tests/net/java/com/android/server/connectivity/NetworkNotificationManagerTest.java b/tests/net/java/com/android/server/connectivity/NetworkNotificationManagerTest.java index 813e928118..98073ce1e5 100644 --- a/tests/net/java/com/android/server/connectivity/NetworkNotificationManagerTest.java +++ b/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; - verify(mNotificationManager, times(1)) - .notifyAsUser(eq(NOTIFICATION_ID), eq(expectedId), any(), any()); - verify(mNotificationManager, times(1)) - .cancelAsUser(eq(NOTIFICATION_ID), eq(expectedId), any()); + final int id = ids.get(i); + final int eventId = types.get(i).eventId; + final String tag = NetworkNotificationManager.tagFor(id); + verify(mNotificationManager, times(1)).notifyAsUser(eq(tag), eq(eventId), any(), 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)) - .notifyAsUser(eq(NOTIFICATION_ID), eq(102), any(), any()); + final int eventId = NO_INTERNET.eventId; + final String tag = NetworkNotificationManager.tagFor(102); + verify(mNotificationManager, times(1)).notifyAsUser(eq(tag), eq(eventId), any(), any()); } @SmallTest