GMC: Make MediaNotificationItem independent of Ash

This CL updates the relationship between MediaNotificationItem and
MediaNotificationController (now MediaNotificationControllerImpl) such
that MediaNotificationItem is only responsible for deciding when to
show or hide a media notification, and isn't responsible for how that
media notification is displayed. This is done by creating a
MediaNotificationController interface that
MediaNotificationControllerImpl implements.

This CL also updates the relationship of MediaNotificationItem and
MediaNotificationView so that MediaNotificationView calls methods on
MediaNotificationItem directly instead of calling them through the
MediaNotificationController or the message_center::MessageCenter.

This is part of a refactoring of the media notification to make it
reusable outside of ash/media.

Bug: 959478
Change-Id: I77d3f0329f0e8ee32d4cbb79829a0fdf50ac8113
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1615497Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Becca Hughes <beccahughes@chromium.org>
Commit-Queue: Tommy Steimel <steimel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#660873}

ash/BUILD.gn

@@ -453,8 +453,9 @@ component("ash") {
     "media/media_notification_background.h",
     "media/media_notification_constants.cc",
     "media/media_notification_constants.h",
-    "media/media_notification_controller.cc",
     "media/media_notification_controller.h",
+    "media/media_notification_controller_impl.cc",
+    "media/media_notification_controller_impl.h",
     "media/media_notification_item.cc",
     "media/media_notification_item.h",
     "media/media_notification_view.cc",
@@ -1693,7 +1694,7 @@ test("ash_unittests") {
     "magnifier/partial_magnification_controller_unittest.cc",
     "media/media_controller_unittest.cc",
     "media/media_notification_background_unittest.cc",
-    "media/media_notification_controller_unittest.cc",
+    "media/media_notification_controller_impl_unittest.cc",
     "media/media_notification_view_unittest.cc",
     "metrics/demo_session_metrics_recorder_unittest.cc",
     "metrics/desktop_task_switch_metric_recorder_unittest.cc",

ash/media/media_notification_controller.h

-// Copyright 2018 The Chromium Authors. All rights reserved.
+// Copyright 2019 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 ASH_MEDIA_MEDIA_NOTIFICATION_CONTROLLER_H_
 #define ASH_MEDIA_MEDIA_NOTIFICATION_CONTROLLER_H_
 
-#include <map>
-#include <memory>
 #include <string>
 
 #include "ash/ash_export.h"
-#include "ash/media/media_notification_item.h"
-#include "base/macros.h"
-#include "mojo/public/cpp/bindings/binding.h"
-#include "services/media_session/public/mojom/audio_focus.mojom.h"
-#include "services/media_session/public/mojom/media_controller.mojom.h"
-
-namespace service_manager {
-class Connector;
-}  // namespace service_manager
 
 namespace ash {
 
-namespace {
-class MediaNotificationBlocker;
-}  // namespace
-
-class MediaNotificationItem;
-class MediaNotificationView;
-
-// MediaNotificationController will show/hide media notifications when media
-// sessions are active. These notifications will show metadata and playback
-// controls.
-class ASH_EXPORT MediaNotificationController
-    : public media_session::mojom::AudioFocusObserver {
+// MediaNotificationController does the actual hiding and showing of the media
+// notification.
+class ASH_EXPORT MediaNotificationController {
  public:
-  // The name of the histogram used to record the number of concurrent media
-  // notifications.
-  static const char kCountHistogramName[];
-
-  explicit MediaNotificationController(service_manager::Connector* connector);
-  ~MediaNotificationController() override;
-
-  // media_session::mojom::AudioFocusObserver:
-  void OnFocusGained(
-      media_session::mojom::AudioFocusRequestStatePtr session) override;
-  void OnFocusLost(
-      media_session::mojom::AudioFocusRequestStatePtr session) override;
-
-  void SetView(const std::string& id, MediaNotificationView* view);
-
-  MediaNotificationItem* GetItem(const std::string& id) {
-    auto it = notifications_.find(id);
-    DCHECK(it != notifications_.end());
-    return &it->second;
-  }
-
-  // Called by |MediaNotificationItem| when it displays a new media
-  // notification. It will record the concurrent number of media notifications
-  // displayed.
-  void RecordConcurrentNotificationCount();
-
- private:
-  media_session::mojom::MediaControllerManagerPtr controller_manager_ptr_;
-
-  mojo::Binding<media_session::mojom::AudioFocusObserver>
-      audio_focus_observer_binding_{this};
-
-  // Stores a |MediaNotificationItem| for each media session keyed by its
-  // |request_id| in string format.
-  std::map<const std::string, MediaNotificationItem> notifications_;
-
-  std::unique_ptr<MediaNotificationBlocker> blocker_;
-
-  DISALLOW_COPY_AND_ASSIGN(MediaNotificationController);
+  // Shows/hides a notification with the given request id. Called by
+  // MediaNotificationItem when the notification should be shown/hidden.
+  virtual void ShowNotification(const std::string& id) = 0;
+  virtual void HideNotification(const std::string& id) = 0;
 };
 
 }  // namespace ash

ash/media/media_notification_controller.cc → ash/media/media_notification_controller_impl.cc

@@ -2,10 +2,12 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "ash/media/media_notification_controller.h"
+#include "ash/media/media_notification_controller_impl.h"
 
 #include "ash/media/media_notification_constants.h"
+#include "ash/media/media_notification_item.h"
 #include "ash/media/media_notification_view.h"
+#include "ash/public/cpp/notification_utils.h"
 #include "ash/session/session_controller_impl.h"
 #include "ash/session/session_observer.h"
 #include "ash/shell.h"
@@ -16,6 +18,8 @@
 #include "services/service_manager/public/cpp/connector.h"
 #include "ui/message_center/message_center.h"
 #include "ui/message_center/notification_blocker.h"
+#include "ui/message_center/public/cpp/notification.h"
+#include "ui/message_center/public/cpp/notifier_id.h"
 #include "ui/message_center/views/message_view_factory.h"
 
 namespace ash {
@@ -26,7 +30,10 @@ std::unique_ptr<message_center::MessageView> CreateCustomMediaNotificationView(
     const message_center::Notification& notification) {
   DCHECK_EQ(kMediaSessionNotificationCustomViewType,
             notification.custom_view_type());
-  return std::make_unique<MediaNotificationView>(notification);
+  auto* controller = Shell::Get()->media_notification_controller();
+  if (controller)
+    return controller->CreateMediaNotification(notification);
+  return nullptr;
 }
 
 // The maximum number of media notifications to count when recording the
@@ -95,10 +102,10 @@ class MediaNotificationBlocker : public message_center::NotificationBlocker,
 }  // namespace
 
 // static
-const char MediaNotificationController::kCountHistogramName[] =
+const char MediaNotificationControllerImpl::kCountHistogramName[] =
     "Media.Notification.Count";
 
-MediaNotificationController::MediaNotificationController(
+MediaNotificationControllerImpl::MediaNotificationControllerImpl(
     service_manager::Connector* connector)
     : blocker_(std::make_unique<MediaNotificationBlocker>(
           message_center::MessageCenter::Get(),
@@ -125,9 +132,9 @@ MediaNotificationController::MediaNotificationController(
   audio_focus_ptr->AddObserver(std::move(audio_focus_observer));
 }
 
-MediaNotificationController::~MediaNotificationController() = default;
+MediaNotificationControllerImpl::~MediaNotificationControllerImpl() = default;
 
-void MediaNotificationController::OnFocusGained(
+void MediaNotificationControllerImpl::OnFocusGained(
     media_session::mojom::AudioFocusRequestStatePtr session) {
   const std::string id = session->request_id->ToString();
 
@@ -145,25 +152,60 @@ void MediaNotificationController::OnFocusGained(
 
   notifications_.emplace(
       std::piecewise_construct, std::forward_as_tuple(id),
-      std::forward_as_tuple(id, session->source_name.value_or(std::string()),
-                            std::move(controller),
-                            std::move(session->session_info)));
+      std::forward_as_tuple(
+          this, id, session->source_name.value_or(std::string()),
+          std::move(controller), std::move(session->session_info)));
 }
 
-void MediaNotificationController::OnFocusLost(
+void MediaNotificationControllerImpl::OnFocusLost(
     media_session::mojom::AudioFocusRequestStatePtr session) {
   notifications_.erase(session->request_id->ToString());
 }
 
-void MediaNotificationController::SetView(const std::string& id,
-                                          MediaNotificationView* view) {
-  auto it = notifications_.find(id);
-  if (it == notifications_.end())
+void MediaNotificationControllerImpl::ShowNotification(const std::string& id) {
+  // If a notification already exists, do nothing.
+  if (message_center::MessageCenter::Get()->FindVisibleNotificationById(id))
     return;
-  it->second.SetView(view);
+
+  std::unique_ptr<message_center::Notification> notification =
+      ash::CreateSystemNotification(
+          message_center::NotificationType::NOTIFICATION_TYPE_CUSTOM, id,
+          base::string16(), base::string16(), base::string16(), GURL(),
+          message_center::NotifierId(
+              message_center::NotifierType::SYSTEM_COMPONENT,
+              kMediaSessionNotifierId),
+          message_center::RichNotificationData(), nullptr, gfx::VectorIcon(),
+          message_center::SystemNotificationWarningLevel::NORMAL);
+
+  // Set the priority to low to prevent the notification showing as a popup and
+  // keep it at the bottom of the list.
+  notification->set_priority(message_center::LOW_PRIORITY);
+
+  notification->set_custom_view_type(kMediaSessionNotificationCustomViewType);
+
+  message_center::MessageCenter::Get()->AddNotification(
+      std::move(notification));
+
+  RecordConcurrentNotificationCount();
+}
+
+void MediaNotificationControllerImpl::HideNotification(const std::string& id) {
+  message_center::MessageCenter::Get()->RemoveNotification(id, false);
+}
+
+std::unique_ptr<MediaNotificationView>
+MediaNotificationControllerImpl::CreateMediaNotification(
+    const message_center::Notification& notification) {
+  base::WeakPtr<MediaNotificationItem> item;
+
+  auto it = notifications_.find(notification.id());
+  if (it != notifications_.end())
+    item = it->second.GetWeakPtr();
+
+  return std::make_unique<MediaNotificationView>(notification, std::move(item));
 }
 
-void MediaNotificationController::RecordConcurrentNotificationCount() {
+void MediaNotificationControllerImpl::RecordConcurrentNotificationCount() {
   UMA_HISTOGRAM_EXACT_LINEAR(
       kCountHistogramName,
       message_center::MessageCenter::Get()

ash/media/media_notification_controller_impl.h

+// Copyright 2018 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 ASH_MEDIA_MEDIA_NOTIFICATION_CONTROLLER_IMPL_H_
+#define ASH_MEDIA_MEDIA_NOTIFICATION_CONTROLLER_IMPL_H_
+
+#include <map>
+#include <memory>
+#include <string>
+
+#include "ash/ash_export.h"
+#include "ash/media/media_notification_controller.h"
+#include "ash/media/media_notification_item.h"
+#include "base/macros.h"
+#include "mojo/public/cpp/bindings/binding.h"
+#include "services/media_session/public/mojom/audio_focus.mojom.h"
+#include "services/media_session/public/mojom/media_controller.mojom.h"
+
+namespace service_manager {
+class Connector;
+}  // namespace service_manager
+
+namespace message_center {
+class Notification;
+}  // namespace message_center
+
+namespace ash {
+
+namespace {
+class MediaNotificationBlocker;
+}  // namespace
+
+class MediaNotificationView;
+
+// MediaNotificationControllerImpl will show/hide media notifications when media
+// sessions are active. These notifications will show metadata and playback
+// controls.
+class ASH_EXPORT MediaNotificationControllerImpl
+    : public media_session::mojom::AudioFocusObserver,
+      public MediaNotificationController {
+ public:
+  // The name of the histogram used to record the number of concurrent media
+  // notifications.
+  static const char kCountHistogramName[];
+
+  explicit MediaNotificationControllerImpl(
+      service_manager::Connector* connector);
+  ~MediaNotificationControllerImpl() override;
+
+  // media_session::mojom::AudioFocusObserver:
+  void OnFocusGained(
+      media_session::mojom::AudioFocusRequestStatePtr session) override;
+  void OnFocusLost(
+      media_session::mojom::AudioFocusRequestStatePtr session) override;
+
+  // MediaNotificationController:
+  void ShowNotification(const std::string& id) override;
+  void HideNotification(const std::string& id) override;
+
+  std::unique_ptr<MediaNotificationView> CreateMediaNotification(
+      const message_center::Notification& notification);
+
+  ash::MediaNotificationItem* GetItem(const std::string& id) {
+    auto it = notifications_.find(id);
+    DCHECK(it != notifications_.end());
+    return &it->second;
+  }
+
+ private:
+  // Called when we display a new media notification. It will record the
+  // concurrent number of media notifications displayed.
+  void RecordConcurrentNotificationCount();
+
+  media_session::mojom::MediaControllerManagerPtr controller_manager_ptr_;
+
+  mojo::Binding<media_session::mojom::AudioFocusObserver>
+      audio_focus_observer_binding_{this};
+
+  // Stores a |MediaNotificationItem| for each media session keyed by its
+  // |request_id| in string format.
+  std::map<const std::string, MediaNotificationItem> notifications_;
+
+  std::unique_ptr<MediaNotificationBlocker> blocker_;
+
+  DISALLOW_COPY_AND_ASSIGN(MediaNotificationControllerImpl);
+};
+
+}  // namespace ash
+
+#endif  // ASH_MEDIA_MEDIA_NOTIFICATION_CONTROLLER_IMPL_H_

ash/media/media_notification_controller_unittest.cc → ash/media/media_notification_controller_impl_unittest.cc

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "ash/media/media_notification_controller.h"
+#include "ash/media/media_notification_controller_impl.h"
 
 #include <memory>
 
@@ -44,10 +44,10 @@ media_session::mojom::AudioFocusRequestStatePtr GetRequestStateWithId(
 
 }  // namespace
 
-class MediaNotificationControllerTest : public AshTestBase {
+class MediaNotificationControllerImplTest : public AshTestBase {
  public:
-  MediaNotificationControllerTest() = default;
-  ~MediaNotificationControllerTest() override = default;
+  MediaNotificationControllerImplTest() = default;
+  ~MediaNotificationControllerImplTest() override = default;
 
   // AshTestBase
   void SetUp() override {
@@ -77,7 +77,7 @@ class MediaNotificationControllerTest : public AshTestBase {
 
   void ExpectHistogramCountRecorded(int count, int size) {
     histogram_tester_.ExpectBucketCount(
-        MediaNotificationController::kCountHistogramName, count, size);
+        MediaNotificationControllerImpl::kCountHistogramName, count, size);
   }
 
   void ExpectHistogramSourceRecorded(MediaNotificationItem::Source source) {
@@ -98,12 +98,12 @@ class MediaNotificationControllerTest : public AshTestBase {
 
   base::HistogramTester histogram_tester_;
 
-  DISALLOW_COPY_AND_ASSIGN(MediaNotificationControllerTest);
+  DISALLOW_COPY_AND_ASSIGN(MediaNotificationControllerImplTest);
 };
 
 // Test toggling the notification multiple times with the same ID. Since the
 // notification is keyed by ID we should only ever show one.
-TEST_F(MediaNotificationControllerTest, OnFocusGainedLost_SameId) {
+TEST_F(MediaNotificationControllerImplTest, OnFocusGainedLost_SameId) {
   base::UnguessableToken id = base::UnguessableToken::Create();
 
   ExpectNotificationCount(0);
@@ -133,7 +133,7 @@ TEST_F(MediaNotificationControllerTest, OnFocusGainedLost_SameId) {
 
 // Test toggling the notification multiple times with different IDs. This should
 // show one notification per ID.
-TEST_F(MediaNotificationControllerTest, OnFocusGainedLost_MultipleIds) {
+TEST_F(MediaNotificationControllerImplTest, OnFocusGainedLost_MultipleIds) {
   base::UnguessableToken id1 = base::UnguessableToken::Create();
   base::UnguessableToken id2 = base::UnguessableToken::Create();
 
@@ -169,8 +169,9 @@ TEST_F(MediaNotificationControllerTest, OnFocusGainedLost_MultipleIds) {
 }
 
 // Test that a notification is hidden when it becomes uncontrollable. We still
-// keep the MediaNotificationItem around in case it becomes controllable again.
-TEST_F(MediaNotificationControllerTest,
+// keep the MediaNotificationItem around in case it becomes
+// controllable again.
+TEST_F(MediaNotificationControllerImplTest,
        OnFocusGained_ControllableBecomesUncontrollable) {
   base::UnguessableToken id = base::UnguessableToken::Create();
 
@@ -196,7 +197,7 @@ TEST_F(MediaNotificationControllerTest,
 }
 
 // Test that a notification is shown when it becomes controllable.
-TEST_F(MediaNotificationControllerTest,
+TEST_F(MediaNotificationControllerImplTest,
        OnFocusGained_NotControllableBecomesControllable) {
   base::UnguessableToken id = base::UnguessableToken::Create();
 
@@ -225,7 +226,7 @@ TEST_F(MediaNotificationControllerTest,
 }
 
 // Test hiding a notification with an invalid ID.
-TEST_F(MediaNotificationControllerTest, OnFocusLost_Noop) {
+TEST_F(MediaNotificationControllerImplTest, OnFocusLost_Noop) {
   ExpectNotificationCount(0);
 
   Shell::Get()->media_notification_controller()->OnFocusLost(
@@ -235,7 +236,7 @@ TEST_F(MediaNotificationControllerTest, OnFocusLost_Noop) {
 }
 
 // Test that media notifications have the correct custom view type.
-TEST_F(MediaNotificationControllerTest, NotificationHasCustomViewType) {
+TEST_F(MediaNotificationControllerImplTest, NotificationHasCustomViewType) {
   ExpectNotificationCount(0);
 
   base::UnguessableToken id = base::UnguessableToken::Create();
@@ -262,7 +263,7 @@ TEST_F(MediaNotificationControllerTest, NotificationHasCustomViewType) {
 
 // Test that if we recieve a null media session info that we hide the
 // notification.
-TEST_F(MediaNotificationControllerTest, HandleNullMediaSessionInfo) {
+TEST_F(MediaNotificationControllerImplTest, HandleNullMediaSessionInfo) {
   ExpectNotificationCount(0);
 
   base::UnguessableToken id = base::UnguessableToken::Create();
@@ -286,7 +287,7 @@ TEST_F(MediaNotificationControllerTest, HandleNullMediaSessionInfo) {
   ExpectNotificationCount(0);
 }
 
-TEST_F(MediaNotificationControllerTest, MediaMetadata_NoArtist) {
+TEST_F(MediaNotificationControllerImplTest, MediaMetadata_NoArtist) {
   base::UnguessableToken id = base::UnguessableToken::Create();
 
   ExpectNotificationCount(0);
@@ -305,7 +306,7 @@ TEST_F(MediaNotificationControllerTest, MediaMetadata_NoArtist) {
   ExpectNotificationCount(0);
 }
 
-TEST_F(MediaNotificationControllerTest, MediaMetadata_NoTitle) {
+TEST_F(MediaNotificationControllerImplTest, MediaMetadata_NoTitle) {
   base::UnguessableToken id = base::UnguessableToken::Create();
 
   ExpectNotificationCount(0);
@@ -324,7 +325,7 @@ TEST_F(MediaNotificationControllerTest, MediaMetadata_NoTitle) {
   ExpectNotificationCount(0);
 }
 
-TEST_F(MediaNotificationControllerTest, MediaMetadataUpdated_MissingInfo) {
+TEST_F(MediaNotificationControllerImplTest, MediaMetadataUpdated_MissingInfo) {
   base::UnguessableToken id = base::UnguessableToken::Create();
 
   ExpectNotificationCount(0);
@@ -348,7 +349,7 @@ TEST_F(MediaNotificationControllerTest, MediaMetadataUpdated_MissingInfo) {
   ExpectNotificationCount(0);
 }
 
-TEST_F(MediaNotificationControllerTest, RecordHistogramSource_Unknown) {
+TEST_F(MediaNotificationControllerImplTest, RecordHistogramSource_Unknown) {
   base::UnguessableToken id = base::UnguessableToken::Create();
 
   ExpectNotificationCount(0);
@@ -365,7 +366,7 @@ TEST_F(MediaNotificationControllerTest, RecordHistogramSource_Unknown) {
   ExpectHistogramSourceRecorded(MediaNotificationItem::Source::kUnknown);
 }
 
-TEST_F(MediaNotificationControllerTest, RecordHistogramSource_Web) {
+TEST_F(MediaNotificationControllerImplTest, RecordHistogramSource_Web) {
   base::UnguessableToken id = base::UnguessableToken::Create();
 
   ExpectNotificationCount(0);
@@ -386,7 +387,7 @@ TEST_F(MediaNotificationControllerTest, RecordHistogramSource_Web) {
   ExpectHistogramSourceRecorded(MediaNotificationItem::Source::kWeb);
 }
 
-TEST_F(MediaNotificationControllerTest, RecordHistogramSource_Assistant) {
+TEST_F(MediaNotificationControllerImplTest, RecordHistogramSource_Assistant) {
   base::UnguessableToken id = base::UnguessableToken::Create();
 
   ExpectNotificationCount(0);
@@ -407,7 +408,7 @@ TEST_F(MediaNotificationControllerTest, RecordHistogramSource_Assistant) {
   ExpectHistogramSourceRecorded(MediaNotificationItem::Source::kAssistant);
 }
 
-TEST_F(MediaNotificationControllerTest, RecordHistogramSource_Arc) {
+TEST_F(MediaNotificationControllerImplTest, RecordHistogramSource_Arc) {
   base::UnguessableToken id = base::UnguessableToken::Create();
 
   ExpectNotificationCount(0);
@@ -430,7 +431,7 @@ TEST_F(MediaNotificationControllerTest, RecordHistogramSource_Arc) {
 
 // Test that locking the screen will hide the media notifications. Unlocking the
 // screen should re-show the notifications.
-TEST_F(MediaNotificationControllerTest, HideWhenScreenLocked) {
+TEST_F(MediaNotificationControllerImplTest, HideWhenScreenLocked) {
   message_center::MessageCenter* message_center =
       message_center::MessageCenter::Get();
 

ash/media/media_notification_item.cc

@@ -7,22 +7,14 @@
 #include "ash/media/media_notification_constants.h"
 #include "ash/media/media_notification_controller.h"
 #include "ash/media/media_notification_view.h"
-#include "ash/public/cpp/notification_utils.h"
-#include "ash/shell.h"
 #include "base/bind.h"
 #include "base/metrics/histogram_macros.h"
-#include "base/strings/string16.h"
 #include "base/time/time.h"
 #include "services/media_session/public/mojom/constants.mojom.h"
 #include "services/media_session/public/mojom/media_controller.mojom.h"
 #include "services/media_session/public/mojom/media_session.mojom.h"
 #include "ui/gfx/image/image.h"
-#include "ui/message_center/message_center.h"
 #include "ui/message_center/public/cpp/message_center_constants.h"
-#include "ui/message_center/public/cpp/notification.h"
-#include "ui/message_center/public/cpp/notification_delegate.h"
-#include "ui/message_center/public/cpp/notifier_id.h"
-#include "url/gurl.h"
 
 namespace ash {
 
@@ -57,14 +49,18 @@ const char MediaNotificationItem::kUserActionHistogramName[] =
     "Media.Notification.UserAction";
 
 MediaNotificationItem::MediaNotificationItem(
-    const std::string& id,
+    MediaNotificationController* notification_controller,
+    const std::string& request_id,
     const std::string& source_name,
     media_session::mojom::MediaControllerPtr controller,
     media_session::mojom::MediaSessionInfoPtr session_info)
-    : id_(id),
+    : controller_(notification_controller),
+      request_id_(request_id),
       source_(GetSource(source_name)),
       media_controller_ptr_(std::move(controller)),
       session_info_(std::move(session_info)) {
+  DCHECK(controller_);
+
   if (media_controller_ptr_.is_bound()) {
     // Bind an observer to the associated media controller.
     media_session::mojom::MediaControllerObserverPtr media_controller_observer;
@@ -93,7 +89,7 @@ MediaNotificationItem::MediaNotificationItem(
 }
 
 MediaNotificationItem::~MediaNotificationItem() {
-  HideNotification();
+  controller_->HideNotification(request_id_);
 }
 
 void MediaNotificationItem::MediaSessionInfoChanged(
@@ -110,10 +106,19 @@ void MediaNotificationItem::MediaSessionMetadataChanged(
     const base::Optional<media_session::MediaMetadata>& metadata) {
   session_metadata_ = metadata.value_or(media_session::MediaMetadata());
 
+  view_needs_metadata_update_ = true;
+
   MaybeHideOrShowNotification();
 
-  if (view_)
+  // |MaybeHideOrShowNotification()| can synchronously create a
+  // MediaNotificationView that calls |SetView()|. If that happens, then we
+  // don't want to call |view_->UpdateWithMediaMetadata()| below since |view_|
+  // will have already received the metadata when calling |SetView()|.
+  // |view_needs_metadata_update_| is set to false in |SetView()|.
+  if (view_ && view_needs_metadata_update_)
     view_->UpdateWithMediaMetadata(session_metadata_);
+
+  view_needs_metadata_update_ = false;
 }
 
 void MediaNotificationItem::MediaSessionActionsChanged(
@@ -150,6 +155,7 @@ void MediaNotificationItem::SetView(MediaNotificationView* view) {
   view_ = view;
 
   if (view) {
+    view_needs_metadata_update_ = false;
     view_->UpdateWithMediaSessionInfo(session_info_);
     view_->UpdateWithMediaMetadata(session_metadata_);
     view_->UpdateWithMediaActions(session_actions_);
@@ -167,61 +173,28 @@ void MediaNotificationItem::MaybeHideOrShowNotification() {
   // If the |is_controllable| bit is set in MediaSessionInfo then we should show
   // a media notification.
   if (!session_info_ || !session_info_->is_controllable) {
-    HideNotification();
+    controller_->HideNotification(request_id_);
     return;
   }
 
   // If we do not have a title and an artist then we should hide the
   // notification.
   if (session_metadata_.title.empty() || session_metadata_.artist.empty()) {
-    HideNotification();
+    controller_->HideNotification(request_id_);
     return;
   }
 
-  if (message_center::MessageCenter::Get()->FindVisibleNotificationById(id_))
+  // If we have an existing view, then we don't need to create a new one.
+  if (view_)
     return;
 
-  std::unique_ptr<message_center::Notification> notification =
-      ash::CreateSystemNotification(
-          message_center::NotificationType::NOTIFICATION_TYPE_CUSTOM, id_,
-          base::string16(), base::string16(), base::string16(), GURL(),
-          message_center::NotifierId(
-              message_center::NotifierType::SYSTEM_COMPONENT,
-              kMediaSessionNotifierId),
-          message_center::RichNotificationData(),
-          base::MakeRefCounted<message_center::HandleNotificationClickDelegate>(
-              base::BindRepeating(&MediaNotificationItem::OnNotificationClicked,
-                                  weak_ptr_factory_.GetWeakPtr())),
-          gfx::VectorIcon(),
-          message_center::SystemNotificationWarningLevel::NORMAL);
-
-  // Set the priority to low to prevent the notification showing as a popup and
-  // keep it at the bottom of the list.
-  notification->set_priority(message_center::LOW_PRIORITY);
-
-  notification->set_custom_view_type(kMediaSessionNotificationCustomViewType);
-
-  message_center::MessageCenter::Get()->AddNotification(
-      std::move(notification));
-
-  Shell::Get()
-      ->media_notification_controller()
-      ->RecordConcurrentNotificationCount();
+  controller_->ShowNotification(request_id_);
 
   UMA_HISTOGRAM_ENUMERATION(kSourceHistogramName, source_);
 }
 
-void MediaNotificationItem::HideNotification() {
-  message_center::MessageCenter::Get()->RemoveNotification(id_, false);
-}
-
-void MediaNotificationItem::OnNotificationClicked(
-    base::Optional<int> button_id) {
-  if (!button_id)
-    return;
-
-  const MediaSessionAction action = static_cast<MediaSessionAction>(*button_id);
-
+void MediaNotificationItem::OnMediaSessionActionButtonPressed(
+    MediaSessionAction action) {
   UMA_HISTOGRAM_ENUMERATION(kUserActionHistogramName, action);
 
   switch (action) {

ash/media/media_notification_item.h

@@ -19,6 +19,7 @@
 
 namespace ash {
 
+class MediaNotificationController;
 class MediaNotificationView;
 
 // MediaNotificationItem manages hiding/showing a media notification and
@@ -43,7 +44,8 @@ class ASH_EXPORT MediaNotificationItem
     kMaxValue = kArc,
   };
 
-  MediaNotificationItem(const std::string& id,
+  MediaNotificationItem(MediaNotificationController* notification_controller,
+                        const std::string& request_id,
                         const std::string& source_name,
                         media_session::mojom::MediaControllerPtr controller,
                         media_session::mojom::MediaSessionInfoPtr session_info);
@@ -65,8 +67,16 @@ class ASH_EXPORT MediaNotificationItem
       media_session::mojom::MediaSessionImageType type,
       const SkBitmap& bitmap) override;
 
+  // Called by MediaNotificationView when created or destroyed.
   void SetView(MediaNotificationView* view);
 
+  void OnMediaSessionActionButtonPressed(
+      media_session::mojom::MediaSessionAction action);
+
+  base::WeakPtr<MediaNotificationItem> GetWeakPtr() {
+    return weak_ptr_factory_.GetWeakPtr();
+  }
+
   void FlushForTesting();
 
   void SetMediaControllerForTesting(
@@ -79,15 +89,14 @@ class ASH_EXPORT MediaNotificationItem
 
   void HideNotification();
 
+  MediaNotificationController* controller_;
+
   // Weak reference to the view of the currently shown media notification.
   MediaNotificationView* view_ = nullptr;
 
-  void OnNotificationClicked(base::Optional<int> button_id);
-
-  // The id is the |request_id| of the media session and is guaranteed to be
-  // globally unique. It is also used as the id of the notification for this
-  // media session.
-  const std::string id_;
+  // The |request_id_| is the request id of the media session and is guaranteed
+  // to be globally unique.
+  const std::string request_id_;
 
   // The source of the media session (e.g. arc, web).
   const Source source_;
@@ -104,6 +113,10 @@ class ASH_EXPORT MediaNotificationItem
 
   gfx::ImageSkia session_icon_;
 
+  // True if the metadata needs to be updated on |view_|. Used to prevent
+  // updating |view_|'s metadata twice on a single change.
+  bool view_needs_metadata_update_ = false;
+
   mojo::Binding<media_session::mojom::MediaControllerObserver>
       observer_binding_{this};
 

ash/media/media_notification_view.cc

@@ -6,7 +6,7 @@
 
 #include "ash/media/media_notification_background.h"
 #include "ash/media/media_notification_constants.h"
-#include "ash/media/media_notification_controller.h"
+#include "ash/media/media_notification_item.h"
 #include "ash/shell.h"
 #include "ash/strings/grit/ash_strings.h"
 #include "base/metrics/histogram_macros.h"
@@ -108,8 +108,9 @@ const char MediaNotificationView::kMetadataHistogramName[] =
     "Media.Notification.MetadataPresent";
 
 MediaNotificationView::MediaNotificationView(
-    const message_center::Notification& notification)
-    : message_center::MessageView(notification) {
+    const message_center::Notification& notification,
+    base::WeakPtr<MediaNotificationItem> item)
+    : message_center::MessageView(notification), item_(std::move(item)) {
   SetLayoutManager(std::make_unique<views::BoxLayout>(
       views::BoxLayout::kVertical, gfx::Insets(), 0));
 
@@ -209,13 +210,13 @@ MediaNotificationView::MediaNotificationView(
                      message_center::kNotificationCornerRadius);
   UpdateViewForExpandedState();
 
-  Shell::Get()->media_notification_controller()->SetView(notification_id(),
-                                                         this);
+  if (item_)
+    item_->SetView(this);
 }
 
 MediaNotificationView::~MediaNotificationView() {
-  Shell::Get()->media_notification_controller()->SetView(notification_id(),
-                                                         nullptr);
+  if (item_)
+    item_->SetView(nullptr);
 }
 
 void MediaNotificationView::UpdateWithNotification(
@@ -284,8 +285,10 @@ void MediaNotificationView::ButtonPressed(views::Button* sender,
   }
 
   if (sender->parent() == button_row_) {
-    message_center::MessageCenter::Get()->ClickOnNotificationButton(
-        notification_id(), sender->tag());
+    if (item_) {
+      item_->OnMediaSessionActionButtonPressed(
+          static_cast<MediaSessionAction>(sender->tag()));
+    }
     return;
   }
 

ash/media/media_notification_view.h

@@ -6,6 +6,7 @@
 #define ASH_MEDIA_MEDIA_NOTIFICATION_VIEW_H_
 
 #include "ash/ash_export.h"
+#include "base/memory/weak_ptr.h"
 #include "services/media_session/public/mojom/media_session.mojom.h"
 #include "ui/message_center/views/message_view.h"
 #include "ui/views/controls/button/button.h"
@@ -18,9 +19,6 @@ class ImageSkia;
 
 namespace media_session {
 struct MediaMetadata;
-}  // namespace media_session
-
-namespace media_session {
 enum class MediaSessionAction;
 }  // namespace media_session
 
@@ -37,6 +35,7 @@ class View;
 namespace ash {
 
 class MediaNotificationBackground;
+class MediaNotificationItem;
 
 // MediaNotificationView will show up as a custom notification. It will show the
 // currently playing media and provide playback controls. There will also be
@@ -63,8 +62,8 @@ class ASH_EXPORT MediaNotificationView : public message_center::MessageView,
     kMaxValue = kCount,
   };
 
-  explicit MediaNotificationView(
-      const message_center::Notification& notification);
+  MediaNotificationView(const message_center::Notification& notification,
+                        base::WeakPtr<MediaNotificationItem> item);
   ~MediaNotificationView() override;
 
   // message_center::MessageView:
@@ -115,6 +114,8 @@ class ASH_EXPORT MediaNotificationView : public message_center::MessageView,
 
   void UpdateForegroundColor();
 
+  base::WeakPtr<MediaNotificationItem> item_;
+
   // View containing close and settings buttons.
   std::unique_ptr<message_center::NotificationControlButtonsView>
       control_buttons_view_;

ash/media/media_notification_view_unittest.cc

@@ -9,7 +9,7 @@
 #include "ash/focus_cycler.h"
 #include "ash/media/media_notification_background.h"
 #include "ash/media/media_notification_constants.h"
-#include "ash/media/media_notification_controller.h"
+#include "ash/media/media_notification_controller_impl.h"
 #include "ash/media/media_notification_item.h"
 #include "ash/public/cpp/ash_features.h"
 #include "ash/public/cpp/shell_window_ids.h"
@@ -262,7 +262,8 @@ class MediaNotificationViewTest : public AshTestBase {
  private:
   std::unique_ptr<message_center::MessageView> CreateAndCaptureCustomView(
       const message_center::Notification& notification) {
-    auto view = std::make_unique<MediaNotificationView>(notification);
+    auto view = std::make_unique<MediaNotificationView>(
+        notification, GetItem()->GetWeakPtr());
     view_ = view.get();
     return view;
   }

ash/shell.cc

@@ -65,7 +65,7 @@
 #include "ash/magnifier/magnification_controller.h"
 #include "ash/magnifier/partial_magnification_controller.h"
 #include "ash/media/media_controller.h"
-#include "ash/media/media_notification_controller.h"
+#include "ash/media/media_notification_controller_impl.h"
 #include "ash/multi_device_setup/multi_device_notification_presenter.h"
 #include "ash/new_window_controller.h"
 #include "ash/note_taking_controller.h"
@@ -849,8 +849,8 @@ Shell::~Shell() {
   // before it.
   detachable_base_handler_.reset();
 
-  // MediaNotificationController depends on MessageCenter and must be destructed
-  // before it.
+  // MediaNotificationControllerImpl depends on MessageCenter and must be
+  // destructed before it.
   media_notification_controller_.reset();
 
   // Destroys the MessageCenter singleton, so must happen late.
@@ -1154,7 +1154,7 @@ void Shell::Init(
 
   if (base::FeatureList::IsEnabled(features::kMediaSessionNotification)) {
     media_notification_controller_ =
-        std::make_unique<MediaNotificationController>(connector_);
+        std::make_unique<MediaNotificationControllerImpl>(connector_);
   }
 
   for (auto& observer : shell_observers_)

ash/shell.h

@@ -140,7 +140,7 @@ class LoginScreenController;
 class MagnificationController;
 class TabletModeController;
 class MediaController;
-class MediaNotificationController;
+class MediaNotificationControllerImpl;
 class MessageCenterController;
 class MouseCursorEventFilter;
 class MruWindowTracker;
@@ -414,7 +414,7 @@ class ASH_EXPORT Shell : public SessionObserver,
     return magnification_controller_.get();
   }
   MediaController* media_controller() { return media_controller_.get(); }
-  MediaNotificationController* media_notification_controller() {
+  MediaNotificationControllerImpl* media_notification_controller() {
     return media_notification_controller_.get();
   }
   MessageCenterController* message_center_controller() {
@@ -695,7 +695,8 @@ class ASH_EXPORT Shell : public SessionObserver,
   std::unique_ptr<LogoutConfirmationController> logout_confirmation_controller_;
   std::unique_ptr<TabletModeController> tablet_mode_controller_;
   std::unique_ptr<MediaController> media_controller_;
-  std::unique_ptr<MediaNotificationController> media_notification_controller_;
+  std::unique_ptr<MediaNotificationControllerImpl>
+      media_notification_controller_;
   std::unique_ptr<MruWindowTracker> mru_window_tracker_;
   std::unique_ptr<MultiDeviceNotificationPresenter>
       multidevice_notification_presenter_;