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

https://chromium-review.googlesource.com/c/chromium/src/+/744205

atom/browser/api/atom_api_notification.cc

@@ -9,6 +9,7 @@
 #include "atom/common/native_mate_converters/gfx_converter.h"
 #include "atom/common/native_mate_converters/image_converter.h"
 #include "atom/common/native_mate_converters/string16_converter.h"
+#include "base/guid.h"
 #include "base/strings/utf_string_conversions.h"
 #include "brightray/browser/browser_client.h"
 #include "native_mate/constructor.h"
@@ -197,7 +198,7 @@ void Notification::Close() {
 void Notification::Show() {
   Close();
   if (presenter_) {
-    notification_ = presenter_->CreateNotification(this);
+    notification_ = presenter_->CreateNotification(this, base::GenerateGUID());
     if (notification_) {
       brightray::NotificationOptions options;
       options.title = title_;

brightray/browser/notification.h

@@ -64,8 +64,11 @@ class Notification {
   }
 
   void set_delegate(NotificationDelegate* delegate) { delegate_ = delegate; }
+  void set_notification_id(const std::string& id) { notification_id_ = id; }
+
   NotificationDelegate* delegate() const { return delegate_; }
   NotificationPresenter* presenter() const { return presenter_; }
+  const std::string& notification_id() const { return notification_id_; }
 
  protected:
   Notification(NotificationDelegate* delegate,
@@ -74,6 +77,7 @@ class Notification {
  private:
   NotificationDelegate* delegate_;
   NotificationPresenter* presenter_;
+  std::string notification_id_;
 
   base::WeakPtrFactory<Notification> weak_factory_;
 

brightray/browser/notification_presenter.cc

@@ -16,8 +16,10 @@ NotificationPresenter::~NotificationPresenter() {
 }
 
 base::WeakPtr<Notification> NotificationPresenter::CreateNotification(
-    NotificationDelegate* delegate) {
+    NotificationDelegate* delegate,
+    const std::string& notification_id) {
   Notification* notification = CreateNotificationObject(delegate);
+  notification->set_notification_id(notification_id);
   notifications_.insert(notification);
   return notification->GetWeakPtr();
 }
@@ -27,4 +29,14 @@ void NotificationPresenter::RemoveNotification(Notification* notification) {
   delete notification;
 }
 
+void NotificationPresenter::CloseNotificationWithId(
+    const std::string& notification_id) {
+  auto it = std::find_if(notifications_.begin(), notifications_.end(),
+                         [&notification_id](const Notification* n) {
+                           return n->notification_id() == notification_id;
+                         });
+  if (it != notifications_.end())
+    (*it)->Dismiss();
+}
+
 }  // namespace brightray

brightray/browser/notification_presenter.h

@@ -6,6 +6,7 @@
 #define BRIGHTRAY_BROWSER_NOTIFICATION_PRESENTER_H_
 
 #include <set>
+#include <string>
 
 #include "base/memory/weak_ptr.h"
 
@@ -21,7 +22,9 @@ class NotificationPresenter {
   virtual ~NotificationPresenter();
 
   base::WeakPtr<Notification> CreateNotification(
-      NotificationDelegate* delegate);
+      NotificationDelegate* delegate,
+      const std::string& notification_id);
+  void CloseNotificationWithId(const std::string& notification_id);
 
   std::set<Notification*> notifications() const { return notifications_; }
 

brightray/browser/platform_notification_service.cc

@@ -18,11 +18,6 @@ namespace brightray {
 
 namespace {
 
-void RemoveNotification(base::WeakPtr<Notification> notification) {
-  if (notification)
-    notification->Dismiss();
-}
-
 void OnWebNotificationAllowed(base::WeakPtr<Notification> notification,
                               const SkBitmap& icon,
                               const content::PlatformNotificationData& data,
@@ -103,16 +98,14 @@ void PlatformNotificationService::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) {
   auto* presenter = browser_client_->GetNotificationPresenter();
   if (!presenter)
     return;
   NotificationDelegateImpl* delegate =
       new NotificationDelegateImpl(notification_id);
-  auto notification = presenter->CreateNotification(delegate);
+  auto notification = presenter->CreateNotification(delegate, notification_id);
   if (notification) {
-    *cancel_callback = base::Bind(&RemoveNotification, notification);
     browser_client_->WebNotificationAllowed(
         render_process_id_, base::Bind(&OnWebNotificationAllowed, notification,
                                        notification_resources.notification_icon,
@@ -132,6 +125,15 @@ void PlatformNotificationService::ClosePersistentNotification(
     content::BrowserContext* browser_context,
     const std::string& notification_id) {}
 
+void PlatformNotificationService::CloseNotification(
+    content::BrowserContext* browser_context,
+    const std::string& notification_id) {
+  auto presenter = browser_client_->GetNotificationPresenter();
+  if (!presenter)
+    return;
+  presenter->CloseNotificationWithId(notification_id);
+}
+
 void PlatformNotificationService::GetDisplayedNotifications(
     content::BrowserContext* browser_context,
     const DisplayedNotificationsCallback& callback) {}

brightray/browser/platform_notification_service.h

@@ -36,8 +36,7 @@ class PlatformNotificationService
       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,
@@ -47,6 +46,8 @@ class PlatformNotificationService
       const content::NotificationResources& notification_resources) override;
   void ClosePersistentNotification(content::BrowserContext* browser_context,
                                    const std::string& notification_id) override;
+  void CloseNotification(content::BrowserContext* browser_context,
+                         const std::string& notification_id) override;
   void GetDisplayedNotifications(
       content::BrowserContext* browser_context,
       const DisplayedNotificationsCallback& callback) override;