Only close notification if it is still displayed.

This is an optimization to only try to close a notification after
deleting its data, if we believe that it is still on screen. If the
notification got closed by the user for example, we do not need to
explicitly close it again as it is already gone.

Bug: 962885
Change-Id: Id949ff4a15623070d6781e404d873361c433cf55
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1609852
Commit-Queue: Richard Knoll <knollr@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#659972}

chrome/browser/push_messaging/push_messaging_notification_manager.cc

@@ -175,7 +175,7 @@ void PushMessagingNotificationManager::DidGetNotificationsFromDatabase(
               ->GetPlatformNotificationContext();
       notification_context->DeleteNotificationData(
           notification_database_data.notification_id, origin,
-          base::DoNothing());
+          /* close_notification= */ true, base::DoNothing());
       break;
     }
   }

content/browser/notifications/blink_notification_service_impl.cc

@@ -269,7 +269,8 @@ void BlinkNotificationServiceImpl::ClosePersistentNotification(
     return;
 
   notification_context_->DeleteNotificationData(
-      notification_id, origin_.GetURL(), base::DoNothing());
+      notification_id, origin_.GetURL(), /* close_notification= */ true,
+      base::DoNothing());
 }
 
 void BlinkNotificationServiceImpl::GetNotifications(

content/browser/notifications/notification_event_dispatcher_impl.cc

@@ -96,7 +96,6 @@ void ServiceWorkerNotificationEventFinished(
 // called on the IO thread.
 void DispatchNotificationEventOnRegistration(
     const NotificationDatabaseData& notification_database_data,
-    const scoped_refptr<PlatformNotificationContext>& notification_context,
     NotificationOperationCallback dispatch_event_action,
     NotificationDispatchCompleteCallback dispatch_complete_callback,
     blink::ServiceWorkerStatusCode service_worker_status,
@@ -158,7 +157,6 @@ void DispatchNotificationEventOnRegistration(
 void FindServiceWorkerRegistration(
     const GURL& origin,
     const scoped_refptr<ServiceWorkerContextWrapper>& service_worker_context,
-    const scoped_refptr<PlatformNotificationContext>& notification_context,
     NotificationOperationCallback notification_action_callback,
     NotificationDispatchCompleteCallback dispatch_complete_callback,
     bool success,
@@ -178,14 +176,14 @@ void FindServiceWorkerRegistration(
 
   base::PostTaskWithTraits(
       FROM_HERE, {BrowserThread::IO},
-      base::BindOnce(
-          &ServiceWorkerContextWrapper::FindReadyRegistrationForId,
-          service_worker_context,
-          notification_database_data.service_worker_registration_id, origin,
-          base::BindOnce(&DispatchNotificationEventOnRegistration,
-                         notification_database_data, notification_context,
-                         std::move(notification_action_callback),
-                         std::move(dispatch_complete_callback))));
+      base::BindOnce(&ServiceWorkerContextWrapper::FindReadyRegistrationForId,
+                     service_worker_context,
+                     notification_database_data.service_worker_registration_id,
+                     origin,
+                     base::BindOnce(&DispatchNotificationEventOnRegistration,
+                                    notification_database_data,
+                                    std::move(notification_action_callback),
+                                    std::move(dispatch_complete_callback))));
 }
 
 // Reads the data associated with the |notification_id| belonging to |origin|
@@ -202,7 +200,7 @@ void ReadNotificationDatabaseData(
   notification_context->ReadNotificationDataAndRecordInteraction(
       notification_id, origin, interaction,
       base::BindOnce(&FindServiceWorkerRegistration, origin,
-                     service_worker_context, notification_context,
+                     service_worker_context,
                      std::move(notification_read_callback),
                      std::move(dispatch_complete_callback)));
 }
@@ -287,6 +285,7 @@ void DeleteNotificationDataFromDatabase(
       base::BindOnce(
           &PlatformNotificationContext::DeleteNotificationData,
           notification_context, notification_id, origin,
+          /* close_notification= */ false,
           base::BindOnce(&OnPersistentNotificationDataDeleted, status_code,
                          std::move(dispatch_complete_callback))));
 }

content/browser/notifications/platform_notification_context_impl.cc

@@ -757,13 +757,15 @@ void PlatformNotificationContextImpl::DoWriteNotificationData(
 void PlatformNotificationContextImpl::DeleteNotificationData(
     const std::string& notification_id,
     const GURL& origin,
+    bool close_notification,
     DeleteResultCallback callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   if (!service_proxy_)
     return;
 
   // Close notification as we're about to delete its data.
-  service_proxy_->CloseNotification(notification_id);
+  if (close_notification)
+    service_proxy_->CloseNotification(notification_id);
 
   LazyInitialize(
       base::BindOnce(&PlatformNotificationContextImpl::DoDeleteNotificationData,

content/browser/notifications/platform_notification_context_impl.h

@@ -93,6 +93,7 @@ class CONTENT_EXPORT PlatformNotificationContextImpl
                              WriteResultCallback callback) override;
   void DeleteNotificationData(const std::string& notification_id,
                               const GURL& origin,
+                              bool close_notification,
                               DeleteResultCallback callback) override;
   void DeleteAllNotificationDataForBlockedOrigins(
       DeleteAllResultCallback callback) override;

content/browser/notifications/platform_notification_context_unittest.cc

@@ -311,6 +311,7 @@ TEST_F(PlatformNotificationContextTest, DeleteInvalidNotification) {
 
   context->DeleteNotificationData(
       "invalid-notification-id", GURL("https://example.com"),
+      /* close_notification= */ false,
       base::BindOnce(
           &PlatformNotificationContextTest::DidDeleteNotificationData,
           base::Unretained(this)));
@@ -344,6 +345,7 @@ TEST_F(PlatformNotificationContextTest, DeleteNotification) {
 
   context->DeleteNotificationData(
       notification_id(), origin,
+      /* close_notification= */ false,
       base::BindOnce(
           &PlatformNotificationContextTest::DidDeleteNotificationData,
           base::Unretained(this)));
@@ -392,6 +394,7 @@ TEST_F(PlatformNotificationContextTest, DeleteClosesNotification) {
 
   context->DeleteNotificationData(
       notification_id(), origin,
+      /* close_notification= */ true,
       base::BindOnce(
           &PlatformNotificationContextTest::DidDeleteNotificationData,
           base::Unretained(this)));

content/public/browser/platform_notification_context.h

@@ -107,10 +107,12 @@ class PlatformNotificationContext
       WriteResultCallback callback) = 0;
 
   // Deletes all data associated with |notification_id| belonging to |origin|
-  // from the database. |callback| will be invoked with the success status
-  // when the operation has completed.
+  // from the database. Closes the notification if |close_notification| is true.
+  // |callback| will be invoked with the success status when the operation has
+  // completed.
   virtual void DeleteNotificationData(const std::string& notification_id,
                                       const GURL& origin,
+                                      bool close_notification,
                                       DeleteResultCallback callback) = 0;
 
   // Checks permissions for all notifications in the database and deletes all