Allow additional reason/surface combinations for cancel Bubbles log REASON_GROUP_SUMMARY_CANCELED/DISMISSAL_BUBBLE, which wasn't an allowed combination. To make this more robust, only look at surface values for REASON_CANCEL, which is the only reason that needs them. Also, use wtf instead of exceptions so we can track issues without crashing. Test: atest NotificationRecordLogger Fixes: 288516227 (cherry picked from commit 80ce161ff11dd075107708ff4c64b219cc48bb6c) (cherry picked from https://googleplex-android-review.googlesource.com/q/commit:4c39ad7c8f9384a8ce3e91112a0b42ecaeeb7fb1) Merged-In: I054969dc82da5277002049d063abdb044dcc811e Change-Id: I054969dc82da5277002049d063abdb044dcc811e
diff --git a/services/core/java/com/android/server/notification/NotificationRecordLogger.java b/services/core/java/com/android/server/notification/NotificationRecordLogger.java index 0cc4fc4..f26d56e 100644 --- a/services/core/java/com/android/server/notification/NotificationRecordLogger.java +++ b/services/core/java/com/android/server/notification/NotificationRecordLogger.java
@@ -30,6 +30,7 @@ import android.os.Bundle; import android.service.notification.NotificationListenerService; import android.service.notification.NotificationStats; +import android.util.Log; import com.android.internal.logging.InstanceId; import com.android.internal.logging.UiEvent; @@ -45,6 +46,8 @@ */ interface NotificationRecordLogger { + static final String TAG = "NotificationRecordLogger"; + // The high-level interface used by clients. /** @@ -225,51 +228,40 @@ @NotificationStats.DismissalSurface int surface) { // Shouldn't be possible to get a non-dismissed notification here. if (surface == NotificationStats.DISMISSAL_NOT_DISMISSED) { - if (NotificationManagerService.DBG) { - throw new IllegalArgumentException("Unexpected surface " + surface); - } + Log.wtf(TAG, "Unexpected surface: " + surface + " with reason " + reason); return INVALID; } - // Most cancel reasons do not have a meaningful surface. Reason codes map directly - // to NotificationCancelledEvent codes. - if (surface == NotificationStats.DISMISSAL_OTHER) { + + // User cancels have a meaningful surface, which we differentiate by. See b/149038335 + // for caveats. + if (reason == REASON_CANCEL) { + switch (surface) { + case NotificationStats.DISMISSAL_PEEK: + return NOTIFICATION_CANCEL_USER_PEEK; + case NotificationStats.DISMISSAL_AOD: + return NOTIFICATION_CANCEL_USER_AOD; + case NotificationStats.DISMISSAL_SHADE: + return NOTIFICATION_CANCEL_USER_SHADE; + case NotificationStats.DISMISSAL_BUBBLE: + return NOTIFICATION_CANCEL_USER_BUBBLE; + case NotificationStats.DISMISSAL_LOCKSCREEN: + return NOTIFICATION_CANCEL_USER_LOCKSCREEN; + case NotificationStats.DISMISSAL_OTHER: + return NOTIFICATION_CANCEL_USER_OTHER; + default: + Log.wtf(TAG, "Unexpected surface: " + surface + " with reason " + reason); + return INVALID; + } + } else { if ((REASON_CLICK <= reason) && (reason <= REASON_CLEAR_DATA)) { return NotificationCancelledEvent.values()[reason]; } if (reason == REASON_ASSISTANT_CANCEL) { return NotificationCancelledEvent.NOTIFICATION_CANCEL_ASSISTANT; } - if (NotificationManagerService.DBG) { - throw new IllegalArgumentException("Unexpected cancel reason " + reason); - } + Log.wtf(TAG, "Unexpected reason: " + reason + " with surface " + surface); return INVALID; } - // User cancels have a meaningful surface, which we differentiate by. See b/149038335 - // for caveats. - if (reason != REASON_CANCEL) { - if (NotificationManagerService.DBG) { - throw new IllegalArgumentException("Unexpected cancel with surface " + reason); - } - return INVALID; - } - switch (surface) { - case NotificationStats.DISMISSAL_PEEK: - return NOTIFICATION_CANCEL_USER_PEEK; - case NotificationStats.DISMISSAL_AOD: - return NOTIFICATION_CANCEL_USER_AOD; - case NotificationStats.DISMISSAL_SHADE: - return NOTIFICATION_CANCEL_USER_SHADE; - case NotificationStats.DISMISSAL_BUBBLE: - return NOTIFICATION_CANCEL_USER_BUBBLE; - case NotificationStats.DISMISSAL_LOCKSCREEN: - return NOTIFICATION_CANCEL_USER_LOCKSCREEN; - default: - if (NotificationManagerService.DBG) { - throw new IllegalArgumentException("Unexpected surface for user-dismiss " - + reason); - } - return INVALID; - } } }
diff --git a/services/tests/uiservicestests/src/com/android/server/notification/NotificationRecordLoggerTest.java b/services/tests/uiservicestests/src/com/android/server/notification/NotificationRecordLoggerTest.java index beab107..40f34a6 100644 --- a/services/tests/uiservicestests/src/com/android/server/notification/NotificationRecordLoggerTest.java +++ b/services/tests/uiservicestests/src/com/android/server/notification/NotificationRecordLoggerTest.java
@@ -19,6 +19,15 @@ import static android.app.Notification.FLAG_FOREGROUND_SERVICE; import static android.app.NotificationManager.IMPORTANCE_DEFAULT; +import static android.service.notification.NotificationListenerService.REASON_CANCEL; +import static android.service.notification.NotificationListenerService.REASON_GROUP_SUMMARY_CANCELED; +import static android.service.notification.NotificationStats.DISMISSAL_BUBBLE; +import static android.service.notification.NotificationStats.DISMISSAL_OTHER; + +import static com.android.server.notification.NotificationRecordLogger.NotificationCancelledEvent.NOTIFICATION_CANCEL_CLICK; +import static com.android.server.notification.NotificationRecordLogger.NotificationCancelledEvent.NOTIFICATION_CANCEL_GROUP_SUMMARY_CANCELED; +import static com.android.server.notification.NotificationRecordLogger.NotificationCancelledEvent.NOTIFICATION_CANCEL_USER_OTHER; + import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotEquals; @@ -155,4 +164,18 @@ // Then: should return false assertFalse(NotificationRecordLogger.isNonDismissible(p.r)); } + + @Test + public void testBubbleGroupSummaryDismissal() { + assertEquals(NOTIFICATION_CANCEL_GROUP_SUMMARY_CANCELED, + NotificationRecordLogger.NotificationCancelledEvent.fromCancelReason( + REASON_GROUP_SUMMARY_CANCELED, DISMISSAL_BUBBLE)); + } + + @Test + public void testOtherNotificationCancel() { + assertEquals(NOTIFICATION_CANCEL_USER_OTHER, + NotificationRecordLogger.NotificationCancelledEvent.fromCancelReason( + REASON_CANCEL, DISMISSAL_OTHER)); + } }