Handle ARC notifications in ash

Makes ui/arc code run inside ash instead of the browser. The code
itself will be moved to //ash in follow-up CLs.

- ArcNotificationManager lives in ash::MessageCenterController;
- Arc bridge host forwards the NotificationsInstance mojo interface
  to ash via AshMessageCenterController interface;
- ArcNotificationSurfaceManager lives in ash::WaylandServerController;

Bug: 768439
Change-Id: Id3572a64cd4d8fac709cd2c22f86b392571d5041
Reviewed-on: https://chromium-review.googlesource.com/1011205
Commit-Queue: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Reviewed-by: Yoshiki Iguchi <yoshiki@chromium.org>
Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553397}

ash/BUILD.gn

@@ -1235,6 +1235,7 @@ component("ash") {
     "//third_party/qcms",
     "//third_party/re2",
     "//ui/accessibility",
+    "//ui/arc",
     "//ui/base",
     "//ui/base:ui_data_pack",
     "//ui/base/ime",

ash/DEPS

@@ -111,6 +111,9 @@ specific_include_rules = {
   "app_launch_unittest.cc": [
     "+ash/components/quick_launch/public",
   ],
+  "message_center_controller\.*": [
+    "+components/arc/common/notifications.mojom.h"
+  ],
   "root_window_controller\.*": [
     "+ash/host"
   ],

ash/message_center/message_center_controller.cc

@@ -6,9 +6,12 @@
 
 #include "ash/public/cpp/ash_switches.h"
 #include "ash/public/cpp/vector_icons/vector_icons.h"
+#include "ash/session/session_controller.h"
+#include "ash/shell.h"
 #include "ash/strings/grit/ash_strings.h"
 #include "base/command_line.h"
 #include "base/unguessable_token.h"
+#include "components/signin/core/account_id/account_id.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/message_center/message_center.h"
 #include "ui/message_center/public/cpp/notification.h"
@@ -148,6 +151,19 @@ void MessageCenterController::SetClient(
   client_.Bind(std::move(client));
 }
 
+void MessageCenterController::SetArcNotificationsInstance(
+    arc::mojom::NotificationsInstancePtr arc_notification_instance) {
+  if (!arc_notification_manager_) {
+    arc_notification_manager_ = std::make_unique<arc::ArcNotificationManager>(
+        Shell::Get()
+            ->session_controller()
+            ->GetPrimaryUserSession()
+            ->user_info->account_id,
+        message_center::MessageCenter::Get());
+  }
+  arc_notification_manager_->SetInstance(std::move(arc_notification_instance));
+}
+
 void MessageCenterController::ShowClientNotification(
     const message_center::Notification& notification,
     const base::UnguessableToken& display_token) {

ash/message_center/message_center_controller.h

@@ -11,8 +11,10 @@
 #include "ash/system/web_notification/inactive_user_notification_blocker.h"
 #include "ash/system/web_notification/session_state_notification_blocker.h"
 #include "base/macros.h"
+#include "components/arc/common/notifications.mojom.h"
 #include "mojo/public/cpp/bindings/associated_binding.h"
 #include "mojo/public/cpp/bindings/binding_set.h"
+#include "ui/arc/notification/arc_notification_manager.h"
 
 namespace message_center {
 struct NotifierId;
@@ -37,6 +39,8 @@ class ASH_EXPORT MessageCenterController
   // mojom::AshMessageCenterController:
   void SetClient(
       mojom::AshMessageCenterClientAssociatedPtrInfo client) override;
+  void SetArcNotificationsInstance(
+      arc::mojom::NotificationsInstancePtr arc_notification_instance) override;
   void ShowClientNotification(
       const message_center::Notification& notification,
       const base::UnguessableToken& display_token) override;
@@ -88,6 +92,8 @@ class ASH_EXPORT MessageCenterController
 
   mojom::AshMessageCenterClientAssociatedPtr client_;
 
+  std::unique_ptr<arc::ArcNotificationManager> arc_notification_manager_;
+
   DISALLOW_COPY_AND_ASSIGN(MessageCenterController);
 };
 

ash/public/interfaces/BUILD.gn

@@ -61,6 +61,7 @@ mojom("interfaces_internal") {
   public_deps = [
     "//chromeos/components/proximity_auth/public/interfaces",
     "//chromeos/services/assistant/public/mojom",
+    "//components/arc/common:notifications",
     "//components/password_manager/public/interfaces",
     "//components/signin/public/interfaces",
     "//components/sync/mojo:interfaces",

ash/public/interfaces/ash_message_center_controller.mojom

@@ -4,6 +4,7 @@
 
 module ash.mojom;
 
+import "components/arc/common/notifications.mojom";
 import "ui/gfx/image/mojo/image.mojom";
 import "ui/message_center/public/mojo/notification.mojom";
 import "ui/message_center/public/mojo/notifier_id.mojom";
@@ -34,6 +35,9 @@ struct NotifierUiData {
 interface AshMessageCenterController {
   SetClient(associated AshMessageCenterClient client);
 
+  // Sets the ARC notification instance.
+  SetArcNotificationsInstance(arc.mojom.NotificationsInstance instance);
+
   // Shows a notification. |display_token| will be used to reference this
   // notification for AshMessageCenterClient::Close().
   ShowClientNotification(message_center.mojom.Notification notification,

ash/shell.cc

@@ -424,11 +424,9 @@ void Shell::RegisterUserProfilePrefs(PrefRegistrySimple* registry,
   PowerPrefs::RegisterUserProfilePrefs(registry, for_test);
 }
 
-void Shell::InitWaylandServer(
-    exo::NotificationSurfaceManager* notification_surface_manager,
-    std::unique_ptr<exo::FileHelper> file_helper) {
-  wayland_server_controller_ = WaylandServerController::CreateIfNecessary(
-      notification_surface_manager, std::move(file_helper));
+void Shell::InitWaylandServer(std::unique_ptr<exo::FileHelper> file_helper) {
+  wayland_server_controller_ =
+      WaylandServerController::CreateIfNecessary(std::move(file_helper));
 }
 
 void Shell::DestroyWaylandServer() {

ash/shell.h

@@ -46,7 +46,6 @@ class DisplayManager;
 
 namespace exo {
 class FileHelper;
-class NotificationSurfaceManager;
 }  // namespace exo
 
 namespace gfx {
@@ -290,9 +289,7 @@ class ASH_EXPORT Shell : public SessionObserver,
                                        bool for_test = false);
 
   // If necessary, initializes the Wayland server.
-  void InitWaylandServer(
-      exo::NotificationSurfaceManager* notification_surface_manager,
-      std::unique_ptr<exo::FileHelper> file_helper);
+  void InitWaylandServer(std::unique_ptr<exo::FileHelper> file_helper);
   void DestroyWaylandServer();
 
   // Creates a default views::NonClientFrameView for use by windows in the

ash/shell/content/client/shell_browser_main_parts.cc

@@ -103,7 +103,7 @@ void ShellBrowserMainParts::PreMainMessageLoopRun() {
 
   ash::Shell::GetPrimaryRootWindow()->GetHost()->Show();
 
-  ash::Shell::Get()->InitWaylandServer(nullptr, nullptr);
+  ash::Shell::Get()->InitWaylandServer(nullptr);
 }
 
 void ShellBrowserMainParts::PostMainMessageLoopRun() {

ash/wayland/BUILD.gn

@@ -16,5 +16,6 @@ source_set("wayland") {
     "//components/exo",
     "//components/exo/wayland",
     "//skia",
+    "//ui/arc",
   ]
 }

ash/wayland/wayland_server_controller.cc

@@ -15,6 +15,7 @@
 #include "components/exo/file_helper.h"
 #include "components/exo/wayland/server.h"
 #include "components/exo/wm_helper.h"
+#include "ui/arc/notification/arc_notification_surface_manager_impl.h"
 
 namespace ash {
 
@@ -46,15 +47,13 @@ class WaylandServerController::WaylandWatcher
 // static
 std::unique_ptr<WaylandServerController>
 WaylandServerController::CreateIfNecessary(
-    exo::NotificationSurfaceManager* notification_surface_manager,
     std::unique_ptr<exo::FileHelper> file_helper) {
   if (!base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kAshEnableWaylandServer)) {
     return nullptr;
   }
 
-  return base::WrapUnique(new WaylandServerController(
-      notification_surface_manager, std::move(file_helper)));
+  return base::WrapUnique(new WaylandServerController(std::move(file_helper)));
 }
 
 WaylandServerController::~WaylandServerController() {
@@ -66,12 +65,13 @@ WaylandServerController::~WaylandServerController() {
 }
 
 WaylandServerController::WaylandServerController(
-    exo::NotificationSurfaceManager* notification_surface_manager,
     std::unique_ptr<exo::FileHelper> file_helper) {
+  arc_notification_surface_manager_ =
+      std::make_unique<arc::ArcNotificationSurfaceManagerImpl>();
   wm_helper_ = std::make_unique<exo::WMHelper>();
   exo::WMHelper::SetInstance(wm_helper_.get());
-  display_ = std::make_unique<exo::Display>(notification_surface_manager,
-                                            std::move(file_helper));
+  display_ = std::make_unique<exo::Display>(
+      arc_notification_surface_manager_.get(), std::move(file_helper));
   wayland_server_ = exo::wayland::Server::Create(display_.get());
   // Wayland server creation can fail if XDG_RUNTIME_DIR is not set correctly.
   if (wayland_server_)

ash/wayland/wayland_server_controller.h

@@ -9,10 +9,13 @@
 
 #include "base/macros.h"
 
+namespace arc {
+class ArcNotificationSurfaceManagerImpl;
+}  // namespace arc
+
 namespace exo {
 class Display;
 class FileHelper;
-class NotificationSurfaceManager;
 class WMHelper;
 namespace wayland {
 class Server;
@@ -26,14 +29,12 @@ class WaylandServerController {
   // Creates WaylandServerController. Returns null if controller should not be
   // created.
   static std::unique_ptr<WaylandServerController> CreateIfNecessary(
-      exo::NotificationSurfaceManager* notification_surface_manager,
       std::unique_ptr<exo::FileHelper> file_helper);
 
   ~WaylandServerController();
 
  private:
-  WaylandServerController(
-      exo::NotificationSurfaceManager* notification_surface_manager,
+  explicit WaylandServerController(
       std::unique_ptr<exo::FileHelper> file_helper);
 
   std::unique_ptr<exo::WMHelper> wm_helper_;
@@ -42,6 +43,9 @@ class WaylandServerController {
   class WaylandWatcher;
   std::unique_ptr<WaylandWatcher> wayland_watcher_;
 
+  std::unique_ptr<arc::ArcNotificationSurfaceManagerImpl>
+      arc_notification_surface_manager_;
+
   DISALLOW_COPY_AND_ASSIGN(WaylandServerController);
 };
 

ash/window_manager.cc

@@ -340,10 +340,8 @@ void WindowManager::OnWmConnected() {
 
   // We only create controller in the ash process for mash.
   if (Shell::GetAshConfig() == Config::MASH) {
-    // TODO(penghuang): wire up notification surface manager.
-    // https://crbug.com/768439
     // TODO(hirono): wire up the file helper. http://crbug.com/768395
-    Shell::Get()->InitWaylandServer(nullptr, nullptr);
+    Shell::Get()->InitWaylandServer(nullptr);
   }
 }
 

chrome/browser/chromeos/arc/arc_service_launcher.cc

@@ -62,7 +62,6 @@
 #include "components/arc/volume_mounter/arc_volume_mounter_bridge.h"
 #include "components/arc/wake_lock/arc_wake_lock_bridge.h"
 #include "components/prefs/pref_member.h"
-#include "ui/arc/notification/arc_notification_manager.h"
 
 namespace arc {
 namespace {
@@ -156,10 +155,6 @@ void ArcServiceLauncher::OnPrimaryUserProfilePrepared(Profile* profile) {
   ArcMetricsService::GetForBrowserContext(profile);
   ArcMidisBridge::GetForBrowserContext(profile);
   ArcNetHostImpl::GetForBrowserContext(profile);
-  // TODO(https://crbug.com/816441): Remove the callback workaround.
-  ArcNotificationManager::GetForBrowserContext(profile)
-      ->set_get_app_id_callback(
-          ArcAppListPrefs::Get(profile)->GetAppIdByPackageNameCallback());
   ArcObbMounterBridge::GetForBrowserContext(profile);
   ArcOemCryptoBridge::GetForBrowserContext(profile);
   ArcPolicyBridge::GetForBrowserContext(profile);

chrome/browser/exo_parts.cc

@@ -23,7 +23,6 @@
 #include "content/public/common/drop_data.h"
 #include "storage/browser/fileapi/file_system_context.h"
 #include "storage/browser/fileapi/file_system_url.h"
-#include "ui/arc/notification/arc_notification_surface_manager_impl.h"
 
 namespace {
 
@@ -123,10 +122,7 @@ ExoParts::~ExoParts() {
 
 ExoParts::ExoParts() {
   DCHECK(!ash_util::IsRunningInMash());
-  arc_notification_surface_manager_ =
-      std::make_unique<arc::ArcNotificationSurfaceManagerImpl>();
   std::unique_ptr<ChromeFileHelper> file_helper =
       std::make_unique<ChromeFileHelper>();
-  ash::Shell::Get()->InitWaylandServer(arc_notification_surface_manager_.get(),
-                                       std::move(file_helper));
+  ash::Shell::Get()->InitWaylandServer(std::move(file_helper));
 }

chrome/browser/exo_parts.h

@@ -9,10 +9,6 @@
 
 #include "base/macros.h"
 
-namespace arc {
-class ArcNotificationSurfaceManagerImpl;
-}
-
 class ExoParts {
  public:
   // Creates ExoParts. Returns null if exo should not be created.
@@ -23,9 +19,6 @@ class ExoParts {
  private:
   ExoParts();
 
-  std::unique_ptr<arc::ArcNotificationSurfaceManagerImpl>
-      arc_notification_surface_manager_;
-
   DISALLOW_COPY_AND_ASSIGN(ExoParts);
 };
 

components/arc/BUILD.gn

@@ -149,11 +149,6 @@ static_library("arc_base") {
     "arc_stop_reason.h",
     "arc_util.cc",
     "arc_util.h",
-    "connection_holder.h",
-    "connection_notifier.cc",
-    "connection_notifier.h",
-    "connection_observer.h",
-    "mojo_channel.h",
   ]
 
   deps = [
@@ -165,16 +160,33 @@ static_library("arc_base") {
     "//components/prefs",
     "//components/signin/core/account_id",
     "//components/user_manager",
+    "//content/public/common",
     "//mojo/edk",
     "//ui/aura",
   ]
 
   public_deps = [
+    ":connection_holder",
     ":prefs",
     "//components/arc/common",
   ]
 }
 
+source_set("connection_holder") {
+  sources = [
+    "connection_holder.h",
+    "connection_notifier.cc",
+    "connection_notifier.h",
+    "connection_observer.h",
+    "mojo_channel.h",
+  ]
+
+  deps = [
+    "//base",
+    "//mojo/public/cpp/bindings",
+  ]
+}
+
 static_library("arc_test_support") {
   testonly = true
   sources = [

components/arc/DEPS

@@ -22,6 +22,11 @@ include_rules = [
 ]
 
 specific_include_rules = {
+  "arc_bridge_host_impl.cc": [
+    "+ash/public",
+    "+content/public/common/service_manager_connection.h",
+    "+services/service_manager/public",
+  ],
   "arc_util.cc": [
     "+ui/aura",
   ],

components/arc/arc_bridge_host_impl.cc

@@ -7,10 +7,14 @@
 #include <algorithm>
 #include <utility>
 
+#include "ash/public/interfaces/ash_message_center_controller.mojom.h"
+#include "ash/public/interfaces/constants.mojom.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "components/arc/arc_bridge_service.h"
 #include "components/arc/mojo_channel.h"
+#include "content/public/common/service_manager_connection.h"
+#include "services/service_manager/public/cpp/connector.h"
 
 namespace arc {
 
@@ -139,8 +143,13 @@ void ArcBridgeHostImpl::OnNetInstanceReady(mojom::NetInstancePtr net_ptr) {
 
 void ArcBridgeHostImpl::OnNotificationsInstanceReady(
     mojom::NotificationsInstancePtr notifications_ptr) {
-  OnInstanceReady(arc_bridge_service_->notifications(),
-                  std::move(notifications_ptr));
+  // Forward notification instance to ash.
+  ash::mojom::AshMessageCenterControllerPtr ash_message_center_controller;
+  content::ServiceManagerConnection::GetForProcess()
+      ->GetConnector()
+      ->BindInterface(ash::mojom::kServiceName, &ash_message_center_controller);
+  ash_message_center_controller->SetArcNotificationsInstance(
+      std::move(notifications_ptr));
 }
 
 void ArcBridgeHostImpl::OnObbMounterInstanceReady(

components/arc/arc_bridge_service.h

@@ -51,8 +51,6 @@ class MidisHost;
 class MidisInstance;
 class NetHost;
 class NetInstance;
-class NotificationsHost;
-class NotificationsInstance;
 class ObbMounterHost;
 class ObbMounterInstance;
 class OemCryptoHost;
@@ -163,10 +161,6 @@ class ArcBridgeService {
     return &midis_;
   }
   ConnectionHolder<mojom::NetInstance, mojom::NetHost>* net() { return &net_; }
-  ConnectionHolder<mojom::NotificationsInstance, mojom::NotificationsHost>*
-  notifications() {
-    return &notifications_;
-  }
   ConnectionHolder<mojom::ObbMounterInstance, mojom::ObbMounterHost>*
   obb_mounter() {
     return &obb_mounter_;
@@ -257,8 +251,6 @@ class ArcBridgeService {
   ConnectionHolder<mojom::MetricsInstance, mojom::MetricsHost> metrics_;
   ConnectionHolder<mojom::MidisInstance, mojom::MidisHost> midis_;
   ConnectionHolder<mojom::NetInstance, mojom::NetHost> net_;
-  ConnectionHolder<mojom::NotificationsInstance, mojom::NotificationsHost>
-      notifications_;
   ConnectionHolder<mojom::ObbMounterInstance, mojom::ObbMounterHost>
       obb_mounter_;
   ConnectionHolder<mojom::OemCryptoInstance, mojom::OemCryptoHost> oemcrypto_;

components/arc/common/BUILD.gn

@@ -17,7 +17,6 @@ if (is_chromeos) {
       "audio.mojom",
       "auth.mojom",
       "backup_settings.mojom",
-      "bitmap.mojom",
       "bluetooth.mojom",
       "boot_phase_monitor.mojom",
       "cast_receiver.mojom",
@@ -33,7 +32,6 @@ if (is_chromeos) {
       "metrics.mojom",
       "midis.mojom",
       "net.mojom",
-      "notifications.mojom",
       "obb_mounter.mojom",
       "oemcrypto.mojom",
       "oemcrypto_daemon.mojom",
@@ -58,12 +56,24 @@ if (is_chromeos) {
 
     public_deps = [
       ":media",
+      ":notifications",
       "//device/usb/public/mojom",
       "//mojo/common:common_custom_types",
       "//mojo/public/mojom/base",
       "//ui/gfx/geometry/mojo",
     ]
   }
+
+  mojom("notifications") {
+    sources = [
+      "bitmap.mojom",
+      "notifications.mojom",
+    ]
+
+    deps = [
+      ":media",  # for gfx.mojom
+    ]
+  }
 }
 
 # Media related mojo interfaces. There are used by

ui/arc/BUILD.gn

@@ -28,11 +28,10 @@ static_library("arc") {
   ]
 
   deps = [
-    "//ash",
     "//base",
-    "//components/arc:arc_base",
+    "//components/arc:connection_holder",
+    "//components/arc/common:notifications",
     "//components/exo",
-    "//components/keyed_service/content:content",
     "//components/signin/core/account_id",
     "//mojo/public/cpp/system",
     "//skia",

ui/arc/notification/arc_notification_content_view.cc

@@ -4,7 +4,8 @@
 
 #include "ui/arc/notification/arc_notification_content_view.h"
 
-#include "ash/wm/window_util.h"
+// TODO(https://crbug.com/768439): Remove nogncheck when moved to ash.
+#include "ash/wm/window_util.h"  // nogncheck
 #include "base/auto_reset.h"
 #include "components/exo/notification_surface.h"
 #include "components/exo/surface.h"

ui/arc/notification/arc_notification_item_impl.cc

@@ -7,7 +7,8 @@
 #include <utility>
 #include <vector>
 
-#include "ash/shelf/shelf_constants.h"
+// TODO(https://crbug.com/768439): Remove nogncheck when moved to ash.
+#include "ash/shelf/shelf_constants.h"  // nogncheck
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "ui/arc/notification/arc_notification_delegate.h"

ui/arc/notification/arc_notification_manager.cc

@@ -7,13 +7,13 @@
 #include <memory>
 #include <utility>
 
-#include "base/memory/ptr_util.h"
-#include "base/memory/singleton.h"
+// TODO(https://crbug.com/768439): Remove nogncheck when moved to ash.
+#include "ash/login_status.h"                // nogncheck
+#include "ash/session/session_controller.h"  // nogncheck
+#include "ash/shell.h"                       // nogncheck
 #include "base/stl_util.h"
 #include "base/strings/utf_string_conversions.h"
-#include "components/arc/arc_bridge_service.h"
-#include "components/arc/arc_browser_context_keyed_service_factory_base.h"
-#include "components/arc/arc_util.h"
+#include "components/arc/mojo_channel.h"
 #include "ui/arc/notification/arc_notification_delegate.h"
 #include "ui/arc/notification/arc_notification_item_impl.h"
 #include "ui/arc/notification/arc_notification_view.h"
@@ -33,43 +33,51 @@ std::unique_ptr<message_center::MessageView> CreateCustomMessageView(
   return arc_delegate->CreateCustomMessageView(notification);
 }
 
-// Singleton factory for ArcNotificationManager.
-class ArcNotificationManagerFactory
-    : public internal::ArcBrowserContextKeyedServiceFactoryBase<
-          ArcNotificationManager,
-          ArcNotificationManagerFactory> {
+bool IsPublicSessionOrKiosk() {
+  const ash::LoginStatus login_status =
+      ash::Shell::Get()->session_controller()->login_status();
+
+  return login_status == ash::LoginStatus::PUBLIC ||
+         login_status == ash::LoginStatus::KIOSK_APP ||
+         login_status == ash::LoginStatus::ARC_KIOSK_APP;
+}
+
+}  // namespace
+
+class ArcNotificationManager::InstanceOwner {
  public:
-  // Factory name used by ArcBrowserContextKeyedServiceFactoryBase.
-  static constexpr const char* kName = "ArcNotificationManagerFactory";
+  InstanceOwner() = default;
+  ~InstanceOwner() = default;
+
+  void SetInstancePtr(mojom::NotificationsInstancePtr instance_ptr) {
+    DCHECK(!channel_);
+
+    channel_ = std::make_unique<
+        MojoChannel<mojom::NotificationsInstance, mojom::NotificationsHost>>(
+        &holder_, std::move(instance_ptr));
 
-  static ArcNotificationManagerFactory* GetInstance() {
-    return base::Singleton<ArcNotificationManagerFactory>::get();
+    // Using base::Unretained because |this| owns |channel_|.
+    channel_->set_connection_error_handler(
+        base::BindOnce(&InstanceOwner::OnDisconnected, base::Unretained(this)));
+    channel_->QueryVersion();
+  }
+
+  ConnectionHolder<mojom::NotificationsInstance, mojom::NotificationsHost>*
+  holder() {
+    return &holder_;
   }
 
  private:
-  friend base::DefaultSingletonTraits<ArcNotificationManagerFactory>;
-  ArcNotificationManagerFactory() = default;
-  ~ArcNotificationManagerFactory() override = default;
-};
+  void OnDisconnected() { channel_.reset(); }
 
-}  // namespace
+  ConnectionHolder<mojom::NotificationsInstance, mojom::NotificationsHost>
+      holder_;
+  std::unique_ptr<
+      MojoChannel<mojom::NotificationsInstance, mojom::NotificationsHost>>
+      channel_;
 
-// static
-ArcNotificationManager* ArcNotificationManager::GetForBrowserContext(
-    content::BrowserContext* context) {
-  return ArcNotificationManagerFactory::GetForBrowserContext(context);
-}
-
-// static
-std::unique_ptr<ArcNotificationManager>
-ArcNotificationManager::CreateForTesting(
-    ArcBridgeService* bridge_service,
-    const AccountId& main_profile_id,
-    message_center::MessageCenter* message_center) {
-  // MakeUnique cannot be used because the used ctor is private.
-  return base::WrapUnique(new ArcNotificationManager(
-      bridge_service, main_profile_id, message_center));
-}
+  DISALLOW_COPY_AND_ASSIGN(InstanceOwner);
+};
 
 // static
 void ArcNotificationManager::SetCustomNotificationViewFactory() {
@@ -77,28 +85,31 @@ void ArcNotificationManager::SetCustomNotificationViewFactory() {
       base::Bind(&CreateCustomMessageView));
 }
 
-ArcNotificationManager::ArcNotificationManager(content::BrowserContext* context,
-                                               ArcBridgeService* bridge_service)
-    : ArcNotificationManager(bridge_service,
-                             ArcServiceManager::Get()->account_id(),
-                             message_center::MessageCenter::Get()) {}
-
 ArcNotificationManager::ArcNotificationManager(
-    ArcBridgeService* bridge_service,
     const AccountId& main_profile_id,
     message_center::MessageCenter* message_center)
-    : arc_bridge_service_(bridge_service),
-      main_profile_id_(main_profile_id),
-      message_center_(message_center) {
-  arc_bridge_service_->notifications()->SetHost(this);
-  arc_bridge_service_->notifications()->AddObserver(this);
+    : main_profile_id_(main_profile_id),
+      message_center_(message_center),
+      instance_owner_(std::make_unique<InstanceOwner>()) {
+  instance_owner_->holder()->SetHost(this);
+  instance_owner_->holder()->AddObserver(this);
   if (!message_center::MessageViewFactory::HasCustomNotificationViewFactory())
     SetCustomNotificationViewFactory();
 }
 
 ArcNotificationManager::~ArcNotificationManager() {
-  arc_bridge_service_->notifications()->RemoveObserver(this);
-  arc_bridge_service_->notifications()->SetHost(nullptr);
+  instance_owner_->holder()->RemoveObserver(this);
+  instance_owner_->holder()->SetHost(nullptr);
+}
+
+void ArcNotificationManager::SetInstance(
+    mojom::NotificationsInstancePtr instance) {
+  instance_owner_->SetInstancePtr(std::move(instance));
+}
+
+ConnectionHolder<mojom::NotificationsInstance, mojom::NotificationsHost>*
+ArcNotificationManager::GetConnectionHolderForTest() {
+  return instance_owner_->holder();
 }
 
 void ArcNotificationManager::OnConnectionReady() {
@@ -187,7 +198,7 @@ void ArcNotificationManager::SendNotificationRemovedFromChrome(
   items_.erase(it);
 
   auto* notifications_instance = ARC_GET_INSTANCE_FOR_METHOD(
-      arc_bridge_service_->notifications(), SendNotificationEventToAndroid);
+      instance_owner_->holder(), SendNotificationEventToAndroid);
 
   // On shutdown, the ARC channel may quit earlier than notifications.
   if (!notifications_instance) {
@@ -209,7 +220,7 @@ void ArcNotificationManager::SendNotificationClickedOnChrome(
   }
 
   auto* notifications_instance = ARC_GET_INSTANCE_FOR_METHOD(
-      arc_bridge_service_->notifications(), SendNotificationEventToAndroid);
+      instance_owner_->holder(), SendNotificationEventToAndroid);
 
   // On shutdown, the ARC channel may quit earlier than notifications.
   if (!notifications_instance) {
@@ -232,7 +243,7 @@ void ArcNotificationManager::SendNotificationButtonClickedOnChrome(
   }
 
   auto* notifications_instance = ARC_GET_INSTANCE_FOR_METHOD(
-      arc_bridge_service_->notifications(), SendNotificationEventToAndroid);
+      instance_owner_->holder(), SendNotificationEventToAndroid);
 
   // On shutdown, the ARC channel may quit earlier than notifications.
   if (!notifications_instance) {
@@ -275,7 +286,7 @@ void ArcNotificationManager::CreateNotificationWindow(const std::string& key) {
   }
 
   auto* notifications_instance = ARC_GET_INSTANCE_FOR_METHOD(
-      arc_bridge_service_->notifications(), CreateNotificationWindow);
+      instance_owner_->holder(), CreateNotificationWindow);
   if (!notifications_instance)
     return;
 
@@ -290,7 +301,7 @@ void ArcNotificationManager::CloseNotificationWindow(const std::string& key) {
   }
 
   auto* notifications_instance = ARC_GET_INSTANCE_FOR_METHOD(
-      arc_bridge_service_->notifications(), CloseNotificationWindow);
+      instance_owner_->holder(), CloseNotificationWindow);
   if (!notifications_instance)
     return;
 
@@ -305,7 +316,7 @@ void ArcNotificationManager::OpenNotificationSettings(const std::string& key) {
   }
 
   auto* notifications_instance = ARC_GET_INSTANCE_FOR_METHOD(
-      arc_bridge_service_->notifications(), OpenNotificationSettings);
+      instance_owner_->holder(), OpenNotificationSettings);
 
   // On shutdown, the ARC channel may quit earlier than notifications.
   if (!notifications_instance)
@@ -316,7 +327,7 @@ void ArcNotificationManager::OpenNotificationSettings(const std::string& key) {
 
 bool ArcNotificationManager::IsOpeningSettingsSupported() const {
   const auto* notifications_instance = ARC_GET_INSTANCE_FOR_METHOD(
-      arc_bridge_service_->notifications(), OpenNotificationSettings);
+      instance_owner_->holder(), OpenNotificationSettings);
   return notifications_instance != nullptr;
 }
 
@@ -329,7 +340,7 @@ void ArcNotificationManager::SendNotificationToggleExpansionOnChrome(
   }
 
   auto* notifications_instance = ARC_GET_INSTANCE_FOR_METHOD(
-      arc_bridge_service_->notifications(), SendNotificationEventToAndroid);
+      instance_owner_->holder(), SendNotificationEventToAndroid);
 
   // On shutdown, the ARC channel may quit earlier than notifications.
   if (!notifications_instance) {
@@ -342,10 +353,6 @@ void ArcNotificationManager::SendNotificationToggleExpansionOnChrome(
       key, mojom::ArcNotificationEvent::TOGGLE_EXPANSION);
 }
 
-void ArcNotificationManager::Shutdown() {
-  get_app_id_callback_.Reset();
-}
-
 bool ArcNotificationManager::ShouldIgnoreNotification(
     arc::mojom::ArcNotificationData* data) {
   if (data->priority == mojom::ArcNotificationPriority::NONE)
@@ -354,7 +361,8 @@ bool ArcNotificationManager::ShouldIgnoreNotification(
   // Notifications from Play Store are ignored in Public Session and Kiosk mode.
   // TODO: Use centralized const for Play Store package.
   if (data->package_name.has_value() &&
-      *data->package_name == kPlayStorePackageName && IsRobotAccountMode()) {
+      *data->package_name == kPlayStorePackageName &&
+      IsPublicSessionOrKiosk()) {
     return true;
   }
 

ui/arc/notification/arc_notification_manager.h

@@ -10,47 +10,33 @@
 #include <unordered_map>
 
 #include "components/arc/common/notifications.mojom.h"
+#include "components/arc/connection_holder.h"
 #include "components/arc/connection_observer.h"
-#include "components/keyed_service/core/keyed_service.h"
 #include "components/signin/core/account_id/account_id.h"
 #include "ui/message_center/message_center.h"
 
-namespace content {
-class BrowserContext;
-}  // namespace content
-
 namespace arc {
 
-class ArcBridgeService;
 class ArcNotificationItem;
 
 class ArcNotificationManager
-    : public KeyedService,
-      public ConnectionObserver<mojom::NotificationsInstance>,
+    : public ConnectionObserver<mojom::NotificationsInstance>,
       public mojom::NotificationsHost {
  public:
-  // Returns singleton instance for the given BrowserContext,
-  // or nullptr if the browser |context| is not allowed to use ARC.
-  static ArcNotificationManager* GetForBrowserContext(
-      content::BrowserContext* context);
-
-  // Returns a created instance for testing.
-  static std::unique_ptr<ArcNotificationManager> CreateForTesting(
-      ArcBridgeService* bridge_service,
-      const AccountId& main_profile_id,
-      message_center::MessageCenter* message_center);
-
   // Sets the factory function to create ARC notification views. Exposed for
   // testing.
   static void SetCustomNotificationViewFactory();
 
-  // TODO(hidehiko): Make ctor private to enforce all service users should
-  // use GetForBrowserContext().
-  ArcNotificationManager(content::BrowserContext* context,
-                         ArcBridgeService* bridge_service);
+  ArcNotificationManager(const AccountId& main_profile_id,
+                         message_center::MessageCenter* message_center);
 
   ~ArcNotificationManager() override;
 
+  void SetInstance(mojom::NotificationsInstancePtr instance);
+
+  ConnectionHolder<mojom::NotificationsInstance, mojom::NotificationsHost>*
+  GetConnectionHolderForTest();
+
   void set_get_app_id_callback(
       base::RepeatingCallback<std::string(const std::string&)>
           get_app_id_callback) {
@@ -77,20 +63,15 @@ class ArcNotificationManager
   bool IsOpeningSettingsSupported() const;
   void SendNotificationToggleExpansionOnChrome(const std::string& key);
 
-  // Overridden from KeyedService:
-  void Shutdown() override;
-
  private:
-  ArcNotificationManager(ArcBridgeService* bridge_service,
-                         const AccountId& main_profile_id,
-                         message_center::MessageCenter* message_center);
+  // Helper class to own MojoChannel and ConnectionHolder.
+  class InstanceOwner;
 
   bool ShouldIgnoreNotification(mojom::ArcNotificationData* data);
 
   // Calls |get_app_id_callback_| to retrieve the app id from ArcAppListPrefs.
   std::string GetAppId(const std::string& package_name) const;
 
-  ArcBridgeService* const arc_bridge_service_;  // Owned by ArcServiceManager.
   const AccountId main_profile_id_;
   message_center::MessageCenter* const message_center_;
 
@@ -102,6 +83,10 @@ class ArcNotificationManager
 
   bool ready_ = false;
 
+  // Put as a last member to ensure that any callback tied to the elements
+  // is not invoked.
+  std::unique_ptr<InstanceOwner> instance_owner_;
+
   DISALLOW_COPY_AND_ASSIGN(ArcNotificationManager);
 };
 

ui/arc/notification/arc_notification_manager_unittest.cc

@@ -10,10 +10,11 @@
 
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
-#include "components/arc/arc_bridge_service.h"
 #include "components/arc/connection_holder.h"
 #include "components/arc/test/connection_holder_util.h"
 #include "components/arc/test/fake_notifications_instance.h"
+#include "mojo/public/cpp/bindings/binding.h"
+#include "mojo/public/cpp/bindings/interface_ptr.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/arc/notification/arc_notification_manager.h"
 #include "ui/message_center/fake_message_center.h"
@@ -63,8 +64,8 @@ class MockMessageCenter : public message_center::FakeMessageCenter {
 
 class ArcNotificationManagerTest : public testing::Test {
  public:
-  ArcNotificationManagerTest() {}
-  ~ArcNotificationManagerTest() override { base::RunLoop().RunUntilIdle(); }
+  ArcNotificationManagerTest() = default;
+  ~ArcNotificationManagerTest() override = default;
 
  protected:
   FakeNotificationsInstance* arc_notifications_instance() {
@@ -91,32 +92,40 @@ class ArcNotificationManagerTest : public testing::Test {
     return key;
   }
 
- private:
-  base::MessageLoop loop_;
-  std::unique_ptr<ArcBridgeService> service_;
-  std::unique_ptr<FakeNotificationsInstance> arc_notifications_instance_;
-  std::unique_ptr<ArcNotificationManager> arc_notification_manager_;
-  std::unique_ptr<MockMessageCenter> message_center_;
+  void FlushInstanceCall() { binding_->FlushForTesting(); }
 
+ private:
   void SetUp() override {
     arc_notifications_instance_ = std::make_unique<FakeNotificationsInstance>();
-    service_ = std::make_unique<ArcBridgeService>();
     message_center_ = std::make_unique<MockMessageCenter>();
 
-    arc_notification_manager_ = ArcNotificationManager::CreateForTesting(
-        service_.get(), EmptyAccountId(), message_center_.get());
+    arc_notification_manager_ = std::make_unique<ArcNotificationManager>(
+        EmptyAccountId(), message_center_.get());
+
+    binding_ = std::make_unique<mojo::Binding<mojom::NotificationsInstance>>(
+        arc_notifications_instance_.get());
+    mojom::NotificationsInstancePtr instance_ptr;
+    binding_->Bind(mojo::MakeRequest(&instance_ptr));
 
-    service_->notifications()->SetInstance(arc_notifications_instance_.get());
-    WaitForInstanceReady(service_->notifications());
+    arc_notification_manager_->SetInstance(std::move(instance_ptr));
+    WaitForInstanceReady(
+        arc_notification_manager_->GetConnectionHolderForTest());
   }
 
   void TearDown() override {
     arc_notification_manager_.reset();
     message_center_.reset();
-    service_.reset();
+    binding_.reset();
     arc_notifications_instance_.reset();
+    base::RunLoop().RunUntilIdle();
   }
 
+  base::MessageLoop loop_;
+  std::unique_ptr<FakeNotificationsInstance> arc_notifications_instance_;
+  std::unique_ptr<mojo::Binding<mojom::NotificationsInstance>> binding_;
+  std::unique_ptr<ArcNotificationManager> arc_notification_manager_;
+  std::unique_ptr<MockMessageCenter> message_center_;
+
   DISALLOW_COPY_AND_ASSIGN(ArcNotificationManagerTest);
 };
 
@@ -142,6 +151,8 @@ TEST_F(ArcNotificationManagerTest, NotificationRemovedByChrome) {
     // |notification| gets stale here.
   }
 
+  FlushInstanceCall();
+
   ASSERT_EQ(1u, arc_notifications_instance()->events().size());
   EXPECT_EQ(key, arc_notifications_instance()->events().at(0).first);
   EXPECT_EQ(mojom::ArcNotificationEvent::CLOSED,