Migrate extension notifications to the new NotificationDisplayService

chrome/browser/extensions/api/notifications/notifications_api.cc

Index: chrome/browser/extensions/api/notifications/notifications_api.cc
diff --git a/chrome/browser/extensions/api/notifications/notifications_api.cc b/chrome/browser/extensions/api/notifications/notifications_api.cc
index addcd1118d7a8cea2cffc1448aee5e18d1a88e39..1e6c040da90f7b81235bb442ea9752ed15e99c75 100644
--- a/chrome/browser/extensions/api/notifications/notifications_api.cc
+++ b/chrome/browser/extensions/api/notifications/notifications_api.cc
@@ -19,9 +19,9 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
 #include "build/build_config.h"
-#include "chrome/browser/browser_process.h"
+#include "chrome/browser/extensions/api/notifications/extension_notification_display_helper.h"
+#include "chrome/browser/extensions/api/notifications/extension_notification_display_helper_factory.h"
 #include "chrome/browser/notifications/notification.h"
-#include "chrome/browser/notifications/notification_ui_manager.h"
 #include "chrome/browser/notifications/notifier_state_tracker.h"
 #include "chrome/browser/notifications/notifier_state_tracker_factory.h"
 #include "chrome/browser/profiles/profile.h"
@@ -212,6 +212,7 @@ class NotificationsApiDelegate : public NotificationDelegate {
                            const std::string& id)
       : api_function_(api_function),
         event_router_(EventRouter::Get(profile)),
+        profile_(profile),
         extension_id_(extension_id),
         id_(id),
         scoped_id_(CreateScopedIdentifier(extension_id, id)) {
@@ -230,6 +231,10 @@ class NotificationsApiDelegate : public NotificationDelegate {
     args->AppendBoolean(by_user);
     SendEvent(events::NOTIFICATIONS_ON_CLOSED,
               notifications::OnClosed::kEventName, gesture, std::move(args));
+
+    DCHECK(profile_);
+    ExtensionNotificationDisplayHelperFactory::GetForProfile(profile_)
+        ->EraseDataForNotificationId(scoped_id_);
   }
 
   void Click() override {
@@ -302,8 +307,17 @@ class NotificationsApiDelegate : public NotificationDelegate {
   }
 
   void Shutdown() {
-    event_router_ = nullptr;
+    // Make a copy of |profile_| so that the member can be nulled out.
+    Profile* profile = profile_;
+
     shutdown_notifier_subscription_.reset();
+    event_router_ = nullptr;
+    profile_ = nullptr;
+
+    // The following call will result in deleting |this|. Do not access members
+    // after this, as you will be accessing invalid memory.
+    ExtensionNotificationDisplayHelperFactory::GetForProfile(profile)->Close(

[Miguel Garcia, 2017/02/21 10:56:43]

Wouldn't it be safer to post it as a task then?

[Peter Beverloo, 2017/02/21 17:23:50]

The issue there is that Shutdown() will be called when the profile is being shut
down. ExtensionNotificationDisplayHelperFactory requires *the profile*, so we
are unlikely to be able to access it when the task is ready to execute.

However, if we move this logic to the shutdown method of the
ExtensionNotificationDisplayHelper we avoid this problem entirely. Done.

+        scoped_id_);
   }
 
   std::unique_ptr<base::ListValue> CreateBaseEventArgs() {
@@ -318,6 +332,7 @@ class NotificationsApiDelegate : public NotificationDelegate {
   // profile-keyed service shutdown events and reset to nullptr at that time,
   // so make sure to check for a valid pointer before use.
   EventRouter* event_router_;
+  Profile* profile_;
 
   const std::string extension_id_;
   const std::string id_;
@@ -496,7 +511,7 @@ bool NotificationsApiFunction::CreateNotification(
   notification.set_never_timeout(options->require_interaction &&
                                  *options->require_interaction);
 
-  g_browser_process->notification_ui_manager()->Add(notification, GetProfile());
+  GetDisplayHelper()->Display(notification);
   return true;
 }
 
@@ -630,8 +645,10 @@ bool NotificationsApiFunction::UpdateNotification(
   if (options->is_clickable.get())
     notification->set_clickable(*options->is_clickable);
 
-  g_browser_process->notification_ui_manager()->Update(*notification,
-                                                       GetProfile());
+  // It's safe to follow the regular path for adding a new notification as it's
+  // already been verified that there is a notification that can be updated.
+  GetDisplayHelper()->Display(*notification);
+
   return true;
 }
 
@@ -652,6 +669,11 @@ bool NotificationsApiFunction::CanRunWhileDisabled() const {
   return false;
 }
 
+ExtensionNotificationDisplayHelper* NotificationsApiFunction::GetDisplayHelper()
+    const {
+  return ExtensionNotificationDisplayHelperFactory::GetForProfile(GetProfile());
+}
+
 bool NotificationsApiFunction::RunAsync() {
   if (IsNotificationsApiAvailable() && IsNotificationsApiEnabled()) {
     return RunNotificationsApi();
@@ -728,9 +750,9 @@ bool NotificationsUpdateFunction::RunNotificationsApi() {
   // We are in update.  If the ID doesn't exist, succeed but call the callback
   // with "false".
   const Notification* matched_notification =
-      g_browser_process->notification_ui_manager()->FindById(
-          CreateScopedIdentifier(extension_->id(), params_->notification_id),
-          NotificationUIManager::GetProfileID(GetProfile()));
+      GetDisplayHelper()->GetByNotificationId(
+          CreateScopedIdentifier(extension_->id(), params_->notification_id));
+
   if (!matched_notification) {
     SetResult(base::MakeUnique<base::FundamentalValue>(false));
     SendResponse(true);
@@ -767,9 +789,8 @@ bool NotificationsClearFunction::RunNotificationsApi() {
   params_ = api::notifications::Clear::Params::Create(*args_);
   EXTENSION_FUNCTION_VALIDATE(params_.get());
 
-  bool cancel_result = g_browser_process->notification_ui_manager()->CancelById(
-      CreateScopedIdentifier(extension_->id(), params_->notification_id),
-      NotificationUIManager::GetProfileID(GetProfile()));
+  bool cancel_result = GetDisplayHelper()->Close(
+      CreateScopedIdentifier(extension_->id(), params_->notification_id));
 
   SetResult(base::MakeUnique<base::FundamentalValue>(cancel_result));
   SendResponse(true);
@@ -782,11 +803,8 @@ NotificationsGetAllFunction::NotificationsGetAllFunction() {}
 NotificationsGetAllFunction::~NotificationsGetAllFunction() {}
 
 bool NotificationsGetAllFunction::RunNotificationsApi() {
-  NotificationUIManager* notification_ui_manager =
-      g_browser_process->notification_ui_manager();
   std::set<std::string> notification_ids =
-      notification_ui_manager->GetAllIdsByProfileAndSourceOrigin(
-          NotificationUIManager::GetProfileID(GetProfile()), extension_->url());
+      GetDisplayHelper()->GetNotificationIdsForExtension(extension_->url());
 
   std::unique_ptr<base::DictionaryValue> result(new base::DictionaryValue());