Minimize the delegate dependencies for native extension notifications.

Support for events like click, close etc is now fully handled by the
ExtensionNotificationHandler like persistent web notifications.

The delegate is now just a little shim to hold the delegate id since it
is still required by the Notification class.

BUG=720345

Review-Url: https://codereview.chromium.org/2875673002
Cr-Commit-Position: refs/heads/master@{#473427}

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

@@ -5,12 +5,119 @@
 #include "chrome/browser/extensions/api/notifications/extension_notification_handler.h"
 
 #include "base/logging.h"
+#include "base/strings/nullable_string16.h"
+#include "base/strings/string_piece.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/profiles/profile.h"
+#include "chrome/common/extensions/api/notifications.h"
+#include "extensions/common/constants.h"
+#include "url/gurl.h"
+
+namespace extensions {
+
+namespace {
+
+std::string GetExtensionId(const std::string& extension_url) {
+  GURL url(extension_url);
+  if (!url.is_valid() || !url.SchemeIs(extensions::kExtensionScheme))
+    return "";
+  return url.GetOrigin().host_piece().as_string();
+}
+
+std::unique_ptr<base::ListValue> CreateBaseEventArgs(
+    const std::string& extension_id,
+    const std::string& scoped_notification_id) {
+  // Unscope the notification id before returning it.
+  size_t index_of_separator = extension_id.length() + 1;
+  DCHECK_LT(index_of_separator, scoped_notification_id.length());
+  std::string unscoped_notification_id =
+      scoped_notification_id.substr(index_of_separator);
+
+  std::unique_ptr<base::ListValue> args(new base::ListValue());
+  args->AppendString(unscoped_notification_id);
+  return args;
+}
+
+}  // namespace
 
 ExtensionNotificationHandler::ExtensionNotificationHandler() = default;
 
 ExtensionNotificationHandler::~ExtensionNotificationHandler() = default;
 
+void ExtensionNotificationHandler::OnClose(Profile* profile,
+                                           const std::string& origin,
+                                           const std::string& notification_id,
+                                           bool by_user) {
+  EventRouter::UserGestureState gesture =
+      by_user ? EventRouter::USER_GESTURE_ENABLED
+              : EventRouter::USER_GESTURE_NOT_ENABLED;
+  std::string extension_id(GetExtensionId(origin));
+  DCHECK(!extension_id.empty());
+
+  std::unique_ptr<base::ListValue> args(
+      CreateBaseEventArgs(extension_id, notification_id));
+  args->AppendBoolean(by_user);
+  SendEvent(profile, extension_id, events::NOTIFICATIONS_ON_CLOSED,
+            api::notifications::OnClosed::kEventName, gesture, std::move(args));
+
+  ExtensionNotificationDisplayHelper* display_helper =
+      ExtensionNotificationDisplayHelperFactory::GetForProfile(profile);
+  if (display_helper)
+    display_helper->EraseDataForNotificationId(notification_id);
+}
+
+void ExtensionNotificationHandler::OnClick(
+    Profile* profile,
+    const std::string& origin,
+    const std::string& notification_id,
+    int action_index,
+    const base::NullableString16& reply) {
+  DCHECK(reply.is_null());
+
+  std::string extension_id(GetExtensionId(origin));
+  std::unique_ptr<base::ListValue> args(
+      CreateBaseEventArgs(extension_id, notification_id));
+  if (action_index > -1)
+    args->AppendInteger(action_index);
+  events::HistogramValue histogram_value =
+      action_index > -1 ? events::NOTIFICATIONS_ON_BUTTON_CLICKED
+                        : events::NOTIFICATIONS_ON_CLICKED;
+  const std::string& event_name =
+      action_index > -1 ? api::notifications::OnButtonClicked::kEventName
+                        : api::notifications::OnClicked::kEventName;
+
+  SendEvent(profile, extension_id, histogram_value, event_name,
+            EventRouter::USER_GESTURE_ENABLED, std::move(args));
+}
+
 void ExtensionNotificationHandler::OpenSettings(Profile* profile) {
   // Extension notifications don't display a settings button.
   NOTREACHED();
 }
+
+void ExtensionNotificationHandler::RegisterNotification(
+    const std::string& notification_id,
+    NotificationDelegate* delegate) {}
+
+void ExtensionNotificationHandler::SendEvent(
+    Profile* profile,
+    const std::string& extension_id,
+    events::HistogramValue histogram_value,
+    const std::string& event_name,
+    EventRouter::UserGestureState user_gesture,
+    std::unique_ptr<base::ListValue> args) {
+  if (extension_id.empty())
+    return;
+
+  EventRouter* event_router = EventRouter::Get(profile);
+  if (!event_router)
+    return;
+
+  std::unique_ptr<Event> event(
+      new Event(histogram_value, event_name, std::move(args)));
+  event->user_gesture = user_gesture;
+  event_router->DispatchEventToExtension(extension_id, std::move(event));
+}
+
+}  // namespace extensions

chrome/browser/extensions/api/notifications/extension_notification_handler.h

@@ -6,20 +6,46 @@
 #define CHROME_BROWSER_EXTENSIONS_API_NOTIFICATIONS_EXTENSION_NOTIFICATION_HANDLER_H_
 
 #include "base/macros.h"
-#include "chrome/browser/notifications/non_persistent_notification_handler.h"
+#include "chrome/browser/notifications/notification_handler.h"
+#include "extensions/browser/event_router.h"
+
+class Profile;
+
+namespace extensions {
 
 // Handler for notifications shown by extensions. Will be created and owned by
-// the NotificationDisplayService.
-class ExtensionNotificationHandler : public NonPersistentNotificationHandler {
+// the NativeNotificationDisplayService.
+class ExtensionNotificationHandler : public NotificationHandler {
  public:
   ExtensionNotificationHandler();
   ~ExtensionNotificationHandler() override;
 
   // NotificationHandler implementation.
+  void OnClose(Profile* profile,
+               const std::string& origin,
+               const std::string& notification_id,
+               bool by_user) override;
+  void OnClick(Profile* profile,
+               const std::string& origin,
+               const std::string& notification_id,
+               int action_index,
+               const base::NullableString16& reply) override;
   void OpenSettings(Profile* profile) override;
+  void RegisterNotification(const std::string& notification_id,
+                            NotificationDelegate* delegate) override;
+
+ protected:
+  // Overriden in unit tests.
+  virtual void SendEvent(Profile* profile,
+                         const std::string& extension_id,
+                         events::HistogramValue histogram_value,
+                         const std::string& name,
+                         EventRouter::UserGestureState user_gesture,
+                         std::unique_ptr<base::ListValue> args);
 
- private:
   DISALLOW_COPY_AND_ASSIGN(ExtensionNotificationHandler);
 };
 
+}  // namespace extensions
+
 #endif  // CHROME_BROWSER_EXTENSIONS_API_NOTIFICATIONS_EXTENSION_NOTIFICATION_HANDLER_H_

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

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <string>
+
+#include "base/strings/nullable_string16.h"
+#include "chrome/browser/extensions/api/notifications/extension_notification_handler.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/test/base/testing_browser_process.h"
+#include "chrome/test/base/testing_profile.h"
+#include "chrome/test/base/testing_profile_manager.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace extensions {
+namespace {
+
+static const char kChromeExtensionOrigin[] =
+    "chrome-extension://gclcddgeeaknflkijpcbplmhbkonmlij/";
+static const char kChromeExtensionId[] = "gclcddgeeaknflkijpcbplmhbkonmlij";
+static const char kChromeNotificationId[] =
+    "gclcddgeeaknflkijpcbplmhbkonmlij-id1";
+
+class TestExtensionNotificationHandler : public ExtensionNotificationHandler {
+ public:
+  // Set expected arguments for this test handler.
+  void SetTestExpectations(const std::string& extension_id,
+                           const std::string& event_name,
+                           size_t param_count) {
+    extension_id_ = extension_id;
+    event_name_ = event_name;
+    param_count_ = param_count;
+  }
+
+ protected:
+  void SendEvent(Profile* profile,
+                 const std::string& extension_id,
+                 events::HistogramValue histogram_value,
+                 const std::string& event_name,
+                 EventRouter::UserGestureState user_gesture,
+                 std::unique_ptr<base::ListValue> args) final {
+    EXPECT_EQ(event_name_, event_name);
+    EXPECT_EQ(extension_id_, extension_id);
+    EXPECT_EQ(param_count_, args->GetSize());
+  }
+
+ private:
+  std::string extension_id_;
+  std::string event_name_;
+  size_t param_count_;
+};
+}  // namespace
+
+class ExtensionNotificationHandlerTest : public testing::Test {
+ public:
+  ExtensionNotificationHandlerTest()
+      : thread_bundle_(content::TestBrowserThreadBundle::DEFAULT) {}
+
+ private:
+  content::TestBrowserThreadBundle thread_bundle_;
+};
+
+TEST_F(ExtensionNotificationHandlerTest, CloseHandler) {
+  EXPECT_TRUE(true);
+  TestingProfile::Builder builder;
+  std::unique_ptr<TestingProfile> profile = builder.Build();
+
+  TestExtensionNotificationHandler handler;
+  handler.SetTestExpectations(kChromeExtensionId, "notifications.onClosed", 2);
+  handler.OnClose(profile.get(), kChromeExtensionOrigin, kChromeNotificationId,
+                  false);
+}
+
+TEST_F(ExtensionNotificationHandlerTest, ClickHandler) {
+  EXPECT_TRUE(true);
+  TestingProfile::Builder builder;
+  std::unique_ptr<TestingProfile> profile = builder.Build();
+
+  TestExtensionNotificationHandler handler;
+  handler.SetTestExpectations(kChromeExtensionId, "notifications.onClicked", 1);
+  handler.OnClick(profile.get(), kChromeExtensionOrigin, kChromeNotificationId,
+                  -1, base::NullableString16() /* reply */);
+}
+
+TEST_F(ExtensionNotificationHandlerTest, ClickHandlerButton) {
+  EXPECT_TRUE(true);
+  TestingProfile::Builder builder;
+  std::unique_ptr<TestingProfile> profile = builder.Build();
+
+  TestExtensionNotificationHandler handler;
+  handler.SetTestExpectations(kChromeExtensionId,
+                              "notifications.onButtonClicked", 2);
+  handler.OnClick(profile.get(), kChromeExtensionOrigin, kChromeNotificationId,
+                  1, base::NullableString16() /* reply */);
+}
+
+}  // namespace extensions

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

@@ -25,7 +25,9 @@
 #include "chrome/browser/notifications/notifier_state_tracker.h"
 #include "chrome/browser/notifications/notifier_state_tracker_factory.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/common/chrome_features.h"
 #include "chrome/common/extensions/api/notifications/notification_style.h"
+#include "chrome/common/features.h"
 #include "components/keyed_service/content/browser_context_keyed_service_shutdown_notifier_factory.h"
 #include "components/keyed_service/core/keyed_service_shutdown_notifier.h"
 #include "content/public/browser/render_process_host.h"
@@ -204,12 +206,38 @@ class ShutdownNotifierFactory
   DISALLOW_COPY_AND_ASSIGN(ShutdownNotifierFactory);
 };
 
-class NotificationsApiDelegate : public NotificationDelegate {
+// Temporary native notification api delagate, it is only used
+// to extract the delegate id.
+// This is an interim state until the work in
+// https://bugs.chromium.org/p/chromium/issues/detail?id=720345
+// is completed. We need a small delegate shim since the
+// Notification object has a non virtual method (delegate_id) that is
+// used all over the place whose implementation returns delegate->id()
+#if BUILDFLAG(ENABLE_NATIVE_NOTIFICATIONS)
+class NativeNotificationApiDelegate : public NotificationDelegate {
  public:
-  NotificationsApiDelegate(ChromeAsyncExtensionFunction* api_function,
-                           Profile* profile,
-                           const std::string& extension_id,
-                           const std::string& id)
+  NativeNotificationApiDelegate(const std::string& extension_id,
+                                const std::string& notification_id)
+      : scoped_notification_id_(
+            CreateScopedIdentifier(extension_id, notification_id)) {}
+
+  std::string id() const override { return scoped_notification_id_; }
+
+ private:
+  ~NativeNotificationApiDelegate() override = default;
+  const std::string scoped_notification_id_;
+
+  DISALLOW_COPY_AND_ASSIGN(NativeNotificationApiDelegate);
+};
+#endif  // BUILDFLAG(ENABLE_NATIVE_NOTIFICATIONS)
+
+// Message center based notification delegate with all the functionality.
+class NotificationApiDelegate : public NotificationDelegate {
+ public:
+  NotificationApiDelegate(ChromeAsyncExtensionFunction* api_function,
+                          Profile* profile,
+                          const std::string& extension_id,
+                          const std::string& id)
       : api_function_(api_function),
         event_router_(EventRouter::Get(profile)),
         display_helper_(
@@ -221,7 +249,7 @@ class NotificationsApiDelegate : public NotificationDelegate {
 
     shutdown_notifier_subscription_ =
         ShutdownNotifierFactory::GetInstance()->Get(profile)->Subscribe(
-            base::Bind(&NotificationsApiDelegate::Shutdown,
+            base::Bind(&NotificationApiDelegate::Shutdown,
                        base::Unretained(this)));
   }
 
@@ -292,7 +320,7 @@ class NotificationsApiDelegate : public NotificationDelegate {
   }
 
  private:
-  ~NotificationsApiDelegate() override {}
+  ~NotificationApiDelegate() override {}
 
   void SendEvent(events::HistogramValue histogram_value,
                  const std::string& name,
@@ -333,7 +361,7 @@ class NotificationsApiDelegate : public NotificationDelegate {
   std::unique_ptr<KeyedServiceShutdownNotifier::Subscription>
       shutdown_notifier_subscription_;
 
-  DISALLOW_COPY_AND_ASSIGN(NotificationsApiDelegate);
+  DISALLOW_COPY_AND_ASSIGN(NotificationApiDelegate);
 };
 
 }  // namespace
@@ -489,9 +517,20 @@ bool NotificationsApiFunction::CreateNotification(
   if (options->is_clickable.get())
     optional_fields.clickable = *options->is_clickable;
 
-  NotificationsApiDelegate* api_delegate(new NotificationsApiDelegate(
-      this, GetProfile(), extension_->id(), id));  // ownership is passed to
-                                                   // Notification
+  // Create the notification api delegate. Ownership passed to the notification.
+  NotificationDelegate* api_delegate;
+#if BUILDFLAG(ENABLE_NATIVE_NOTIFICATIONS)
+  if (base::FeatureList::IsEnabled(features::kNativeNotifications)) {
+    api_delegate = new NativeNotificationApiDelegate(extension_->id(), id);
+  } else {
+    api_delegate =
+        new NotificationApiDelegate(this, GetProfile(), extension_->id(), id);
+  }
+#else
+  api_delegate =
+      new NotificationApiDelegate(this, GetProfile(), extension_->id(), id);
+#endif  // BUILDFLAG(ENABLE_NATIVE_NOTIFICATIONS)
+
   Notification notification(
       type, title, message, icon,
       message_center::NotifierId(message_center::NotifierId::APPLICATION,

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

@@ -40,7 +40,6 @@
 
 #if defined(OS_MACOSX)
 #include "base/mac/mac_util.h"
-#include "ui/base/test/scoped_fake_nswindow_fullscreen.h"
 #endif
 
 using extensions::AppWindow;
@@ -245,6 +244,8 @@ IN_PROC_BROWSER_TEST_F(NotificationsApiTest, TestCSP) {
   ASSERT_TRUE(RunExtensionTest("notifications/api/csp")) << message_;
 }
 
+// Native notifications don't support (nor use) observers.
+#if !defined(OS_MACOSX)
 IN_PROC_BROWSER_TEST_F(NotificationsApiTest, TestByUser) {
   const extensions::Extension* extension =
       LoadExtensionAndWait("notifications/api/by_user");
@@ -281,6 +282,7 @@ IN_PROC_BROWSER_TEST_F(NotificationsApiTest, TestByUser) {
     EXPECT_TRUE(catcher.GetNextResult()) << catcher.message();
   }
 }
+#endif  // !defined(OS_MACOSX)
 
 IN_PROC_BROWSER_TEST_F(NotificationsApiTest, TestPartialUpdate) {
   ASSERT_TRUE(RunExtensionTest("notifications/api/partial_update")) << message_;
@@ -380,6 +382,8 @@ IN_PROC_BROWSER_TEST_F(NotificationsApiTest, TestOnPermissionLevelChanged) {
   }
 }
 
+// Native notifications don't support (nor use) observers.
+#if !defined(OS_MACOSX)
 IN_PROC_BROWSER_TEST_F(NotificationsApiTest, TestUserGesture) {
   const extensions::Extension* extension =
       LoadExtensionAndWait("notifications/api/user_gesture");
@@ -402,6 +406,7 @@ IN_PROC_BROWSER_TEST_F(NotificationsApiTest, TestUserGesture) {
 
   ASSERT_FALSE(GetNotificationForExtension(extension));
 }
+#endif  // !defined(OS_MACOSX)
 
 IN_PROC_BROWSER_TEST_F(NotificationsApiTest, TestRequireInteraction) {
   const extensions::Extension* extension =
@@ -434,10 +439,10 @@ IN_PROC_BROWSER_TEST_F(NotificationsApiTest, TestShouldDisplayNormal) {
   ASSERT_FALSE(notification->delegate()->ShouldDisplayOverFullscreen());
 }
 
+// Full screen related tests don't run on Mac as native notifications full
+// screen decisions are done by the OS directly.
+#if !defined(OS_MACOSX)
 IN_PROC_BROWSER_TEST_F(NotificationsApiTest, TestShouldDisplayFullscreen) {
-#if defined(OS_MACOSX)
-  ui::test::ScopedFakeNSWindowFullscreen fake_fullscreen;
-#endif
   EnableFullscreenNotifications();
   ExtensionTestMessageListener notification_created_listener("created", false);
   const Extension* extension = LoadAppWithWindowState(
@@ -463,9 +468,6 @@ IN_PROC_BROWSER_TEST_F(NotificationsApiTest, TestShouldDisplayFullscreen) {
 }
 
 IN_PROC_BROWSER_TEST_F(NotificationsApiTest, TestShouldDisplayFullscreenOff) {
-#if defined(OS_MACOSX)
-  ui::test::ScopedFakeNSWindowFullscreen fake_fullscreen;
-#endif
   DisableFullscreenNotifications();
   ExtensionTestMessageListener notification_created_listener("created", false);
   const Extension* extension = LoadAppWithWindowState(
@@ -492,7 +494,6 @@ IN_PROC_BROWSER_TEST_F(NotificationsApiTest, TestShouldDisplayFullscreenOff) {
 
 // The Fake OSX fullscreen window doesn't like drawing a second fullscreen
 // window when another is visible.
-#if !defined(OS_MACOSX)
 IN_PROC_BROWSER_TEST_F(NotificationsApiTest, TestShouldDisplayMultiFullscreen) {
   // Start a fullscreen app, and then start another fullscreen app on top of the
   // first. Notifications from the first should not be displayed because it is
@@ -522,15 +523,11 @@ IN_PROC_BROWSER_TEST_F(NotificationsApiTest, TestShouldDisplayMultiFullscreen) {
   // notification shouldn't be displayed.
   ASSERT_FALSE(notification->delegate()->ShouldDisplayOverFullscreen());
 }
-#endif
 
 // Verify that a notification is actually displayed when the app window that
 // creates it is fullscreen with the fullscreen notification flag turned on.
 IN_PROC_BROWSER_TEST_F(NotificationsApiTest,
                        TestShouldDisplayPopupNotification) {
-#if defined(OS_MACOSX)
-  ui::test::ScopedFakeNSWindowFullscreen fake_fullscreen;
-#endif
   EnableFullscreenNotifications();
   ExtensionTestMessageListener notification_created_listener("created", false);
   const Extension* extension = LoadAppWithWindowState(
@@ -554,3 +551,4 @@ IN_PROC_BROWSER_TEST_F(NotificationsApiTest,
   // the notification displays on top of it.
   ASSERT_TRUE(notification->delegate()->ShouldDisplayOverFullscreen());
 }
+#endif  // !defined(OS_MACOSX)

chrome/browser/notifications/native_notification_display_service.cc

@@ -60,8 +60,9 @@ NativeNotificationDisplayService::NativeNotificationDisplayService(
   AddNotificationHandler(NotificationCommon::PERSISTENT,
                          base::MakeUnique<PersistentNotificationHandler>());
 #if BUILDFLAG(ENABLE_EXTENSIONS)
-  AddNotificationHandler(NotificationCommon::EXTENSION,
-                         base::MakeUnique<ExtensionNotificationHandler>());
+  AddNotificationHandler(
+      NotificationCommon::EXTENSION,
+      base::MakeUnique<extensions::ExtensionNotificationHandler>());
 #endif
 }
 

chrome/test/BUILD.gn

@@ -3851,6 +3851,7 @@ test("unit_tests") {
       "../browser/extensions/api/image_writer_private/write_from_file_operation_unittest.cc",
       "../browser/extensions/api/image_writer_private/write_from_url_operation_unittest.cc",
       "../browser/extensions/api/management/management_api_unittest.cc",
+      "../browser/extensions/api/notifications/extension_notification_handler_unittest.cc",
       "../browser/extensions/api/omnibox/omnibox_unittest.cc",
       "../browser/extensions/api/passwords_private/passwords_private_delegate_impl_unittest.cc",
       "../browser/extensions/api/passwords_private/passwords_private_utils_unittest.cc",