Move persistent notification event dispatching code to its handler

This still lived in PlatformNotificationServiceImpl but has no reason to
be there. We can move it to the PersistentNotificationHandler, which is
far more appropriately scoped for this functionality.

Change-Id: I7b7d9001fa80c196aee2953556f0609849e39d8b
Reviewed-on: https://chromium-review.googlesource.com/1127998
Commit-Queue: Peter Beverloo <peter@chromium.org>
Reviewed-by: Anita Woodruff <awdf@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584480}

chrome/browser/notifications/non_persistent_notification_handler.cc

@@ -10,6 +10,7 @@
 #include "build/build_config.h"
 #include "chrome/browser/notifications/notification_common.h"
 #include "chrome/browser/notifications/notification_permission_context.h"
+#include "chrome/browser/profiles/profile.h"
 #include "content/public/browser/notification_event_dispatcher.h"
 
 #if !defined(OS_ANDROID)

chrome/browser/notifications/persistent_notification_handler.cc

@@ -6,9 +6,26 @@
 
 #include "base/callback.h"
 #include "base/logging.h"
+#include "base/metrics/histogram_macros.h"
+#include "chrome/browser/engagement/site_engagement_service.h"
+#include "chrome/browser/notifications/metrics/notification_metrics_logger.h"
+#include "chrome/browser/notifications/metrics/notification_metrics_logger_factory.h"
+#include "chrome/browser/notifications/notification_common.h"
 #include "chrome/browser/notifications/notification_permission_context.h"
 #include "chrome/browser/notifications/platform_notification_service_impl.h"
 #include "chrome/browser/profiles/profile.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/notification_event_dispatcher.h"
+#include "content/public/browser/permission_controller.h"
+#include "content/public/browser/permission_type.h"
+#include "url/gurl.h"
+
+#if BUILDFLAG(ENABLE_BACKGROUND_MODE)
+#include "components/keep_alive_registry/keep_alive_types.h"
+#include "components/keep_alive_registry/scoped_keep_alive.h"
+#endif
+
+using content::BrowserThread;
 
 PersistentNotificationHandler::PersistentNotificationHandler() = default;
 PersistentNotificationHandler::~PersistentNotificationHandler() = default;
@@ -19,15 +36,43 @@ void PersistentNotificationHandler::OnClose(
     const std::string& notification_id,
     bool by_user,
     base::OnceClosure completed_closure) {
-  if (!by_user) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  DCHECK(origin.is_valid());
+
+  // TODO(peter): Should we do permission checks prior to forwarding to the
+  // NotificationEventDispatcher?
+
+  // If we programatically closed this notification, don't dispatch any event.
+  if (PlatformNotificationServiceImpl::GetInstance()->WasClosedProgrammatically(
+          notification_id)) {
     std::move(completed_closure).Run();
-    return;  // no need to propagate back programmatic close events
+    return;
   }
 
-  DCHECK(origin.is_valid());
+  NotificationMetricsLogger* metrics_logger =
+      NotificationMetricsLoggerFactory::GetForBrowserContext(profile);
+  if (by_user)
+    metrics_logger->LogPersistentNotificationClosedByUser();
+  else
+    metrics_logger->LogPersistentNotificationClosedProgrammatically();
 
-  PlatformNotificationServiceImpl::GetInstance()->OnPersistentNotificationClose(
-      profile, notification_id, origin, by_user, std::move(completed_closure));
+  content::NotificationEventDispatcher::GetInstance()
+      ->DispatchNotificationCloseEvent(
+          profile, notification_id, origin, by_user,
+          base::BindOnce(&PersistentNotificationHandler::OnCloseCompleted,
+                         weak_ptr_factory_.GetWeakPtr(),
+                         std::move(completed_closure)));
+}
+
+void PersistentNotificationHandler::OnCloseCompleted(
+    base::OnceClosure completed_closure,
+    content::PersistentNotificationStatus status) {
+  UMA_HISTOGRAM_ENUMERATION(
+      "Notifications.PersistentWebNotificationCloseResult", status,
+      content::PersistentNotificationStatus::
+          PERSISTENT_NOTIFICATION_STATUS_MAX);
+
+  std::move(completed_closure).Run();
 }
 
 void PersistentNotificationHandler::OnClick(
@@ -37,11 +82,70 @@ void PersistentNotificationHandler::OnClick(
     const base::Optional<int>& action_index,
     const base::Optional<base::string16>& reply,
     base::OnceClosure completed_closure) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(origin.is_valid());
 
-  PlatformNotificationServiceImpl::GetInstance()->OnPersistentNotificationClick(
-      profile, notification_id, origin, action_index, reply,
-      std::move(completed_closure));
+  NotificationMetricsLogger* metrics_logger =
+      NotificationMetricsLoggerFactory::GetForBrowserContext(profile);
+
+  blink::mojom::PermissionStatus permission_status =
+      content::BrowserContext::GetPermissionController(profile)
+          ->GetPermissionStatus(content::PermissionType::NOTIFICATIONS, origin,
+                                origin);
+
+  // Don't process click events when the |origin| doesn't have permission. This
+  // can't be a DCHECK because of potential races with native notifications.
+  if (permission_status != blink::mojom::PermissionStatus::GRANTED) {
+    metrics_logger->LogPersistentNotificationClickWithoutPermission();
+
+    std::move(completed_closure).Run();
+    return;
+  }
+
+  if (action_index.has_value())
+    metrics_logger->LogPersistentNotificationActionButtonClick();
+  else
+    metrics_logger->LogPersistentNotificationClick();
+
+#if BUILDFLAG(ENABLE_BACKGROUND_MODE)
+  // Ensure the browser stays alive while the event is processed. The keep alive
+  // will be reset when all click events have been acknowledged.
+  if (pending_click_dispatch_events_++ == 0) {
+    click_dispatch_keep_alive_.reset(
+        new ScopedKeepAlive(KeepAliveOrigin::PENDING_NOTIFICATION_CLICK_EVENT,
+                            KeepAliveRestartOption::DISABLED));
+  }
+#endif
+
+  // Notification clicks are considered a form of engagement with the |origin|,
+  // thus we log the interaction with the Site Engagement service.
+  SiteEngagementService::Get(profile)->HandleNotificationInteraction(origin);
+
+  content::NotificationEventDispatcher::GetInstance()
+      ->DispatchNotificationClickEvent(
+          profile, notification_id, origin, action_index, reply,
+          base::BindOnce(&PersistentNotificationHandler::OnClickCompleted,
+                         weak_ptr_factory_.GetWeakPtr(),
+                         std::move(completed_closure)));
+}
+
+void PersistentNotificationHandler::OnClickCompleted(
+    base::OnceClosure completed_closure,
+    content::PersistentNotificationStatus status) {
+  UMA_HISTOGRAM_ENUMERATION(
+      "Notifications.PersistentWebNotificationClickResult", status,
+      content::PersistentNotificationStatus::
+          PERSISTENT_NOTIFICATION_STATUS_MAX);
+
+#if BUILDFLAG(ENABLE_BACKGROUND_MODE)
+  DCHECK_GT(pending_click_dispatch_events_, 0);
+
+  // Reset the keep alive if all in-flight events have been processed.
+  if (--pending_click_dispatch_events_ == 0)
+    click_dispatch_keep_alive_.reset();
+#endif
+
+  std::move(completed_closure).Run();
 }
 
 void PersistentNotificationHandler::DisableNotifications(Profile* profile,

chrome/browser/notifications/persistent_notification_handler.h

@@ -5,11 +5,18 @@
 #ifndef CHROME_BROWSER_NOTIFICATIONS_PERSISTENT_NOTIFICATION_HANDLER_H_
 #define CHROME_BROWSER_NOTIFICATIONS_PERSISTENT_NOTIFICATION_HANDLER_H_
 
+#include <memory>
+
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "chrome/browser/notifications/notification_handler.h"
+#include "chrome/common/buildflags.h"
+#include "content/public/common/persistent_notification_status.h"
+
+class ScopedKeepAlive;
 
-// NotificationHandler implementation for persistent, service worker backed,
-// notifications.
+// NotificationHandler implementation for persistent Web Notifications, that is,
+// notifications associated with a Service Worker. Lives on the UI thread.
 class PersistentNotificationHandler : public NotificationHandler {
  public:
   PersistentNotificationHandler();
@@ -31,6 +38,24 @@ class PersistentNotificationHandler : public NotificationHandler {
   void OpenSettings(Profile* profile, const GURL& origin) override;
 
  private:
+  void OnCloseCompleted(base::OnceClosure completed_closure,
+                        content::PersistentNotificationStatus status);
+  void OnClickCompleted(base::OnceClosure completed_closure,
+                        content::PersistentNotificationStatus status);
+
+#if BUILDFLAG(ENABLE_BACKGROUND_MODE)
+  // Makes sure we keep the browser alive while the event in being processed.
+  // As we have no control on the click handling, the notification could be
+  // closed before a browser is brought up, thus terminating Chrome if it was
+  // the last KeepAlive. (see https://crbug.com/612815)
+  std::unique_ptr<ScopedKeepAlive> click_dispatch_keep_alive_;
+
+  // Number of in-flight notification click events.
+  int pending_click_dispatch_events_ = 0;
+#endif
+
+  base::WeakPtrFactory<PersistentNotificationHandler> weak_ptr_factory_{this};
+
   DISALLOW_COPY_AND_ASSIGN(PersistentNotificationHandler);
 };
 

chrome/browser/notifications/platform_notification_service_impl.h

@@ -7,17 +7,12 @@
 
 #include <stdint.h>
 
-#include <map>
-#include <memory>
-#include <set>
 #include <string>
 #include <unordered_set>
 
-#include "base/callback_forward.h"
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
 #include "base/memory/singleton.h"
-#include "base/optional.h"
 #include "base/strings/string16.h"
 #include "base/task/cancelable_task_tracker.h"
 #include "chrome/browser/notifications/notification_common.h"
@@ -26,21 +21,15 @@
 #include "components/history/core/browser/history_service.h"
 #include "components/history/core/browser/history_types.h"
 #include "content/public/browser/platform_notification_service.h"
-#include "content/public/common/persistent_notification_status.h"
-#include "third_party/blink/public/platform/modules/permissions/permission_status.mojom.h"
 #include "ui/message_center/public/cpp/notification.h"
 
-class NotificationDelegate;
-class ScopedKeepAlive;
+class GURL;
+class Profile;
 
 namespace content {
 class BrowserContext;
 struct NotificationResources;
-}
-
-namespace gcm {
-class PushMessagingBrowserTest;
-}
+}  // namespace content
 
 // The platform notification service is the profile-agnostic entry point through
 // which Web Notifications can be controlled.
@@ -54,26 +43,9 @@ class PlatformNotificationServiceImpl
   // be called from any thread.
   static PlatformNotificationServiceImpl* GetInstance();
 
-  // To be called when a persistent notification has been clicked on. The
-  // Service Worker associated with the registration will be started if
-  // needed, on which the event will be fired. Must be called on the UI thread.
-  void OnPersistentNotificationClick(
-      content::BrowserContext* browser_context,
-      const std::string& notification_id,
-      const GURL& origin,
-      const base::Optional<int>& action_index,
-      const base::Optional<base::string16>& reply,
-      base::OnceClosure completed_closure);
-
-  // To be called when a persistent notification has been closed. The data
-  // associated with the notification has to be pruned from the database in this
-  // case, to make sure that it continues to be in sync. Must be called on the
-  // UI thread.
-  void OnPersistentNotificationClose(content::BrowserContext* browser_context,
-                                     const std::string& notification_id,
-                                     const GURL& origin,
-                                     bool by_user,
-                                     base::OnceClosure completed_closure);
+  // Returns whether the notification identified by |notification_id| was
+  // closed programmatically through ClosePersistentNotification().
+  bool WasClosedProgrammatically(const std::string& notification_id);
 
   // content::PlatformNotificationService implementation.
   void DisplayNotification(
@@ -122,12 +94,6 @@ class PlatformNotificationServiceImpl
   PlatformNotificationServiceImpl();
   ~PlatformNotificationServiceImpl() override;
 
-  void OnClickEventDispatchComplete(
-      base::OnceClosure completed_closure,
-      content::PersistentNotificationStatus status);
-  void OnCloseEventDispatchComplete(
-      base::OnceClosure completed_closure,
-      content::PersistentNotificationStatus status);
   void OnUrlHistoryQueryComplete(const content::NotificationDatabaseData& data,
                                  bool found_url,
                                  const history::URLRow& url_row,
@@ -135,33 +101,17 @@ class PlatformNotificationServiceImpl
 
   // Creates a new Web Notification-based Notification object. Should only be
   // called when the notification is first shown.
-  // TODO(peter): |delegate| can be a scoped_refptr, but properly passing this
-  // through requires changing a whole lot of Notification constructor calls.
   message_center::Notification CreateNotificationFromData(
       Profile* profile,
       const GURL& origin,
       const std::string& notification_id,
       const content::PlatformNotificationData& notification_data,
-      const content::NotificationResources& notification_resources,
-      scoped_refptr<message_center::NotificationDelegate> delegate) const;
+      const content::NotificationResources& notification_resources) const;
 
   // Returns a display name for an origin, to be used in the context message
   base::string16 DisplayNameForContextMessage(Profile* profile,
                                               const GURL& origin) const;
 
-  void RecordSiteEngagement(content::BrowserContext* browser_context,
-                            const GURL& origin);
-
-#if BUILDFLAG(ENABLE_BACKGROUND_MODE)
-  // Makes sure we keep the browser alive while the event in being processed.
-  // As we have no control on the click handling, the notification could be
-  // closed before a browser is brought up, thus terminating Chrome if it was
-  // the last KeepAlive. (see https://crbug.com/612815)
-  std::unique_ptr<ScopedKeepAlive> click_dispatch_keep_alive_;
-
-  int pending_click_dispatch_events_;
-#endif
-
   // Tracks the id of persistent notifications that have been closed
   // programmatically to avoid dispatching close events for them.
   std::unordered_set<std::string> closed_notifications_;

chrome/browser/notifications/platform_notification_service_unittest.cc

@@ -18,6 +18,7 @@
 #include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/notifications/metrics/mock_notification_metrics_logger.h"
 #include "chrome/browser/notifications/metrics/notification_metrics_logger_factory.h"
+#include "chrome/browser/notifications/notification_display_service_impl.h"
 #include "chrome/browser/notifications/notification_display_service_tester.h"
 #include "chrome/browser/notifications/platform_notification_service_impl.h"
 #include "chrome/common/chrome_features.h"
@@ -203,24 +204,36 @@ TEST_F(PlatformNotificationServiceTest, OnPersistentNotificationClick) {
   profile_.SetNotificationPermissionStatus(
       blink::mojom::PermissionStatus::DENIED);
 
-  service()->OnPersistentNotificationClick(
-      &profile_, "jskdcjdslkcjlds", GURL("https://example.com/"), base::nullopt,
-      base::nullopt, base::DoNothing());
+  NotificationHandler* handler =
+      NotificationDisplayServiceImpl::GetForProfile(&profile_)
+          ->GetNotificationHandler(NotificationHandler::Type::WEB_PERSISTENT);
+
+  handler->OnClick(&profile_, GURL("https://example.com/"),
+                   "fake_notification_id", base::nullopt /* action_index */,
+                   base::nullopt /* reply */, base::DoNothing());
 }
 
 TEST_F(PlatformNotificationServiceTest, OnPersistentNotificationClosedByUser) {
   EXPECT_CALL(*mock_logger_, LogPersistentNotificationClosedByUser());
-  service()->OnPersistentNotificationClose(
-      &profile_, "some_random_id_123", GURL("https://example.com/"),
-      true /* by_user */, base::DoNothing());
+  NotificationHandler* handler =
+      NotificationDisplayServiceImpl::GetForProfile(&profile_)
+          ->GetNotificationHandler(NotificationHandler::Type::WEB_PERSISTENT);
+
+  handler->OnClose(&profile_, GURL("https://example.com/"),
+                   "fake_notification_id", true /* by_user */,
+                   base::DoNothing());
 }
 
 TEST_F(PlatformNotificationServiceTest,
        OnPersistentNotificationClosedProgrammatically) {
   EXPECT_CALL(*mock_logger_, LogPersistentNotificationClosedProgrammatically());
-  service()->OnPersistentNotificationClose(
-      &profile_, "some_random_id_738", GURL("https://example.com/"),
-      false /* by_user */, base::DoNothing());
+  NotificationHandler* handler =
+      NotificationDisplayServiceImpl::GetForProfile(&profile_)
+          ->GetNotificationHandler(NotificationHandler::Type::WEB_PERSISTENT);
+
+  handler->OnClose(&profile_, GURL("https://example.com/"),
+                   "fake_notification_id", false /* by_user */,
+                   base::DoNothing());
 }
 
 TEST_F(PlatformNotificationServiceTest, DisplayNonPersistentPropertiesMatch) {
@@ -468,8 +481,7 @@ TEST_F(PlatformNotificationServiceTest, CreateNotificationFromData) {
   GURL origin("https://chrome.com/");
 
   Notification notification = service()->CreateNotificationFromData(
-      &profile_, origin, "id", notification_data, NotificationResources(),
-      nullptr /* delegate */);
+      &profile_, origin, "id", notification_data, NotificationResources());
   EXPECT_TRUE(notification.context_message().empty());
 
   // Create a mocked extension.
@@ -491,7 +503,7 @@ TEST_F(PlatformNotificationServiceTest, CreateNotificationFromData) {
   notification = service()->CreateNotificationFromData(
       &profile_,
       GURL("chrome-extension://honijodknafkokifofgiaalefdiedpko/main.html"),
-      "id", notification_data, NotificationResources(), nullptr /* delegate */);
+      "id", notification_data, NotificationResources());
   EXPECT_EQ("NotificationTest",
             base::UTF16ToUTF8(notification.context_message()));
 }

chrome/browser/push_messaging/push_messaging_notification_manager.cc

@@ -172,19 +172,9 @@ void PushMessagingNotificationManager::DidGetNotificationsFromDatabase(
           kPushMessagingForcedNotificationTag)
         continue;
 
-      PlatformNotificationServiceImpl* platform_notification_service =
-          PlatformNotificationServiceImpl::GetInstance();
-
-      // First close the notification for the user's point of view, and then
-      // manually tell the service that the notification has been closed in
-      // order to avoid duplicating the thread-jump logic.
-      platform_notification_service->ClosePersistentNotification(
-          profile_, notification_database_data.notification_id);
-      platform_notification_service->OnPersistentNotificationClose(
-          profile_, notification_database_data.notification_id,
-          notification_database_data.origin, false /* by_user */,
-          base::DoNothing());
-
+      PlatformNotificationServiceImpl::GetInstance()
+          ->ClosePersistentNotification(
+              profile_, notification_database_data.notification_id);
       break;
     }
   }