GMC: Have the media notification container respond to size changes

This change allows the MediaNotificationContianerImplView to dynamically
respond to size changes of the audio device selector. This will be
helpful in redesign of that UI in which the size of the view will vary.

To facilitate communication between the device picker and the container
a new delegate class was created.

Because the container should support varying sizes, its observer class
was modified to eliminate the notion of expanded vs unexpanded. Instead
observers will be notified of a size change and should query the
container for its new size.

Bug: 1116694
Change-Id: Id832b77a4617645a6c48672670c22ccda88eab82
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2358890
Commit-Queue: Noah Rose Ledesma <noahrose@google.com>
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Reviewed-by: Tommy Steimel <steimel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#799247}

chrome/browser/ui/BUILD.gn

@@ -3475,6 +3475,7 @@ static_library("ui") {
       "views/global_media_controls/media_dialog_view_observer.h",
       "views/global_media_controls/media_notification_audio_device_selector_view.cc",
       "views/global_media_controls/media_notification_audio_device_selector_view.h",
+      "views/global_media_controls/media_notification_audio_device_selector_view_delegate.h",
       "views/global_media_controls/media_notification_container_impl_view.cc",
       "views/global_media_controls/media_notification_container_impl_view.h",
       "views/global_media_controls/media_notification_list_view.cc",

chrome/browser/ui/global_media_controls/media_notification_container_impl.h

@@ -11,7 +11,6 @@ class MediaNotificationContainerImpl {
  public:
   virtual void AddObserver(MediaNotificationContainerObserver* observer) = 0;
   virtual void RemoveObserver(MediaNotificationContainerObserver* observer) = 0;
-  virtual void OnAudioSinkChosen(const std::string& sink_id) = 0;
 
  protected:
   virtual ~MediaNotificationContainerImpl() = default;

chrome/browser/ui/global_media_controls/media_notification_container_observer.h

@@ -10,8 +10,8 @@
 
 class MediaNotificationContainerObserver : public base::CheckedObserver {
  public:
-  // Called when the container's expanded state changes.
-  virtual void OnContainerExpanded(bool expanded) = 0;
+  // Called when the size of the container has changed.
+  virtual void OnContainerSizeChanged() = 0;
 
   // Called when the metadata displayed in the container changes.
   virtual void OnContainerMetadataChanged() = 0;

chrome/browser/ui/global_media_controls/media_notification_service.h

@@ -71,7 +71,7 @@ class MediaNotificationService
       media_session::mojom::MediaSessionAction action) override;
 
   // MediaNotificationContainerObserver implementation.
-  void OnContainerExpanded(bool expanded) override {}
+  void OnContainerSizeChanged() override {}
   void OnContainerMetadataChanged() override {}
   void OnContainerActionsChanged() override {}
   void OnContainerClicked(const std::string& id) override;

chrome/browser/ui/views/global_media_controls/media_dialog_view.cc

@@ -122,7 +122,7 @@ gfx::Size MediaDialogView::CalculatePreferredSize() const {
   return gfx::Size(width, 1);
 }
 
-void MediaDialogView::OnContainerExpanded(bool expanded) {
+void MediaDialogView::OnContainerSizeChanged() {
   SizeToContents();
 }
 

chrome/browser/ui/views/global_media_controls/media_dialog_view.h

@@ -41,7 +41,7 @@ class MediaDialogView : public views::BubbleDialogDelegateView,
   gfx::Size CalculatePreferredSize() const override;
 
   // MediaNotificationContainerObserver implementation.
-  void OnContainerExpanded(bool expanded) override;
+  void OnContainerSizeChanged() override;
   void OnContainerMetadataChanged() override;
   void OnContainerActionsChanged() override;
   void OnContainerClicked(const std::string& id) override {}

chrome/browser/ui/views/global_media_controls/media_notification_audio_device_selector_view.cc

@@ -9,6 +9,7 @@
 #include "chrome/app/vector_icons/vector_icons.h"
 #include "chrome/browser/ui/global_media_controls/media_notification_container_impl.h"
 #include "chrome/browser/ui/global_media_controls/media_notification_service.h"
+#include "chrome/browser/ui/views/global_media_controls/media_notification_audio_device_selector_view_delegate.h"
 #include "components/vector_icons/vector_icons.h"
 #include "media/audio/audio_device_description.h"
 #include "ui/gfx/paint_vector_icon.h"
@@ -44,11 +45,11 @@ constexpr gfx::Insets kDeviceButtonInsets = gfx::Insets(5);
 
 MediaNotificationAudioDeviceSelectorView::
     MediaNotificationAudioDeviceSelectorView(
-        MediaNotificationContainerImpl* container,
+        MediaNotificationAudioDeviceSelectorViewDelegate* delegate,
         MediaNotificationService* service,
         gfx::Size size,
         const std::string& current_device_id)
-    : container_(container),
+    : delegate_(delegate),
       service_(service),
       current_device_id_(current_device_id) {
   DCHECK(service);
@@ -147,7 +148,7 @@ void MediaNotificationAudioDeviceSelectorView::ButtonPressed(
     const ui::Event& event) {
   auto it = sink_id_map_.find(sender);
   if (it != sink_id_map_.end()) {
-    container_->OnAudioSinkChosen(it->second);
+    delegate_->OnAudioSinkChosen(it->second);
   }
 }
 

chrome/browser/ui/views/global_media_controls/media_notification_audio_device_selector_view.h

@@ -15,14 +15,14 @@ namespace views {
 class Button;
 }  // namespace views
 
-class MediaNotificationContainerImpl;
+class MediaNotificationAudioDeviceSelectorViewDelegate;
 class MediaNotificationService;
 
 class MediaNotificationAudioDeviceSelectorView : public views::View,
                                                  public views::ButtonListener {
  public:
   MediaNotificationAudioDeviceSelectorView(
-      MediaNotificationContainerImpl* container,
+      MediaNotificationAudioDeviceSelectorViewDelegate* delegate,
       MediaNotificationService* service,
       gfx::Size size,
       const std::string& current_device_id);
@@ -55,9 +55,7 @@ class MediaNotificationAudioDeviceSelectorView : public views::View,
   void CreateDeviceButton(
       const media::AudioDeviceDescription& device_description);
 
-  // The parent container
-  MediaNotificationContainerImpl* const container_;
-
+  MediaNotificationAudioDeviceSelectorViewDelegate* const delegate_;
   MediaNotificationService* const service_;
 
   std::unique_ptr<MediaNotificationDeviceProvider::

chrome/browser/ui/views/global_media_controls/media_notification_audio_device_selector_view_delegate.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_UI_VIEWS_GLOBAL_MEDIA_CONTROLS_MEDIA_NOTIFICATION_AUDIO_DEVICE_SELECTOR_VIEW_DELEGATE_H_
+#define CHROME_BROWSER_UI_VIEWS_GLOBAL_MEDIA_CONTROLS_MEDIA_NOTIFICATION_AUDIO_DEVICE_SELECTOR_VIEW_DELEGATE_H_
+
+class MediaNotificationAudioDeviceSelectorViewDelegate {
+ public:
+  virtual void OnAudioSinkChosen(const std::string& sink_id) = 0;
+  virtual void OnAudioDeviceSelectorViewSizeChanged() = 0;
+};
+
+#endif  // CHROME_BROWSER_UI_VIEWS_GLOBAL_MEDIA_CONTROLS_MEDIA_NOTIFICATION_AUDIO_DEVICE_SELECTOR_VIEW_DELEGATE_H_

chrome/browser/ui/views/global_media_controls/media_notification_audio_device_selector_view_unittest.cc

@@ -9,9 +9,9 @@
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/media/router/media_router_factory.h"
 #include "chrome/browser/media/router/test/mock_media_router.h"
-#include "chrome/browser/ui/global_media_controls/media_notification_container_impl.h"
 #include "chrome/browser/ui/global_media_controls/media_notification_device_provider.h"
 #include "chrome/browser/ui/global_media_controls/media_notification_service.h"
+#include "chrome/browser/ui/views/global_media_controls/media_notification_audio_device_selector_view_delegate.h"
 #include "chrome/test/base/testing_profile.h"
 #include "chrome/test/views/chrome_views_test_base.h"
 #include "media/audio/audio_device_description.h"
@@ -44,21 +44,14 @@ class MockMediaNotificationDeviceProvider
   media::AudioDeviceDescriptions device_descriptions;
 };
 
-class MockMediaNotificationContainerImpl
-    : public MediaNotificationContainerImpl {
+class MockMediaNotificationAudioDeviceSelectorViewDelegate
+    : public MediaNotificationAudioDeviceSelectorViewDelegate {
  public:
-  MOCK_METHOD(void,
-              AddObserver,
-              (MediaNotificationContainerObserver * observer),
-              (override));
-  MOCK_METHOD(void,
-              RemoveObserver,
-              (MediaNotificationContainerObserver * observer),
-              (override));
   MOCK_METHOD(void,
               OnAudioSinkChosen,
               (const std::string& sink_id),
               (override));
+  MOCK_METHOD(void, OnAudioDeviceSelectorViewSizeChanged, (), (override));
 };
 
 }  // anonymous namespace
@@ -102,8 +95,9 @@ TEST_F(MediaNotificationAudioDeviceSelectorViewTest, DeviceButtonsCreated) {
   provider_->AddDevice("Earbuds", "3");
   service_->set_device_provider_for_testing(std::move(provider_));
 
+  MockMediaNotificationAudioDeviceSelectorViewDelegate delegate;
   view_ = std::make_unique<MediaNotificationAudioDeviceSelectorView>(
-      nullptr, service_.get(), gfx::Size(), "1");
+      &delegate, service_.get(), gfx::Size(), "1");
 
   std::vector<std::string> button_texts;
   ASSERT_TRUE(view_->device_button_container_ != nullptr);
@@ -128,13 +122,13 @@ TEST_F(MediaNotificationAudioDeviceSelectorViewTest,
   provider_->AddDevice("Earbuds", "3");
   service_->set_device_provider_for_testing(std::move(provider_));
 
-  MockMediaNotificationContainerImpl container;
-  EXPECT_CALL(container, OnAudioSinkChosen("1")).Times(1);
-  EXPECT_CALL(container, OnAudioSinkChosen("2")).Times(1);
-  EXPECT_CALL(container, OnAudioSinkChosen("3")).Times(1);
+  MockMediaNotificationAudioDeviceSelectorViewDelegate delegate;
+  EXPECT_CALL(delegate, OnAudioSinkChosen("1")).Times(1);
+  EXPECT_CALL(delegate, OnAudioSinkChosen("2")).Times(1);
+  EXPECT_CALL(delegate, OnAudioSinkChosen("3")).Times(1);
 
   view_ = std::make_unique<MediaNotificationAudioDeviceSelectorView>(
-      &container, service_.get(), gfx::Size(), "1");
+      &delegate, service_.get(), gfx::Size(), "1");
 
   for (views::View* child : view_->device_button_container_->children()) {
     view_->ButtonPressed(
@@ -152,9 +146,9 @@ TEST_F(MediaNotificationAudioDeviceSelectorViewTest, CurrentDeviceHighlighted) {
   provider_->AddDevice("Earbuds", "3");
   service_->set_device_provider_for_testing(std::move(provider_));
 
-  MockMediaNotificationContainerImpl container;
+  MockMediaNotificationAudioDeviceSelectorViewDelegate delegate;
   view_ = std::make_unique<MediaNotificationAudioDeviceSelectorView>(
-      &container, service_.get(), gfx::Size(), "3");
+      &delegate, service_.get(), gfx::Size(), "3");
 
   auto* first_button = static_cast<views::MdTextButton*>(
       view_->device_button_container_->children().front());
@@ -170,9 +164,9 @@ TEST_F(MediaNotificationAudioDeviceSelectorViewTest,
   provider_->AddDevice("Earbuds", "3");
   service_->set_device_provider_for_testing(std::move(provider_));
 
-  MockMediaNotificationContainerImpl container;
+  MockMediaNotificationAudioDeviceSelectorViewDelegate delegate;
   view_ = std::make_unique<MediaNotificationAudioDeviceSelectorView>(
-      &container, service_.get(), gfx::Size(), "1");
+      &delegate, service_.get(), gfx::Size(), "1");
 
   auto button_is_highlighted = [](views::View* view) {
     return static_cast<views::MdTextButton*>(view)->GetProminent();

chrome/browser/ui/views/global_media_controls/media_notification_container_impl_view.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/ui/views/global_media_controls/media_notification_container_impl_view.h"
 
 #include "base/feature_list.h"
+#include "chrome/browser/ui/global_media_controls/media_notification_container_impl.h"
 #include "chrome/browser/ui/global_media_controls/media_notification_container_observer.h"
 #include "chrome/browser/ui/global_media_controls/media_notification_service.h"
 #include "chrome/browser/ui/global_media_controls/media_toolbar_button_controller.h"
@@ -272,16 +273,8 @@ void MediaNotificationContainerImplView::OnDidChangeFocus(
 }
 
 void MediaNotificationContainerImplView::OnExpanded(bool expanded) {
-  gfx::Size new_size = expanded ? kExpandedSize : kNormalSize;
-
-  if (overlay_)
-    overlay_->SetSize(new_size);
-
-  SetPreferredSize(new_size);
-  PreferredSizeChanged();
-
-  for (auto& observer : observers_)
-    observer.OnContainerExpanded(expanded);
+  is_expanded_ = expanded;
+  OnSizeChanged();
 }
 
 void MediaNotificationContainerImplView::OnMediaSessionInfoChanged(
@@ -357,6 +350,11 @@ void MediaNotificationContainerImplView::OnAudioSinkChosen(
   }
 }
 
+void MediaNotificationContainerImplView::
+    OnAudioDeviceSelectorViewSizeChanged() {
+  OnSizeChanged();
+}
+
 ui::Layer* MediaNotificationContainerImplView::GetSlideOutLayer() {
   return swipeable_container_->layer();
 }
@@ -473,3 +471,27 @@ bool MediaNotificationContainerImplView::ShouldHandleMouseEvent(
   // We only drag via the left button.
   return event.IsLeftMouseButton();
 }
+
+void MediaNotificationContainerImplView::OnSizeChanged() {
+  gfx::Size new_size = is_expanded_ ? kExpandedSize : kNormalSize;
+
+  // |new_size| does not contain the height for the audio device selector view.
+  // If this view is present, we should query it for its preferred height and
+  // include that in |new_size|.
+  if (audio_device_selector_view_) {
+    auto audio_device_selector_view_size =
+        audio_device_selector_view_->GetPreferredSize();
+    DCHECK(audio_device_selector_view_size.width() == kWidth);
+    new_size.set_height(new_size.height() +
+                        audio_device_selector_view_size.height());
+  }
+
+  if (overlay_)
+    overlay_->SetSize(new_size);
+
+  SetPreferredSize(new_size);
+  PreferredSizeChanged();
+
+  for (auto& observer : observers_)
+    observer.OnContainerSizeChanged();
+}

chrome/browser/ui/views/global_media_controls/media_notification_container_impl_view.h

@@ -10,6 +10,7 @@
 #include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
 #include "chrome/browser/ui/global_media_controls/media_notification_container_impl.h"
+#include "chrome/browser/ui/views/global_media_controls/media_notification_audio_device_selector_view_delegate.h"
 #include "chrome/browser/ui/views/global_media_controls/overlay_media_notification_view.h"
 #include "components/media_message_center/media_notification_container.h"
 #include "components/media_message_center/media_notification_view_impl.h"
@@ -39,6 +40,7 @@ class MediaNotificationContainerImplView
     : public views::Button,
       public media_message_center::MediaNotificationContainer,
       public MediaNotificationContainerImpl,
+      public MediaNotificationAudioDeviceSelectorViewDelegate,
       public views::SlideOutControllerDelegate,
       public views::ButtonListener,
       public views::FocusChangeListener {
@@ -89,8 +91,11 @@ class MediaNotificationContainerImplView
   // MediaNotificationContainerImpl:
   void AddObserver(MediaNotificationContainerObserver* observer) override;
   void RemoveObserver(MediaNotificationContainerObserver* observer) override;
+
+  // MediaNotificationAudioDeviceSelectorViewDelegate
   // Called when an audio device has been selected for output.
   void OnAudioSinkChosen(const std::string& sink_id) override;
+  void OnAudioDeviceSelectorViewSizeChanged() override;
 
   // Sets up the notification to be ready to display in an overlay instead of
   // the dialog.
@@ -108,6 +113,7 @@ class MediaNotificationContainerImplView
   }
 
   bool is_playing_for_testing() { return is_playing_; }
+  bool is_expanded_for_testing() { return is_expanded_; }
 
   views::Widget* drag_image_widget_for_testing() {
     return drag_image_widget_.get();
@@ -135,6 +141,8 @@ class MediaNotificationContainerImplView
   // True if we should handle the given mouse event for dragging purposes.
   bool ShouldHandleMouseEvent(const ui::MouseEvent& event, bool is_press);
 
+  void OnSizeChanged();
+
   const std::string id_;
   views::View* swipeable_container_ = nullptr;
 
@@ -179,6 +187,8 @@ class MediaNotificationContainerImplView
 
   bool is_playing_ = false;
 
+  bool is_expanded_ = false;
+
   std::string audio_sink_id_ = media::AudioDeviceDescription::kDefaultDeviceId;
 
   base::ObserverList<MediaNotificationContainerObserver> observers_;

chrome/browser/ui/views/global_media_controls/media_notification_container_impl_view_unittest.cc

@@ -36,7 +36,7 @@ class MockMediaNotificationContainerObserver
   ~MockMediaNotificationContainerObserver() = default;
 
   // MediaNotificationContainerObserver implementation.
-  MOCK_METHOD1(OnContainerExpanded, void(bool expanded));
+  MOCK_METHOD0(OnContainerSizeChanged, void());
   MOCK_METHOD0(OnContainerMetadataChanged, void());
   MOCK_METHOD0(OnContainerActionsChanged, void());
   MOCK_METHOD1(OnContainerClicked, void(const std::string& id));
@@ -350,30 +350,24 @@ TEST_F(MediaNotificationContainerImplViewTest, KeyboardToDismiss) {
 }
 
 TEST_F(MediaNotificationContainerImplViewTest, ForceExpandedState) {
-  bool notification_expanded = false;
-  EXPECT_CALL(observer(), OnContainerExpanded(_))
-      .WillRepeatedly([&notification_expanded](bool expanded) {
-        notification_expanded = expanded;
-      });
-
   // When we have many actions enabled, we should be forced into the expanded
   // state.
   SimulateAllActionsEnabled();
-  EXPECT_TRUE(notification_expanded);
+  EXPECT_TRUE(notification_container()->is_expanded_for_testing());
 
   // When we don't have many actions enabled, we should be forced out of the
   // expanded state.
   SimulateOnlyPlayPauseEnabled();
-  EXPECT_FALSE(notification_expanded);
+  EXPECT_FALSE(notification_container()->is_expanded_for_testing());
 
   // We will also be forced into the expanded state when artwork is present.
   SimulateHasArtwork();
-  EXPECT_TRUE(notification_expanded);
+  EXPECT_TRUE(notification_container()->is_expanded_for_testing());
 
   // Once the artwork is gone, we should be forced back out of the expanded
   // state.
   SimulateHasNoArtwork();
-  EXPECT_FALSE(notification_expanded);
+  EXPECT_FALSE(notification_container()->is_expanded_for_testing());
 }
 
 TEST_F(MediaNotificationContainerImplViewTest, SendsMetadataUpdates) {