lacros: Refactor NotificationPlatformBridgeChromeOs

Lacros needs to support two notification pathways:
* In-process, for the ash-chrome binary
* Out-of-process, for the lacros-chrome binary

However, both of these pathways have some shared behavior around
handling multiple profiles (for the pre-lacros ash case) and transient
notifications. This code lives in NotificationPlatformBridgeChromeOs
and we want to reuse it.

1. Introduce MessageCenterClientLacros. This is a stub for now. In
   a follow-up CL it will send notifications over mojo from lacros
   to ash.
2. Change NotificationPlatformBridgeChromeOs so it supports different
   |impl_| helpers for ash and lacros.
3. Refactor ChromeAshMessageCenterClient to inherit from
   NotificationPlatformBridge. This allows NPBChromeOs to have a
   generic |impl_| for both helpers, and avoids introducing a new
   abstract interface class.

See go/lacros-notifications for overall plan.

Bug: 1113889
Test: existing unit_tests and browser_tests
Change-Id: I159f410f3d84cc6e9bde0b3757015691f2289419
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2343217Reviewed-by: Jun Mukai <mukai@chromium.org>
Commit-Queue: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#796096}

chrome/browser/BUILD.gn

@@ -4032,6 +4032,7 @@ static_library("browser") {
     ]
   }
 
+  # TODO(crbug.com/1052397): Rename chromeos_is_browser_only to is_lacros.
   if (chromeos_is_browser_only) {
     sources += [
       "chrome_browser_main_extra_parts_lacros.cc",
@@ -4040,6 +4041,11 @@ static_library("browser") {
       "lacros/lacros_chrome_service_delegate_impl.h",
       "metrics/lacros_metrics_provider.cc",
       "metrics/lacros_metrics_provider.h",
+      "notifications/message_center_client_lacros.cc",
+      "notifications/message_center_client_lacros.h",
+      "notifications/notification_platform_bridge_chromeos.cc",
+      "notifications/notification_platform_bridge_chromeos.h",
+      "notifications/notification_platform_bridge_delegate.h",
     ]
     deps += [
       "//chromeos/crosapi/mojom",
@@ -4438,7 +4444,8 @@ static_library("browser") {
       deps += [ "//components/dbus/thread_linux" ]
     }
 
-    if (enable_native_notifications) {
+    # TODO(crbug.com/1052397): Rename chromeos_is_browser_only to is_lacros.
+    if (enable_native_notifications && !chromeos_is_browser_only) {
       sources += [
         "notifications/notification_platform_bridge_linux.cc",
         "notifications/notification_platform_bridge_linux.h",

chrome/browser/notifications/chrome_ash_message_center_client.cc

@@ -125,7 +125,10 @@ ChromeAshMessageCenterClient::~ChromeAshMessageCenterClient() {
 }
 
 void ChromeAshMessageCenterClient::Display(
-    const message_center::Notification& notification) {
+    NotificationHandler::Type notification_type,
+    Profile* profile,
+    const message_center::Notification& notification,
+    std::unique_ptr<NotificationCommon::Metadata> metadata) {
   auto message_center_notification =
       std::make_unique<message_center::Notification>(
           base::WrapRefCounted(
@@ -134,7 +137,8 @@ void ChromeAshMessageCenterClient::Display(
   MessageCenter::Get()->AddNotification(std::move(message_center_notification));
 }
 
-void ChromeAshMessageCenterClient::Close(const std::string& notification_id) {
+void ChromeAshMessageCenterClient::Close(Profile* profile,
+                                         const std::string& notification_id) {
   // During shutdown, Ash is destroyed before |this|, taking the MessageCenter
   // with it.
   if (MessageCenter::Get()) {
@@ -143,6 +147,21 @@ void ChromeAshMessageCenterClient::Close(const std::string& notification_id) {
   }
 }
 
+void ChromeAshMessageCenterClient::GetDisplayed(
+    Profile* profile,
+    GetDisplayedNotificationsCallback callback) const {
+  // Right now, this is only used to get web notifications that were created by
+  // and have outlived a previous browser process. Ash itself doesn't outlive
+  // the browser process, so there's no need to implement.
+  std::move(callback).Run(/*notification_ids=*/{}, /*supports_sync=*/false);
+}
+
+void ChromeAshMessageCenterClient::SetReadyCallback(
+    NotificationBridgeReadyCallback callback) {
+  // Ash is always available in-process, so report the client is ready.
+  std::move(callback).Run(true);
+}
+
 void ChromeAshMessageCenterClient::GetNotifiers() {
   if (!notifier_observers_.might_have_observers())
     return;

chrome/browser/notifications/chrome_ash_message_center_client.h

@@ -12,21 +12,28 @@
 #include "chrome/browser/notifications/notification_platform_bridge_delegate.h"
 #include "chrome/browser/notifications/notifier_controller.h"
 
-// This class serves as Chrome's AshMessageCenterClient, as well as the
-// NotificationPlatformBridge for ChromeOS. It dispatches notifications to Ash
+// Helper for NotificationPlatformBridgeChromeOs. Sends notifications to Ash
 // and handles interactions with those notifications, plus it keeps track of
 // NotifierControllers to provide notifier settings information to Ash (visible
-// in NotifierSettingsView).
-class ChromeAshMessageCenterClient : public ash::NotifierSettingsController,
+// in NotifierSettingsView). With Lacros, runs in the ash-chrome process.
+class ChromeAshMessageCenterClient : public NotificationPlatformBridge,
+                                     public ash::NotifierSettingsController,
                                      public NotifierController::Observer {
  public:
   explicit ChromeAshMessageCenterClient(
       NotificationPlatformBridgeDelegate* delegate);
-
   ~ChromeAshMessageCenterClient() override;
 
-  void Display(const message_center::Notification& notification);
-  void Close(const std::string& notification_id);
+  // NotificationPlatformBridge:
+  void Display(NotificationHandler::Type notification_type,
+               Profile* profile,
+               const message_center::Notification& notification,
+               std::unique_ptr<NotificationCommon::Metadata> metadata) override;
+  void Close(Profile* profile, const std::string& notification_id) override;
+  void GetDisplayed(Profile* profile,
+                    GetDisplayedNotificationsCallback callback) const override;
+  void SetReadyCallback(NotificationBridgeReadyCallback callback) override;
+  void DisplayServiceShutDown(Profile* profile) override {}
 
   // ash::NotifierSettingsController:
   void GetNotifiers() override;

chrome/browser/notifications/message_center_client_lacros.cc

+// Copyright 2020 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 "chrome/browser/notifications/message_center_client_lacros.h"
+
+#include <utility>
+
+#include "base/check.h"
+#include "base/notreached.h"
+
+MessageCenterClientLacros::MessageCenterClientLacros(
+    NotificationPlatformBridgeDelegate* delegate)
+    : delegate_(delegate) {
+  DCHECK(delegate_);
+}
+
+MessageCenterClientLacros::~MessageCenterClientLacros() = default;
+
+void MessageCenterClientLacros::Display(
+    NotificationHandler::Type notification_type,
+    Profile* profile,
+    const message_center::Notification& notification,
+    std::unique_ptr<NotificationCommon::Metadata> metadata) {
+  NOTIMPLEMENTED();
+}
+
+void MessageCenterClientLacros::Close(Profile* profile,
+                                      const std::string& notification_id) {
+  NOTIMPLEMENTED();
+}
+
+void MessageCenterClientLacros::GetDisplayed(
+    Profile* profile,
+    GetDisplayedNotificationsCallback callback) const {
+  NOTIMPLEMENTED();
+  std::move(callback).Run(/*notification_ids=*/{}, /*supports_sync=*/false);
+}
+
+void MessageCenterClientLacros::SetReadyCallback(
+    NotificationBridgeReadyCallback callback) {
+  NOTIMPLEMENTED();
+  std::move(callback).Run(true);
+}
+
+void MessageCenterClientLacros::DisplayServiceShutDown(Profile* profile) {
+  NOTIMPLEMENTED();
+}

chrome/browser/notifications/message_center_client_lacros.h

+// Copyright 2020 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.
+
+#ifndef CHROME_BROWSER_NOTIFICATIONS_MESSAGE_CENTER_CLIENT_LACROS_H_
+#define CHROME_BROWSER_NOTIFICATIONS_MESSAGE_CENTER_CLIENT_LACROS_H_
+
+#include "chrome/browser/notifications/notification_platform_bridge.h"
+
+class NotificationPlatformBridgeDelegate;
+
+// Sends notifications to ash-chrome over mojo. Responds to user actions like
+// clicks on notifications received over mojo. Works together with
+// NotificationPlatformBridgeChromeOs because that class contains support for
+// transient notifications and multiple profiles.
+// TODO(jamescook): Derive from crosapi::mojom::MessageCenterClient once that
+// mojo interface is introduced.
+class MessageCenterClientLacros : public NotificationPlatformBridge {
+ public:
+  explicit MessageCenterClientLacros(
+      NotificationPlatformBridgeDelegate* delegate);
+  MessageCenterClientLacros(const MessageCenterClientLacros&) = delete;
+  MessageCenterClientLacros& operator=(const MessageCenterClientLacros&) =
+      delete;
+  ~MessageCenterClientLacros() override;
+
+  // NotificationPlatformBridge:
+  void Display(NotificationHandler::Type notification_type,
+               Profile* profile,
+               const message_center::Notification& notification,
+               std::unique_ptr<NotificationCommon::Metadata> metadata) override;
+  void Close(Profile* profile, const std::string& notification_id) override;
+  void GetDisplayed(Profile* profile,
+                    GetDisplayedNotificationsCallback callback) const override;
+  void SetReadyCallback(NotificationBridgeReadyCallback callback) override;
+  void DisplayServiceShutDown(Profile* profile) override;
+
+  // TODO(jamescook): Add "client" methods like OnNotificationClicked,
+  // OnNotificationClosed, etc.
+
+ private:
+  NotificationPlatformBridgeDelegate* delegate_;
+};
+
+#endif  // CHROME_BROWSER_NOTIFICATIONS_MESSAGE_CENTER_CLIENT_LACROS_H_

chrome/browser/notifications/notification_platform_bridge_chromeos.cc

@@ -7,9 +7,9 @@
 #include <memory>
 
 #include "base/callback_list.h"
+#include "build/lacros_buildflags.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
-#include "chrome/browser/notifications/chrome_ash_message_center_client.h"
 #include "chrome/browser/notifications/notification_display_service_factory.h"
 #include "chrome/browser/notifications/notification_display_service_impl.h"
 #include "chrome/browser/profiles/profile.h"
@@ -17,6 +17,12 @@
 #include "chrome/browser/ui/app_icon_loader.h"
 #include "ui/gfx/image/image.h"
 
+#if BUILDFLAG(IS_LACROS)
+#include "chrome/browser/notifications/message_center_client_lacros.h"
+#else
+#include "chrome/browser/notifications/chrome_ash_message_center_client.h"
+#endif
+
 // static
 std::unique_ptr<NotificationPlatformBridge>
 NotificationPlatformBridge::Create() {
@@ -29,10 +35,16 @@ bool NotificationPlatformBridge::CanHandleType(
   return true;
 }
 
-NotificationPlatformBridgeChromeOs::NotificationPlatformBridgeChromeOs()
-    : impl_(std::make_unique<ChromeAshMessageCenterClient>(this)) {}
+NotificationPlatformBridgeChromeOs::NotificationPlatformBridgeChromeOs() {
+#if BUILDFLAG(IS_LACROS)
+  impl_ = std::make_unique<MessageCenterClientLacros>(this);
+#else
+  impl_ = std::make_unique<ChromeAshMessageCenterClient>(this);
+#endif
+}
 
-NotificationPlatformBridgeChromeOs::~NotificationPlatformBridgeChromeOs() {}
+NotificationPlatformBridgeChromeOs::~NotificationPlatformBridgeChromeOs() =
+    default;
 
 void NotificationPlatformBridgeChromeOs::Display(
     NotificationHandler::Type notification_type,
@@ -41,7 +53,8 @@ void NotificationPlatformBridgeChromeOs::Display(
     std::unique_ptr<NotificationCommon::Metadata> metadata) {
   auto active_notification = std::make_unique<ProfileNotification>(
       profile, notification, notification_type);
-  impl_->Display(active_notification->notification());
+  impl_->Display(active_notification->type(), profile,
+                 active_notification->notification(), std::move(metadata));
 
   std::string profile_notification_id =
       active_notification->notification().id();
@@ -56,25 +69,18 @@ void NotificationPlatformBridgeChromeOs::Close(
   const std::string profile_notification_id =
       ProfileNotification::GetProfileNotificationId(
           notification_id, NotificationUIManager::GetProfileID(profile));
-
-  impl_->Close(profile_notification_id);
+  impl_->Close(profile, profile_notification_id);
 }
 
 void NotificationPlatformBridgeChromeOs::GetDisplayed(
     Profile* profile,
     GetDisplayedNotificationsCallback callback) const {
-  // Right now, this is only used to get web notifications that were created by
-  // and have outlived a previous browser process. Ash itself doesn't outlive
-  // the browser process, so there's no need to implement.
-  std::set<std::string> displayed_notifications;
-  std::move(callback).Run(std::move(displayed_notifications), false);
+  impl_->GetDisplayed(profile, std::move(callback));
 }
 
 void NotificationPlatformBridgeChromeOs::SetReadyCallback(
     NotificationBridgeReadyCallback callback) {
-  // We don't handle the absence of Ash or a failure to open a Mojo connection,
-  // so just assume the client is ready.
-  std::move(callback).Run(true);
+  impl_->SetReadyCallback(std::move(callback));
 }
 
 void NotificationPlatformBridgeChromeOs::DisplayServiceShutDown(
@@ -91,6 +97,8 @@ void NotificationPlatformBridgeChromeOs::DisplayServiceShutDown(
 
   for (auto id : ids_to_close)
     HandleNotificationClosed(id, false);
+
+  impl_->DisplayServiceShutDown(profile);
 }
 
 void NotificationPlatformBridgeChromeOs::HandleNotificationClosed(

chrome/browser/notifications/notification_platform_bridge_chromeos.h

@@ -6,23 +6,22 @@
 #define CHROME_BROWSER_NOTIFICATIONS_NOTIFICATION_PLATFORM_BRIDGE_CHROMEOS_H_
 
 #include <map>
+#include <memory>
 #include <string>
 
-#include "base/callback_forward.h"
 #include "base/macros.h"
 #include "chrome/browser/notifications/notification_platform_bridge.h"
 #include "chrome/browser/notifications/notification_platform_bridge_delegate.h"
 #include "chrome/browser/notifications/profile_notification.h"
 
-class ChromeAshMessageCenterClient;
-
 // A platform bridge that uses Ash's message center to display notifications.
+// Forwards requests to a helper implementation class, which either makes
+// in-process C++ calls (pre-lacros) or mojo calls (post-lacros).
 class NotificationPlatformBridgeChromeOs
     : public NotificationPlatformBridge,
       public NotificationPlatformBridgeDelegate {
  public:
   NotificationPlatformBridgeChromeOs();
-
   ~NotificationPlatformBridgeChromeOs() override;
 
   // NotificationPlatformBridge:
@@ -34,6 +33,7 @@ class NotificationPlatformBridgeChromeOs
   void GetDisplayed(Profile* profile,
                     GetDisplayedNotificationsCallback callback) const override;
   void SetReadyCallback(NotificationBridgeReadyCallback callback) override;
+  void DisplayServiceShutDown(Profile* profile) override;
 
   // NotificationPlatformBridgeDelegate:
   void HandleNotificationClosed(const std::string& id, bool by_user) override;
@@ -44,18 +44,18 @@ class NotificationPlatformBridgeChromeOs
       const base::Optional<base::string16>& reply) override;
   void HandleNotificationSettingsButtonClicked(const std::string& id) override;
   void DisableNotification(const std::string& id) override;
-  void DisplayServiceShutDown(Profile* profile) override;
 
  private:
   // Gets the ProfileNotification for the given identifier which has been
   // mutated to uniquely identify the profile. This may return null if the
   // notification has already been closed due to profile shutdown. Ash may
-  // asynchronously inform |this| of actions on notificationafter their
+  // asynchronously inform |this| of actions on notifications after their
   // associated profile has already been destroyed.
   ProfileNotification* GetProfileNotification(
       const std::string& profile_notification_id);
 
-  std::unique_ptr<ChromeAshMessageCenterClient> impl_;
+  // Helper implementation.
+  std::unique_ptr<NotificationPlatformBridge> impl_;
 
   // A container for all active notifications, where IDs are permuted to
   // uniquely identify both the notification and its source profile. The key is

chrome/test/BUILD.gn

@@ -4518,7 +4518,8 @@ test("unit_tests") {
   }
 
   if (enable_native_notifications) {
-    if (is_desktop_linux) {
+    # TODO(crbug.com/1052397): Rename chromeos_is_browser_only to is_lacros.
+    if (is_desktop_linux && !chromeos_is_browser_only) {
       sources += [ "../browser/notifications/notification_platform_bridge_linux_unittest.cc" ]
     }