Remove the notification close-closure in favour of an explicit method

When showing a non-persistent notification, the caller would receive a
closure in return that could be used to close the notification after it
had been shown. This system existed for two reasons:

  1. There was an assumption that it could outlive the profile.
  2. The notification's Id was not known to the //content layer.

Both reasons have changed -- all notification code in //content and
below is explicitly associated with a renderer, and thereby profile. The
Id is generated within //content these days.

We can therefore simplify the code by removing the closure, and instead
extending ClosePersistentNotification to work with non-persistent
notifications as well.

BUG=

Change-Id: I79530f4142c9926edab46b23cdba292c1b5e50d2
Reviewed-on: https://chromium-review.googlesource.com/744205
Commit-Queue: Peter Beverloo <peter@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Anita Woodruff <awdf@chromium.org>
Cr-Commit-Position: refs/heads/master@{#512580}

chrome/browser/notifications/platform_notification_service_impl.cc

@@ -75,25 +75,6 @@ namespace {
 // permission without having an associated renderer process yet.
 const int kInvalidRenderProcessId = -1;
 
-void OnCloseNonPersistentNotificationProfileLoaded(
-    const std::string& notification_id,
-    Profile* profile) {
-  NotificationDisplayServiceFactory::GetForProfile(profile)->Close(
-      NotificationCommon::NON_PERSISTENT, notification_id);
-}
-
-// Callback used to close an non-persistent notification from blink.
-void CancelNotification(const std::string& notification_id,
-                        std::string profile_id,
-                        bool incognito) {
-  ProfileManager* profile_manager = g_browser_process->profile_manager();
-  DCHECK(profile_manager);
-  profile_manager->LoadProfile(
-      profile_id, incognito,
-      base::Bind(&OnCloseNonPersistentNotificationProfileLoaded,
-                 notification_id));
-}
-
 void ReportNotificationImageOnIOThread(
     scoped_refptr<safe_browsing::SafeBrowsingService> safe_browsing_service,
     Profile* profile,
@@ -310,8 +291,7 @@ void PlatformNotificationServiceImpl::DisplayNotification(
     const std::string& notification_id,
     const GURL& origin,
     const content::PlatformNotificationData& notification_data,
-    const content::NotificationResources& notification_resources,
-    base::Closure* cancel_callback) {
+    const content::NotificationResources& notification_resources) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   // Posted tasks can request notifications to be added, which would cause a
@@ -333,16 +313,6 @@ void PlatformNotificationServiceImpl::DisplayNotification(
 
   NotificationDisplayServiceFactory::GetForProfile(profile)->Display(
       NotificationCommon::NON_PERSISTENT, notification_id, notification);
-  if (cancel_callback) {
-#if defined(OS_WIN)
-    std::string profile_id =
-        base::WideToUTF8(profile->GetPath().BaseName().value());
-#elif defined(OS_POSIX)
-    std::string profile_id = profile->GetPath().BaseName().value();
-#endif
-    *cancel_callback = base::Bind(&CancelNotification, notification_id,
-                                  profile_id, profile->IsOffTheRecord());
-  }
 }
 
 void PlatformNotificationServiceImpl::DisplayPersistentNotification(
@@ -377,6 +347,18 @@ void PlatformNotificationServiceImpl::DisplayPersistentNotification(
   base::RecordAction(base::UserMetricsAction("Notifications.Persistent.Shown"));
 }
 
+void PlatformNotificationServiceImpl::CloseNotification(
+    BrowserContext* browser_context,
+    const std::string& notification_id) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  Profile* profile = Profile::FromBrowserContext(browser_context);
+  DCHECK(profile);
+
+  NotificationDisplayServiceFactory::GetForProfile(profile)->Close(
+      NotificationCommon::NON_PERSISTENT, notification_id);
+}
+
 void PlatformNotificationServiceImpl::ClosePersistentNotification(
     BrowserContext* browser_context,
     const std::string& notification_id) {

chrome/browser/notifications/platform_notification_service_impl.h

@@ -80,8 +80,7 @@ class PlatformNotificationServiceImpl
       const std::string& notification_id,
       const GURL& origin,
       const content::PlatformNotificationData& notification_data,
-      const content::NotificationResources& notification_resources,
-      base::Closure* cancel_callback) override;
+      const content::NotificationResources& notification_resources) override;
   void DisplayPersistentNotification(
       content::BrowserContext* browser_context,
       const std::string& notification_id,
@@ -89,6 +88,8 @@ class PlatformNotificationServiceImpl
       const GURL& origin,
       const content::PlatformNotificationData& notification_data,
       const content::NotificationResources& notification_resources) override;
+  void CloseNotification(content::BrowserContext* browser_context,
+                         const std::string& notification_id) override;
   void ClosePersistentNotification(content::BrowserContext* browser_context,
                                    const std::string& notification_id) override;
   void GetDisplayedNotifications(

chrome/browser/notifications/platform_notification_service_unittest.cc

@@ -70,19 +70,6 @@ class PlatformNotificationServiceTest : public testing::Test {
   }
 
  protected:
-  // Displays a simple, fake notification. The close closure may be specified if
-  // desired.
-  void CreateSimplePageNotificationWithCloseClosure(
-      base::Closure* close_closure) {
-    PlatformNotificationData notification_data;
-    notification_data.title = base::ASCIIToUTF16("My Notification");
-    notification_data.body = base::ASCIIToUTF16("Hello, world!");
-
-    service()->DisplayNotification(
-        profile_, kNotificationId, GURL("https://chrome.com/"),
-        notification_data, NotificationResources(), close_closure);
-  }
-
   // Returns the Platform Notification Service these unit tests are for.
   PlatformNotificationServiceImpl* service() const {
     return PlatformNotificationServiceImpl::GetInstance();
@@ -110,21 +97,22 @@ class PlatformNotificationServiceTest : public testing::Test {
   std::unique_ptr<NotificationDisplayServiceTester> display_service_tester_;
 };
 
+TEST_F(PlatformNotificationServiceTest, DisplayNonPersistentClosure) {
+  PlatformNotificationData notification_data;
+  notification_data.title = base::ASCIIToUTF16("My Notification");
+  notification_data.body = base::ASCIIToUTF16("Hello, world!");
 
-TEST_F(PlatformNotificationServiceTest, DisplayPageCloseClosure) {
-  base::Closure close_closure;
-  CreateSimplePageNotificationWithCloseClosure(&close_closure);
+  service()->DisplayNotification(profile_, kNotificationId,
+                                 GURL("https://chrome.com/"), notification_data,
+                                 NotificationResources());
 
   EXPECT_EQ(1u,
             GetNotificationCountForType(NotificationCommon::NON_PERSISTENT));
 
-  ASSERT_FALSE(close_closure.is_null());
-  close_closure.Run();
+  service()->CloseNotification(profile_, kNotificationId);
 
   EXPECT_EQ(0u,
             GetNotificationCountForType(NotificationCommon::NON_PERSISTENT));
-  // Note that we cannot verify whether the closed event was called on the
-  // delegate given that it'd result in a use-after-free.
 }
 
 TEST_F(PlatformNotificationServiceTest, PersistentNotificationDisplay) {
@@ -163,7 +151,7 @@ TEST_F(PlatformNotificationServiceTest, DisplayPageNotificationMatches) {
 
   service()->DisplayNotification(profile_, kNotificationId,
                                  GURL("https://chrome.com/"), notification_data,
-                                 NotificationResources(), nullptr);
+                                 NotificationResources());
 
   ASSERT_EQ(1u,
             GetNotificationCountForType(NotificationCommon::NON_PERSISTENT));

content/browser/notifications/notification_event_dispatcher_impl.cc

@@ -455,12 +455,9 @@ void NotificationEventDispatcherImpl::DispatchNonPersistentCloseEvent(
   // closed.
   if (!sender)
     return;
+
   sender->Send(new PlatformNotificationMsg_DidClose(
       non_persistent_ids_[notification_id]));
-
-  static_cast<RenderProcessHostImpl*>(sender)
-      ->notification_message_filter()
-      ->DidCloseNotification(notification_id);
 }
 
 void NotificationEventDispatcherImpl::RendererGone(int renderer_id) {

content/browser/notifications/notification_message_filter.cc

@@ -98,12 +98,6 @@ NotificationMessageFilter::NotificationMessageFilter(
 
 NotificationMessageFilter::~NotificationMessageFilter() = default;
 
-void NotificationMessageFilter::DidCloseNotification(
-    const std::string& notification_id) {
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  close_closures_.erase(notification_id);
-}
-
 void NotificationMessageFilter::OnDestruct() const {
   if (non_persistent__notification_shown_) {
     NotificationEventDispatcherImpl* event_dispatcher =
@@ -172,13 +166,9 @@ void NotificationMessageFilter::OnShowPlatformNotification(
   event_dispatcher->RegisterNonPersistentNotification(
       notification_id, process_id_, non_persistent_notification_id);
 
-  base::Closure close_closure;
   service->DisplayNotification(browser_context_, notification_id, origin,
                                SanitizeNotificationData(notification_data),
-                               notification_resources, &close_closure);
-
-  if (!close_closure.is_null())
-    close_closures_[notification_id] = close_closure;
+                               notification_resources);
 }
 
 void NotificationMessageFilter::OnShowPersistentNotification(
@@ -328,11 +318,11 @@ void NotificationMessageFilter::OnClosePlatformNotification(
       GetNotificationIdGenerator()->GenerateForNonPersistentNotification(
           origin, tag, non_persistent_notification_id, process_id_);
 
-  if (!close_closures_.count(notification_id))
-    return;
+  PlatformNotificationService* service =
+      GetContentClient()->browser()->GetPlatformNotificationService();
+  DCHECK(service);
 
-  close_closures_[notification_id].Run();
-  close_closures_.erase(notification_id);
+  service->CloseNotification(browser_context_, notification_id);
 }
 
 void NotificationMessageFilter::OnClosePersistentNotification(

content/browser/notifications/notification_message_filter.h

@@ -7,10 +7,8 @@
 
 #include <stdint.h>
 
-#include <map>
 #include <vector>
 
-#include "base/callback_forward.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "content/common/service_worker/service_worker_status_code.h"
@@ -41,10 +39,6 @@ class NotificationMessageFilter : public BrowserMessageFilter {
       const scoped_refptr<ServiceWorkerContextWrapper>& service_worker_context,
       BrowserContext* browser_context);
 
-  // To be called by non-persistent notification delegates when they are closed,
-  // so that the close closure associated with that notification can be removed.
-  void DidCloseNotification(const std::string& notification_id);
-
   // BrowserMessageFilter implementation. Called on the UI thread.
   void OnDestruct() const override;
   bool OnMessageReceived(const IPC::Message& message) override;
@@ -140,10 +134,6 @@ class NotificationMessageFilter : public BrowserMessageFilter {
   scoped_refptr<ServiceWorkerContextWrapper> service_worker_context_;
   BrowserContext* browser_context_;
 
-  // Map mapping notification IDs associated with non-persistent notifications
-  // to the closures that may be used for programmatically closing them.
-  std::unordered_map<std::string, base::Closure> close_closures_;
-
   base::WeakPtrFactory<NotificationMessageFilter> weak_factory_io_;
 
   DISALLOW_COPY_AND_ASSIGN(NotificationMessageFilter);

content/browser/notifications/platform_notification_context_unittest.cc

@@ -32,7 +32,8 @@ const int64_t kFakeServiceWorkerRegistrationId = 42;
 class NotificationBrowserClient : public TestContentBrowserClient {
  public:
   NotificationBrowserClient()
-      : platform_notification_service_(new MockPlatformNotificationService()) {}
+      : platform_notification_service_(
+            std::make_unique<MockPlatformNotificationService>()) {}
 
   PlatformNotificationService* GetPlatformNotificationService() override {
     return platform_notification_service_.get();

content/public/browser/platform_notification_service.h

@@ -53,16 +53,14 @@ class CONTENT_EXPORT PlatformNotificationService {
       const GURL& origin,
       int render_process_id) = 0;
 
-  // Displays the notification described in |notification_data| to the user. A
-  // closure through which the notification can be closed will be stored in the
-  // |cancel_callback| argument. This method must be called on the UI thread.
+  // Displays the notification described in |notification_data| to the user.
+  // This method must be called on the UI thread.
   virtual void DisplayNotification(
       BrowserContext* browser_context,
       const std::string& notification_id,
       const GURL& origin,
       const PlatformNotificationData& notification_data,
-      const NotificationResources& notification_resources,
-      base::Closure* cancel_callback) = 0;
+      const NotificationResources& notification_resources) = 0;
 
   // Displays the persistent notification described in |notification_data| to
   // the user. This method must be called on the UI thread.
@@ -74,8 +72,13 @@ class CONTENT_EXPORT PlatformNotificationService {
       const PlatformNotificationData& notification_data,
       const NotificationResources& notification_resources) = 0;
 
-  // Closes the persistent notification identified by
-  // |persistent_notification_id|. This method must be called on the UI thread.
+  // Closes the notification identified by |notification_id|. This method must
+  // be called on the UI thread.
+  virtual void CloseNotification(BrowserContext* browser_context,
+                                 const std::string& notification_id) = 0;
+
+  // Closes the persistent notification identified by |notification_id|. This
+  // method must be called on the UI thread.
   virtual void ClosePersistentNotification(
       BrowserContext* browser_context,
       const std::string& notification_id) = 0;

content/test/mock_platform_notification_service.cc

@@ -22,23 +22,17 @@ void OnEventDispatchComplete(PersistentNotificationStatus status) {}
 
 }  // namespace
 
-MockPlatformNotificationService::MockPlatformNotificationService()
-    : weak_factory_(this) {}
+MockPlatformNotificationService::MockPlatformNotificationService() = default;
 
-MockPlatformNotificationService::~MockPlatformNotificationService() {}
+MockPlatformNotificationService::~MockPlatformNotificationService() = default;
 
 void MockPlatformNotificationService::DisplayNotification(
     BrowserContext* browser_context,
     const std::string& notification_id,
     const GURL& origin,
     const PlatformNotificationData& notification_data,
-    const NotificationResources& notification_resources,
-    base::Closure* cancel_callback) {
+    const NotificationResources& notification_resources) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  DCHECK(cancel_callback);
-
-  *cancel_callback = base::Bind(&MockPlatformNotificationService::Close,
-                                weak_factory_.GetWeakPtr(), notification_id);
 
   ReplaceNotificationIfNeeded(notification_id);
   non_persistent_notifications_.insert(notification_id);
@@ -70,6 +64,22 @@ void MockPlatformNotificationService::DisplayPersistentNotification(
       notification_id;
 }
 
+void MockPlatformNotificationService::CloseNotification(
+    BrowserContext* browser_context,
+    const std::string& notification_id) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  const auto non_persistent_iter =
+      non_persistent_notifications_.find(notification_id);
+  if (non_persistent_iter == non_persistent_notifications_.end())
+    return;
+
+  NotificationEventDispatcher::GetInstance()->DispatchNonPersistentCloseEvent(
+      notification_id);
+
+  non_persistent_notifications_.erase(non_persistent_iter);
+}
+
 void MockPlatformNotificationService::ClosePersistentNotification(
     BrowserContext* browser_context,
     const std::string& notification_id) {
@@ -162,18 +172,6 @@ MockPlatformNotificationService::CheckPermissionOnIOThread(
   return CheckPermission(origin);
 }
 
-void MockPlatformNotificationService::Close(
-    const std::string& notification_id) {
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  const auto non_persistent_iter =
-      non_persistent_notifications_.find(notification_id);
-  if (non_persistent_iter == non_persistent_notifications_.end()) {
-    NotificationEventDispatcher::GetInstance()->DispatchNonPersistentCloseEvent(
-        notification_id);
-    non_persistent_notifications_.erase(non_persistent_iter);
-  }
-}
-
 void MockPlatformNotificationService::ReplaceNotificationIfNeeded(
     const std::string& notification_id) {
   persistent_notifications_.erase(notification_id);

content/test/mock_platform_notification_service.h

@@ -12,7 +12,6 @@
 
 #include "base/callback.h"
 #include "base/macros.h"
-#include "base/memory/weak_ptr.h"
 #include "base/optional.h"
 #include "base/strings/string16.h"
 #include "content/public/browser/platform_notification_service.h"
@@ -56,8 +55,7 @@ class MockPlatformNotificationService : public PlatformNotificationService {
       const std::string& notification_id,
       const GURL& origin,
       const PlatformNotificationData& notification_data,
-      const NotificationResources& notification_resources,
-      base::Closure* cancel_callback) override;
+      const NotificationResources& notification_resources) override;
   void DisplayPersistentNotification(
       BrowserContext* browser_context,
       const std::string& notification_id,
@@ -65,6 +63,8 @@ class MockPlatformNotificationService : public PlatformNotificationService {
       const GURL& origin,
       const PlatformNotificationData& notification_data,
       const NotificationResources& notification_resources) override;
+  void CloseNotification(BrowserContext* browser_context,
+                         const std::string& notification_id) override;
   void ClosePersistentNotification(BrowserContext* browser_context,
                                    const std::string& notification_id) override;
   void GetDisplayedNotifications(
@@ -83,9 +83,6 @@ class MockPlatformNotificationService : public PlatformNotificationService {
     GURL origin;
   };
 
-  // Closes the notification titled |title|. Must be called on the UI thread.
-  void Close(const std::string& title);
-
   // Fakes replacing the notification identified by |notification_id|. Both
   // persistent and non-persistent notifications will be considered for this.
   void ReplaceNotificationIfNeeded(const std::string& notification_id);
@@ -97,8 +94,6 @@ class MockPlatformNotificationService : public PlatformNotificationService {
   // Mapping of titles to notification ids giving test a usable identifier.
   std::unordered_map<std::string, std::string> notification_id_map_;
 
-  base::WeakPtrFactory<MockPlatformNotificationService> weak_factory_;
-
   DISALLOW_COPY_AND_ASSIGN(MockPlatformNotificationService);
 };