Remove direct use of notification delegates from tests

The delegates will become optional in favour of notification handlers,
so simulate events through the NotificationDisplayServiceTester instead.

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

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

@@ -384,11 +384,23 @@ IN_PROC_BROWSER_TEST_F(NotificationsApiTest, TestUserGesture) {
 
   {
     UserGestureCatcher catcher;
-    notification->ButtonClick(0);
+
+    // Action button event.
+    display_service_tester_->SimulateClick(
+        NotificationHandler::Type::EXTENSION, notification->id(),
+        0 /* action_index */, base::nullopt /* reply */);
     EXPECT_TRUE(catcher.GetNextResult());
-    notification->Click();
+
+    // Click event.
+    display_service_tester_->SimulateClick(
+        NotificationHandler::Type::EXTENSION, notification->id(),
+        base::nullopt /* action_index */, base::nullopt /* reply */);
     EXPECT_TRUE(catcher.GetNextResult());
-    notification->Close(true /* by_user */);
+
+    // Close event.
+    display_service_tester_->RemoveNotification(
+        NotificationHandler::Type::EXTENSION, notification->id(),
+        true /* by_user */, false /* silent */);
     EXPECT_TRUE(catcher.GetNextResult());
 
     // Note that |notification| no longer points to valid memory.

chrome/browser/notifications/notification_display_service_tester.cc

@@ -61,6 +61,21 @@ NotificationDisplayServiceTester::GetMetadataForNotification(
   return display_service_->GetMetadataForNotification(notification);
 }
 
+void NotificationDisplayServiceTester::SimulateClick(
+    NotificationHandler::Type notification_type,
+    const std::string& notification_id,
+    base::Optional<int> action_index,
+    base::Optional<base::string16> reply) {
+  display_service_->SimulateClick(notification_type, notification_id,
+                                  std::move(action_index), std::move(reply));
+}
+
+void NotificationDisplayServiceTester::SimulateSettingsClick(
+    NotificationHandler::Type notification_type,
+    const std::string& notification_id) {
+  display_service_->SimulateSettingsClick(notification_type, notification_id);
+}
+
 void NotificationDisplayServiceTester::RemoveNotification(
     NotificationHandler::Type type,
     const std::string& notification_id,

chrome/browser/notifications/notification_display_service_tester.h

@@ -10,6 +10,7 @@
 #include "base/callback_forward.h"
 #include "base/macros.h"
 #include "base/optional.h"
+#include "base/strings/string16.h"
 #include "chrome/browser/notifications/notification_common.h"
 
 class Profile;
@@ -45,6 +46,18 @@ class NotificationDisplayServiceTester {
   base::Optional<message_center::Notification> GetNotification(
       const std::string& notification_id);
 
+  // Simulates the notification identified by |notification_id| being clicked
+  // on, optionally with the given |action_index| and |reply|.
+  void SimulateClick(NotificationHandler::Type notification_type,
+                     const std::string& notification_id,
+                     base::Optional<int> action_index,
+                     base::Optional<base::string16> reply);
+
+  // Simulates a click on the settings button of the notification identified by
+  // |notification_id|.
+  void SimulateSettingsClick(NotificationHandler::Type notification_type,
+                             const std::string& notification_id);
+
   // Simulates the notification identified by |notification_id| being closed due
   // to external events, such as the user dismissing it when |by_user| is set.
   // When |silent| is set, the notification handlers won't be informed of the

chrome/browser/notifications/platform_notification_service_interactive_uitest.cc

@@ -22,10 +22,10 @@
 #include "chrome/browser/engagement/site_engagement_service.h"
 #include "chrome/browser/notifications/desktop_notification_profile_util.h"
 #include "chrome/browser/notifications/notification_common.h"
+#include "chrome/browser/notifications/notification_display_service.h"
 #include "chrome/browser/notifications/notification_display_service_tester.h"
 #include "chrome/browser/notifications/notification_test_util.h"
 #include "chrome/browser/notifications/platform_notification_service_impl.h"
-#include "chrome/browser/notifications/web_notification_delegate.h"
 #include "chrome/browser/permissions/permission_manager.h"
 #include "chrome/browser/permissions/permission_request_manager.h"
 #include "chrome/browser/permissions/permission_result.h"
@@ -215,7 +215,6 @@ IN_PROC_BROWSER_TEST_F(PlatformNotificationServiceBrowserTest,
 
 IN_PROC_BROWSER_TEST_F(PlatformNotificationServiceBrowserTest,
                        DisplayPersistentNotificationWithPermission) {
-  base::UserActionTester user_action_tester;
   RequestAndAcceptPermission();
 
   // Expect 5 engagement for notification permission and 0.5 for the navigation.
@@ -230,12 +229,9 @@ IN_PROC_BROWSER_TEST_F(PlatformNotificationServiceBrowserTest,
       GetDisplayedNotifications(true /* is_persistent */);
   ASSERT_EQ(1u, notifications.size());
 
-#if BUILDFLAG(ENABLE_BACKGROUND)
-  ASSERT_FALSE(KeepAliveRegistry::GetInstance()->IsOriginRegistered(
-      KeepAliveOrigin::PENDING_NOTIFICATION_CLICK_EVENT));
-#endif
-
-  notifications[0].delegate()->Click();
+  display_service_tester_->SimulateClick(
+      NotificationHandler::Type::WEB_PERSISTENT, notifications[0].id(),
+      base::nullopt /* action_index */, base::nullopt /* reply */);
 
   // We expect +1 engagement for the notification interaction.
   EXPECT_DOUBLE_EQ(6.5, GetEngagementScore(GetLastCommittedURL()));
@@ -244,22 +240,12 @@ IN_PROC_BROWSER_TEST_F(PlatformNotificationServiceBrowserTest,
   notifications = GetDisplayedNotifications(true /* is_persistent */);
   ASSERT_EQ(1u, notifications.size());
 
-#if BUILDFLAG(ENABLE_BACKGROUND)
-  ASSERT_TRUE(KeepAliveRegistry::GetInstance()->IsOriginRegistered(
-      KeepAliveOrigin::PENDING_NOTIFICATION_CLICK_EVENT));
-#endif
-
   EXPECT_EQ(message_center::FullscreenVisibility::NONE,
             notifications[0].fullscreen_visibility());
 
   ASSERT_TRUE(RunScript("GetMessageFromWorker()", &script_result));
   EXPECT_EQ("action_none", script_result);
 
-#if BUILDFLAG(ENABLE_BACKGROUND)
-  ASSERT_FALSE(KeepAliveRegistry::GetInstance()->IsOriginRegistered(
-      KeepAliveOrigin::PENDING_NOTIFICATION_CLICK_EVENT));
-#endif
-
   notifications = GetDisplayedNotifications(true /* is_persistent */);
   ASSERT_EQ(1u, notifications.size());
 
@@ -367,7 +353,8 @@ IN_PROC_BROWSER_TEST_F(PlatformNotificationServiceBrowserTest,
   ASSERT_EQ(1u, notifications.size());
   EXPECT_EQ(0u, notifications[0].buttons().size());
 
-  notifications[0].delegate()->SettingsClick();
+  display_service_tester_->SimulateSettingsClick(
+      NotificationHandler::Type::WEB_PERSISTENT, notifications[0].id());
 
   // Clicking on the settings button should not close the notification.
   notifications = GetDisplayedNotifications(true /* is_persistent */);
@@ -421,7 +408,9 @@ IN_PROC_BROWSER_TEST_F(PlatformNotificationServiceBrowserTest,
       GetDisplayedNotifications(true /* is_persistent */);
   ASSERT_EQ(1u, notifications.size());
 
-  notifications[0].delegate()->Click();
+  display_service_tester_->SimulateClick(
+      NotificationHandler::Type::WEB_PERSISTENT, notifications[0].id(),
+      base::nullopt /* action_index */, base::nullopt /* reply */);
 
   // We have interacted with the button, so expect a notification bump.
   EXPECT_DOUBLE_EQ(6.5, GetEngagementScore(GetLastCommittedURL()));
@@ -435,7 +424,6 @@ IN_PROC_BROWSER_TEST_F(PlatformNotificationServiceBrowserTest,
 
 IN_PROC_BROWSER_TEST_F(PlatformNotificationServiceBrowserTest,
                        UserClosesPersistentNotification) {
-  base::UserActionTester user_action_tester;
   ASSERT_NO_FATAL_FAILURE(GrantNotificationPermissionForTest());
 
   // Expect 5 engagement for notification permission and 0.5 for the navigation.
@@ -595,7 +583,6 @@ IN_PROC_BROWSER_TEST_F(PlatformNotificationServiceBrowserTest,
 
 IN_PROC_BROWSER_TEST_F(PlatformNotificationServiceBrowserTest,
                        DisplayPersistentNotificationWithActionButtons) {
-  base::UserActionTester user_action_tester;
   ASSERT_NO_FATAL_FAILURE(GrantNotificationPermissionForTest());
 
   // Expect 5 engagement for notification permission and 0.5 for the navigation.
@@ -615,7 +602,9 @@ IN_PROC_BROWSER_TEST_F(PlatformNotificationServiceBrowserTest,
   EXPECT_EQ("actionTitle1", base::UTF16ToUTF8(notification.buttons()[0].title));
   EXPECT_EQ("actionTitle2", base::UTF16ToUTF8(notification.buttons()[1].title));
 
-  notification.delegate()->ButtonClick(0);
+  display_service_tester_->SimulateClick(
+      NotificationHandler::Type::WEB_PERSISTENT, notification.id(),
+      0 /* action_index */, base::nullopt /* reply */);
 
   ASSERT_TRUE(RunScript("GetMessageFromWorker()", &script_result));
   EXPECT_EQ("action_button_click actionId1", script_result);
@@ -627,7 +616,9 @@ IN_PROC_BROWSER_TEST_F(PlatformNotificationServiceBrowserTest,
       "Notifications.PersistentWebNotificationClickResult",
       0 /* SERVICE_WORKER_OK */, 1);
 
-  notification.delegate()->ButtonClick(1);
+  display_service_tester_->SimulateClick(
+      NotificationHandler::Type::WEB_PERSISTENT, notification.id(),
+      1 /* action_index */, base::nullopt /* reply */);
 
   ASSERT_TRUE(RunScript("GetMessageFromWorker()", &script_result));
   EXPECT_EQ("action_button_click actionId2", script_result);
@@ -642,7 +633,6 @@ IN_PROC_BROWSER_TEST_F(PlatformNotificationServiceBrowserTest,
 
 IN_PROC_BROWSER_TEST_F(PlatformNotificationServiceBrowserTest,
                        DisplayPersistentNotificationWithReplyButton) {
-  base::UserActionTester user_action_tester;
   ASSERT_NO_FATAL_FAILURE(GrantNotificationPermissionForTest());
 
   // Expect 5 engagement for notification permission and 0.5 for the navigation.
@@ -661,7 +651,9 @@ IN_PROC_BROWSER_TEST_F(PlatformNotificationServiceBrowserTest,
   ASSERT_EQ(1u, notification.buttons().size());
   EXPECT_EQ("actionTitle1", base::UTF16ToUTF8(notification.buttons()[0].title));
 
-  notification.delegate()->ButtonClickWithReply(0, base::ASCIIToUTF16("hello"));
+  display_service_tester_->SimulateClick(
+      NotificationHandler::Type::WEB_PERSISTENT, notification.id(),
+      0 /* action_index */, base::ASCIIToUTF16("hello"));
 
   ASSERT_TRUE(RunScript("GetMessageFromWorker()", &script_result));
   EXPECT_EQ("action_button_click actionId1 hello", script_result);
@@ -813,6 +805,47 @@ IN_PROC_BROWSER_TEST_F(PlatformNotificationServiceBrowserTest,
 
 #endif  // defined(OS_MACOSX)
 
+#if BUILDFLAG(ENABLE_BACKGROUND)
+IN_PROC_BROWSER_TEST_F(PlatformNotificationServiceBrowserTest,
+                       KeepAliveRegistryPendingNotificationEvent) {
+  RequestAndAcceptPermission();
+
+  std::string script_result;
+  ASSERT_TRUE(RunScript("DisplayPersistentNotification('action_none')",
+                        &script_result));
+  EXPECT_EQ("ok", script_result);
+
+  std::vector<message_center::Notification> notifications =
+      GetDisplayedNotifications(true /* is_persistent */);
+  ASSERT_EQ(1u, notifications.size());
+
+  ASSERT_FALSE(KeepAliveRegistry::GetInstance()->IsOriginRegistered(
+      KeepAliveOrigin::PENDING_NOTIFICATION_CLICK_EVENT));
+
+  NotificationDisplayService* display_service =
+      NotificationDisplayService::GetForProfile(browser()->profile());
+  NotificationHandler* handler = display_service->GetNotificationHandler(
+      NotificationHandler::Type::WEB_PERSISTENT);
+  ASSERT_TRUE(handler);
+
+  base::RunLoop run_loop;
+  handler->OnClick(browser()->profile(), notifications[0].origin_url(),
+                   notifications[0].id(), base::nullopt /* action_index */,
+                   base::nullopt /* reply */, run_loop.QuitClosure());
+
+  // The asynchronous part of the click event will still be in progress, but
+  // the keep alive registration should have been created.
+  ASSERT_TRUE(KeepAliveRegistry::GetInstance()->IsOriginRegistered(
+      KeepAliveOrigin::PENDING_NOTIFICATION_CLICK_EVENT));
+
+  // Finish the click event.
+  run_loop.Run();
+
+  ASSERT_FALSE(KeepAliveRegistry::GetInstance()->IsOriginRegistered(
+      KeepAliveOrigin::PENDING_NOTIFICATION_CLICK_EVENT));
+}
+#endif  // BUILDFLAG(ENABLE_BACKGROUND)
+
 class PlatformNotificationServiceWithoutContentImageBrowserTest
     : public PlatformNotificationServiceBrowserTest {
  public:

chrome/browser/notifications/stub_notification_display_service.cc

@@ -72,18 +72,60 @@ StubNotificationDisplayService::GetMetadataForNotification(
   return iter->metadata.get();
 }
 
+void StubNotificationDisplayService::SimulateClick(
+    NotificationHandler::Type notification_type,
+    const std::string& notification_id,
+    base::Optional<int> action_index,
+    base::Optional<base::string16> reply) {
+  auto iter = FindNotification(notification_type, notification_id);
+  if (iter == notifications_.end())
+    return;
+
+  NotificationHandler* handler = GetNotificationHandler(notification_type);
+  if (notification_type == NotificationHandler::Type::TRANSIENT) {
+    DCHECK(!handler);
+
+    auto* delegate = iter->notification.delegate();
+    if (reply.has_value()) {
+      DCHECK(action_index.has_value());
+      delegate->ButtonClickWithReply(action_index.value(), reply.value());
+    } else if (action_index.has_value()) {
+      delegate->ButtonClick(action_index.value());
+    } else {
+      delegate->Click();
+    }
+  } else {
+    DCHECK(handler);
+    base::RunLoop run_loop;
+    handler->OnClick(profile_, iter->notification.origin_url(), notification_id,
+                     action_index, reply, run_loop.QuitClosure());
+    run_loop.Run();
+  }
+}
+
+void StubNotificationDisplayService::SimulateSettingsClick(
+    NotificationHandler::Type notification_type,
+    const std::string& notification_id) {
+  auto iter = FindNotification(notification_type, notification_id);
+  if (iter == notifications_.end())
+    return;
+
+  NotificationHandler* handler = GetNotificationHandler(notification_type);
+  if (notification_type == NotificationHandler::Type::TRANSIENT) {
+    DCHECK(!handler);
+    iter->notification.delegate()->SettingsClick();
+  } else {
+    DCHECK(handler);
+    handler->OpenSettings(profile_);
+  }
+}
+
 void StubNotificationDisplayService::RemoveNotification(
     NotificationHandler::Type notification_type,
     const std::string& notification_id,
     bool by_user,
     bool silent) {
-  auto iter = std::find_if(
-      notifications_.begin(), notifications_.end(),
-      [notification_type, notification_id](const NotificationData& data) {
-        return data.type == notification_type &&
-               data.notification.id() == notification_id;
-      });
-
+  auto iter = FindNotification(notification_type, notification_id);
   if (iter == notifications_.end())
     return;
 
@@ -193,3 +235,15 @@ StubNotificationDisplayService::NotificationData::operator=(
   metadata = std::move(other.metadata);
   return *this;
 }
+
+std::vector<StubNotificationDisplayService::NotificationData>::iterator
+StubNotificationDisplayService::FindNotification(
+    NotificationHandler::Type notification_type,
+    const std::string& notification_id) {
+  return std::find_if(
+      notifications_.begin(), notifications_.end(),
+      [notification_type, notification_id](const NotificationData& data) {
+        return data.type == notification_type &&
+               data.notification.id() == notification_id;
+      });
+}

chrome/browser/notifications/stub_notification_display_service.h

@@ -44,6 +44,18 @@ class StubNotificationDisplayService : public NotificationDisplayService {
   const NotificationCommon::Metadata* GetMetadataForNotification(
       const message_center::Notification& notification);
 
+  // Simulates the notification identified by |notification_id| being clicked
+  // on, optionally with the given |action_index| and |reply|.
+  void SimulateClick(NotificationHandler::Type notification_type,
+                     const std::string& notification_id,
+                     base::Optional<int> action_index,
+                     base::Optional<base::string16> reply);
+
+  // Simulates a click on the settings button of the notification identified by
+  // |notification_id|.
+  void SimulateSettingsClick(NotificationHandler::Type notification_type,
+                             const std::string& notification_id);
+
   // Simulates the notification identified by |notification_id| being closed due
   // to external events, such as the user dismissing it when |by_user| is set.
   // Will wait for the close event to complete. When |silent| is set, the
@@ -83,6 +95,12 @@ class StubNotificationDisplayService : public NotificationDisplayService {
     std::unique_ptr<NotificationCommon::Metadata> metadata;
   };
 
+  // Returns an iterator to the notification matching the given properties. If
+  // there is no notification that matches, returns the end() iterator.
+  std::vector<NotificationData>::iterator FindNotification(
+      NotificationHandler::Type notification_type,
+      const std::string& notification_id);
+
   base::RepeatingClosure notification_added_closure_;
   std::vector<NotificationData> notifications_;
   Profile* profile_;