From 7c6df8dad1c7c45a95b889429cf45dd0fa6bf3c5 Mon Sep 17 00:00:00 2001 From: Evan Laird Date: Wed, 18 Sep 2019 20:09:34 +0000 Subject: [PATCH] DO NOT MERGE: Revert "Force FGS notifications to show for a minimum time" This reverts commit 09843a687bc19f8e360b2f5b0493a1f378bb603b. Reason for revert: There is possible reentrant behavior when posting the safeToRemove callback after a FGS has been canceled/reposted Bug: 140948482 Change-Id: I2c5b9135e57a87f412dce8c167147dcb38f6bbae --- .../ForegroundServiceNotificationListener.java | 70 ------------------ .../statusbar/NotificationLifetimeExtender.java | 12 --- .../notification/NotificationEntryManager.java | 18 +---- .../phone/StatusBarNotificationPresenter.java | 2 +- .../ForegroundServiceNotificationListenerTest.java | 86 ---------------------- .../notification/NotificationManagerService.java | 12 +-- .../NotificationManagerServiceTest.java | 14 ---- 7 files changed, 5 insertions(+), 209 deletions(-) delete mode 100644 packages/SystemUI/tests/src/com/android/systemui/ForegroundServiceNotificationListenerTest.java diff --git a/packages/SystemUI/src/com/android/systemui/ForegroundServiceNotificationListener.java b/packages/SystemUI/src/com/android/systemui/ForegroundServiceNotificationListener.java index 46e08661cd70..96b62ac918ab 100644 --- a/packages/SystemUI/src/com/android/systemui/ForegroundServiceNotificationListener.java +++ b/packages/SystemUI/src/com/android/systemui/ForegroundServiceNotificationListener.java @@ -16,20 +16,14 @@ package com.android.systemui; -import android.annotation.NonNull; import android.app.Notification; import android.app.NotificationManager; import android.content.Context; import android.os.Bundle; -import android.os.Handler; -import android.os.Looper; import android.service.notification.StatusBarNotification; -import android.util.ArraySet; import android.util.Log; -import com.android.internal.annotations.VisibleForTesting; import com.android.internal.statusbar.NotificationVisibility; -import com.android.systemui.statusbar.NotificationLifetimeExtender; import com.android.systemui.statusbar.notification.NotificationEntryListener; import com.android.systemui.statusbar.notification.NotificationEntryManager; import com.android.systemui.statusbar.notification.collection.NotificationEntry; @@ -72,9 +66,6 @@ public class ForegroundServiceNotificationListener { removeNotification(entry.notification); } }); - - notificationEntryManager.addNotificationLifetimeExtender( - new ForegroundServiceNotificationListener.ForegroundServiceLifetimeExtender()); } /** @@ -153,65 +144,4 @@ public class ForegroundServiceNotificationListener { }, true /* create if not found */); } - - /** - * Extends the lifetime of foreground notification services such that they show for at least - * five seconds - */ - public static class ForegroundServiceLifetimeExtender implements NotificationLifetimeExtender { - - private static final String TAG = "FGSLifetimeExtender"; - @VisibleForTesting - static final int MIN_FGS_TIME_MS = 5000; - - private NotificationSafeToRemoveCallback mNotificationSafeToRemoveCallback; - private ArraySet mManagedEntries = new ArraySet<>(); - private Handler mHandler = new Handler(Looper.getMainLooper()); - - public ForegroundServiceLifetimeExtender() { - } - - @Override - public void setCallback(@NonNull NotificationSafeToRemoveCallback callback) { - mNotificationSafeToRemoveCallback = callback; - } - - @Override - public boolean shouldExtendLifetime(@NonNull NotificationEntry entry) { - if ((entry.notification.getNotification().flags - & Notification.FLAG_FOREGROUND_SERVICE) == 0) { - return false; - } - - long currentTime = System.currentTimeMillis(); - return currentTime - entry.notification.getPostTime() < MIN_FGS_TIME_MS; - } - - @Override - public boolean shouldExtendLifetimeForPendingNotification( - @NonNull NotificationEntry entry) { - return shouldExtendLifetime(entry); - } - - @Override - public void setShouldManageLifetime( - @NonNull NotificationEntry entry, boolean shouldManage) { - if (!shouldManage) { - mManagedEntries.remove(entry); - return; - } - - mManagedEntries.add(entry); - - Runnable r = () -> { - if (mManagedEntries.contains(entry)) { - if (mNotificationSafeToRemoveCallback != null) { - mNotificationSafeToRemoveCallback.onSafeToRemove(entry.key); - } - mManagedEntries.remove(entry); - } - }; - mHandler.postDelayed(r, MIN_FGS_TIME_MS); - } - } } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/NotificationLifetimeExtender.java b/packages/SystemUI/src/com/android/systemui/statusbar/NotificationLifetimeExtender.java index 48e2923c97d9..0f295ba75fe4 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/NotificationLifetimeExtender.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/NotificationLifetimeExtender.java @@ -27,18 +27,6 @@ public interface NotificationLifetimeExtender { boolean shouldExtendLifetime(@NonNull NotificationEntry entry); /** - * It's possible that a notification was canceled before it ever became visible. This callback - * gives lifetime extenders a chance to make sure it shows up. For example if a foreground - * service is canceled too quickly but we still want to make sure a FGS notification shows. - * @param pendingEntry the canceled (but pending) entry - * @return true if the notification lifetime should be extended - */ - default boolean shouldExtendLifetimeForPendingNotification( - @NonNull NotificationEntry pendingEntry) { - return false; - } - - /** * Sets whether or not the lifetime should be managed by the extender. In practice, if * shouldManage is true, this is where the extender starts managing the entry internally and is * now responsible for calling {@link NotificationSafeToRemoveCallback#onSafeToRemove(String)} diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationEntryManager.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationEntryManager.java index a37367e4bb25..f8fef7d4778c 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationEntryManager.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationEntryManager.java @@ -281,24 +281,10 @@ public class NotificationEntryManager implements } final NotificationEntry entry = mNotificationData.get(key); - boolean lifetimeExtended = false; - // Notification was canceled before it got inflated - if (entry == null) { - NotificationEntry pendingEntry = mPendingNotifications.get(key); - if (pendingEntry != null) { - for (NotificationLifetimeExtender extender : mNotificationLifetimeExtenders) { - if (extender.shouldExtendLifetimeForPendingNotification(pendingEntry)) { - extendLifetime(pendingEntry, extender); - lifetimeExtended = true; - } - } - } - } + abortExistingInflation(key); - if (!lifetimeExtended) { - abortExistingInflation(key); - } + boolean lifetimeExtended = false; if (entry != null) { // If a manager needs to keep the notification around for whatever reason, we diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarNotificationPresenter.java b/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarNotificationPresenter.java index 069219802cc3..a870590c08ac 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarNotificationPresenter.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarNotificationPresenter.java @@ -224,7 +224,7 @@ public class StatusBarNotificationPresenter implements NotificationPresenter, mVisualStabilityManager.setUpWithPresenter(this); mGutsManager.setUpWithPresenter(this, notifListContainer, mCheckSaveListener, mOnSettingsClickListener); - // ForegroundServiceNotificationListener adds its listener in its constructor + // ForegroundServiceControllerListener adds its listener in its constructor // but we need to request it here in order for it to be instantiated. // TODO: figure out how to do this correctly once Dependency.get() is gone. Dependency.get(ForegroundServiceNotificationListener.class); diff --git a/packages/SystemUI/tests/src/com/android/systemui/ForegroundServiceNotificationListenerTest.java b/packages/SystemUI/tests/src/com/android/systemui/ForegroundServiceNotificationListenerTest.java deleted file mode 100644 index 72a457e9bbbe..000000000000 --- a/packages/SystemUI/tests/src/com/android/systemui/ForegroundServiceNotificationListenerTest.java +++ /dev/null @@ -1,86 +0,0 @@ -/* - * Copyright (C) 2019 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. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package com.android.systemui; - -import static com.android.systemui.ForegroundServiceNotificationListener.ForegroundServiceLifetimeExtender.MIN_FGS_TIME_MS; - -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertTrue; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; - -import android.app.Notification; -import android.service.notification.StatusBarNotification; - -import androidx.test.filters.SmallTest; -import androidx.test.runner.AndroidJUnit4; - -import com.android.systemui.statusbar.notification.collection.NotificationEntry; - -import org.junit.Before; -import org.junit.Test; -import org.junit.runner.RunWith; - -@RunWith(AndroidJUnit4.class) -@SmallTest -public class ForegroundServiceNotificationListenerTest extends SysuiTestCase { - private ForegroundServiceNotificationListener.ForegroundServiceLifetimeExtender mExtender = - new ForegroundServiceNotificationListener.ForegroundServiceLifetimeExtender(); - private StatusBarNotification mSbn; - private NotificationEntry mEntry; - private Notification mNotif; - - @Before - public void setup() { - mNotif = new Notification.Builder(mContext, "") - .setSmallIcon(R.drawable.ic_person) - .setContentTitle("Title") - .setContentText("Text") - .build(); - - mSbn = mock(StatusBarNotification.class); - when(mSbn.getNotification()).thenReturn(mNotif); - - mEntry = new NotificationEntry(mSbn); - } - - /** - * ForegroundServiceLifetimeExtenderTest - */ - @Test - public void testShouldExtendLifetime_should_foreground() { - // Extend the lifetime of a FGS notification iff it has not been visible - // for the minimum time - mNotif.flags |= Notification.FLAG_FOREGROUND_SERVICE; - when(mSbn.getPostTime()).thenReturn(System.currentTimeMillis()); - assertTrue(mExtender.shouldExtendLifetime(mEntry)); - } - - @Test - public void testShouldExtendLifetime_shouldNot_foreground() { - mNotif.flags |= Notification.FLAG_FOREGROUND_SERVICE; - when(mSbn.getPostTime()).thenReturn(System.currentTimeMillis() - MIN_FGS_TIME_MS - 1); - assertFalse(mExtender.shouldExtendLifetime(mEntry)); - } - - @Test - public void testShouldExtendLifetime_shouldNot_notForeground() { - mNotif.flags = 0; - when(mSbn.getPostTime()).thenReturn(System.currentTimeMillis() - MIN_FGS_TIME_MS - 1); - assertFalse(mExtender.shouldExtendLifetime(mEntry)); - } -} diff --git a/services/core/java/com/android/server/notification/NotificationManagerService.java b/services/core/java/com/android/server/notification/NotificationManagerService.java index c0b26467da08..d4b3138f3829 100644 --- a/services/core/java/com/android/server/notification/NotificationManagerService.java +++ b/services/core/java/com/android/server/notification/NotificationManagerService.java @@ -5236,16 +5236,8 @@ public class NotificationManagerService extends SystemService { } synchronized (mNotificationLock) { - // If the notification is currently enqueued, repost this runnable so it has a - // chance to notify listeners - if ((findNotificationByListLocked(mEnqueuedNotifications, mPkg, mTag, mId, mUserId)) - != null) { - mHandler.post(this); - return; - } - // Look for the notification in the posted list, since we already checked enqueued. - NotificationRecord r = - findNotificationByListLocked(mNotificationList, mPkg, mTag, mId, mUserId); + // Look for the notification, searching both the posted and enqueued lists. + NotificationRecord r = findNotificationLocked(mPkg, mTag, mId, mUserId); if (r != null) { // The notification was found, check if it should be removed. diff --git a/services/tests/uiservicestests/src/com/android/server/notification/NotificationManagerServiceTest.java b/services/tests/uiservicestests/src/com/android/server/notification/NotificationManagerServiceTest.java index 5d9e0e21601b..b5e4934f49bf 100644 --- a/services/tests/uiservicestests/src/com/android/server/notification/NotificationManagerServiceTest.java +++ b/services/tests/uiservicestests/src/com/android/server/notification/NotificationManagerServiceTest.java @@ -962,20 +962,6 @@ public class NotificationManagerServiceTest extends UiServiceTestCase { } @Test - public void testCancelImmediatelyAfterEnqueueNotifiesListeners_ForegroundServiceFlag() - throws Exception { - final StatusBarNotification sbn = generateNotificationRecord(null).sbn; - sbn.getNotification().flags = - Notification.FLAG_ONGOING_EVENT | FLAG_FOREGROUND_SERVICE; - mBinderService.enqueueNotificationWithTag(PKG, PKG, "tag", - sbn.getId(), sbn.getNotification(), sbn.getUserId()); - mBinderService.cancelNotificationWithTag(PKG, "tag", sbn.getId(), sbn.getUserId()); - waitForIdle(); - verify(mListeners, times(1)).notifyPostedLocked(any(), any()); - verify(mListeners, times(1)).notifyRemovedLocked(any(), anyInt(), any()); - } - - @Test public void testUserInitiatedClearAll_noLeak() throws Exception { final NotificationRecord n = generateNotificationRecord( mTestNotificationChannel, 1, "group", true); -- 2.11.0