OSDN Git Service

DO NOT MERGE: Revert "Force FGS notifications to show for a minimum time"
authorEvan Laird <evanlaird@google.com>
Wed, 18 Sep 2019 20:09:34 +0000 (20:09 +0000)
committerEvan Laird <evanlaird@google.com>
Thu, 19 Sep 2019 13:09:06 +0000 (13:09 +0000)
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

packages/SystemUI/src/com/android/systemui/ForegroundServiceNotificationListener.java
packages/SystemUI/src/com/android/systemui/statusbar/NotificationLifetimeExtender.java
packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationEntryManager.java
packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBarNotificationPresenter.java
packages/SystemUI/tests/src/com/android/systemui/ForegroundServiceNotificationListenerTest.java [deleted file]
services/core/java/com/android/server/notification/NotificationManagerService.java
services/tests/uiservicestests/src/com/android/server/notification/NotificationManagerServiceTest.java

index 46e0866..96b62ac 100644 (file)
 
 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<NotificationEntry> 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);
-        }
-    }
 }
index 48e2923..0f295ba 100644 (file)
@@ -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)}
index a37367e..f8fef7d 100644 (file)
@@ -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
index 0692198..a870590 100644 (file)
@@ -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 (file)
index 72a457e..0000000
+++ /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));
-    }
-}
index c0b2646..d4b3138 100644 (file)
@@ -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.
 
index 5d9e0e2..b5e4934 100644 (file)
@@ -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);