Change ButtonPressed overrides to callbacks: .../global_media_controls/

Bug: 772945
Change-Id: I580a79f0141bad0dfacbdc6d8c999605fceec029
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2458809
Commit-Queue: Tommy Steimel <steimel@chromium.org>
Reviewed-by: Tommy Steimel <steimel@chromium.org>
Auto-Submit: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#815337}

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

@@ -8,6 +8,7 @@
 #include "base/metrics/field_trial_params.h"
 #include "chrome/browser/media/router/media_router_feature.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/global_media_controls/cast_media_notification_item.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"
@@ -63,8 +64,8 @@ constexpr int kMinMovementSquaredToBeDragging = 10;
 class MediaNotificationContainerImplView::DismissButton
     : public views::ImageButton {
  public:
-  explicit DismissButton(views::ButtonListener* listener)
-      : views::ImageButton(listener) {
+  explicit DismissButton(PressedCallback callback)
+      : views::ImageButton(std::move(callback)) {
     views::ConfigureVectorImageButton(this);
     views::InstallFixedSizeCircleHighlightPathGenerator(
         this, kDismissButtonBackgroundRadius);
@@ -81,7 +82,14 @@ MediaNotificationContainerImplView::MediaNotificationContainerImplView(
     base::WeakPtr<media_message_center::MediaNotificationItem> item,
     MediaNotificationService* service,
     base::Optional<media_message_center::NotificationTheme> theme)
-    : views::Button(this),
+    : views::Button(base::BindRepeating(
+          [](MediaNotificationContainerImplView* view) {
+            // If |is_dragging_| is set, this click should be treated as a drag
+            // and not fire ContainerClicked().
+            if (!view->is_dragging_)
+              view->ContainerClicked();
+          },
+          base::Unretained(this))),
       id_(id),
       foreground_color_(kDefaultForegroundColor),
       background_color_(kDefaultBackgroundColor),
@@ -114,7 +122,9 @@ MediaNotificationContainerImplView::MediaNotificationContainerImplView(
   dismiss_button_container_ = dismiss_button_placeholder_->AddChildView(
       std::move(dismiss_button_container));
 
-  auto dismiss_button = std::make_unique<DismissButton>(this);
+  auto dismiss_button = std::make_unique<DismissButton>(base::BindRepeating(
+      &MediaNotificationContainerImplView::DismissNotification,
+      base::Unretained(this)));
   dismiss_button->SetPreferredSize(kDismissButtonSize);
   dismiss_button->SetFocusBehavior(views::View::FocusBehavior::ALWAYS);
   dismiss_button->SetTooltipText(l10n_util::GetStringUTF16(
@@ -123,16 +133,16 @@ MediaNotificationContainerImplView::MediaNotificationContainerImplView(
       dismiss_button_container_->AddChildView(std::move(dismiss_button));
   UpdateDismissButtonIcon();
 
-  bool is_local_media_session =
-      item ? item->SourceType() ==
-                 media_message_center::SourceType::kLocalMediaSession
-           : false;
-  bool is_cast_notification =
-      item ? item->SourceType() == media_message_center::SourceType::kCast
-           : false;
-  if (is_cast_notification) {
-    cast_item_ = static_cast<CastMediaNotificationItem*>(item.get());
-  }
+  // Compute a few things related to |item| before the construction of |view|
+  // below moves it.
+  const bool is_cast_notification =
+      item && item->SourceType() == media_message_center::SourceType::kCast;
+  auto* const cast_item =
+      is_cast_notification ? static_cast<CastMediaNotificationItem*>(item.get())
+                           : nullptr;
+  const bool is_local_media_session =
+      item && item->SourceType() ==
+                  media_message_center::SourceType::kLocalMediaSession;
 
   std::unique_ptr<media_message_center::MediaNotificationView> view;
   if (base::FeatureList::IsEnabled(media::kGlobalMediaControlsModernUI)) {
@@ -166,7 +176,14 @@ MediaNotificationContainerImplView::MediaNotificationContainerImplView(
 
     stop_cast_button_ =
         stop_button_strip_->AddChildView(std::make_unique<views::LabelButton>(
-            this, l10n_util::GetStringUTF16(
+            base::BindRepeating(
+                [](CastMediaNotificationItem* cast_item) {
+                  media_router::MediaRouterFactory::GetApiForBrowserContext(
+                      cast_item->profile())
+                      ->TerminateRoute(cast_item->route_id());
+                },
+                base::Unretained(cast_item)),
+            l10n_util::GetStringUTF16(
                 IDS_GLOBAL_MEDIA_CONTROLS_STOP_CASTING_BUTTON_LABEL)));
     stop_cast_button_->SetInkDropMode(InkDropMode::ON);
     stop_cast_button_->SetHasInkDropActionOnClick(true);
@@ -454,25 +471,6 @@ void MediaNotificationContainerImplView::OnSlideOut() {
   DismissNotification();
 }
 
-void MediaNotificationContainerImplView::ButtonPressed(views::Button* sender,
-                                                       const ui::Event& event) {
-  if (sender == dismiss_button_) {
-    DismissNotification();
-  } else if (sender == stop_cast_button_) {
-    media_router::MediaRouter* router =
-        media_router::MediaRouterFactory::GetApiForBrowserContext(
-            cast_item_->profile());
-    router->TerminateRoute(cast_item_->route_id());
-  } else if (sender == this) {
-    // If |is_dragging_| is set, this click should be treated as a drag and not
-    // fire the |OnContainerClicked()| event.
-    if (!is_dragging_)
-      ContainerClicked();
-  } else {
-    NOTREACHED();
-  }
-}
-
 void MediaNotificationContainerImplView::AddObserver(
     MediaNotificationContainerObserver* observer) {
   observers_.AddObserver(observer);

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

@@ -9,7 +9,6 @@
 
 #include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
-#include "chrome/browser/ui/global_media_controls/cast_media_notification_item.h"
 #include "chrome/browser/ui/global_media_controls/media_notification_container_impl.h"
 #include "chrome/browser/ui/views/global_media_controls/media_notification_device_selector_view_delegate.h"
 #include "chrome/browser/ui/views/global_media_controls/overlay_media_notification_view.h"
@@ -18,7 +17,6 @@
 #include "media/audio/audio_device_description.h"
 #include "media/base/media_switches.h"
 #include "ui/views/animation/slide_out_controller_delegate.h"
-#include "ui/views/controls/button/button.h"
 #include "ui/views/focus/focus_manager.h"
 #include "ui/views/widget/unique_widget_ptr.h"
 
@@ -45,7 +43,6 @@ class MediaNotificationContainerImplView
       public MediaNotificationContainerImpl,
       public MediaNotificationDeviceSelectorViewDelegate,
       public views::SlideOutControllerDelegate,
-      public views::ButtonListener,
       public views::FocusChangeListener {
  public:
   MediaNotificationContainerImplView(
@@ -90,9 +87,6 @@ class MediaNotificationContainerImplView
   void OnSlideChanged(bool in_progress) override {}
   void OnSlideOut() override;
 
-  // views::ButtonListener:
-  void ButtonPressed(views::Button* sender, const ui::Event& event) override;
-
   // MediaNotificationContainerImpl:
   void AddObserver(MediaNotificationContainerObserver* observer) override;
   void RemoveObserver(MediaNotificationContainerObserver* observer) override;
@@ -220,8 +214,6 @@ class MediaNotificationContainerImplView
 
   MediaNotificationService* const service_;
 
-  CastMediaNotificationItem* cast_item_ = nullptr;
-
   DISALLOW_COPY_AND_ASSIGN(MediaNotificationContainerImplView);
 };
 

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

@@ -9,6 +9,8 @@
 #include "base/test/scoped_feature_list.h"
 #include "build/build_config.h"
 #include "chrome/browser/media/router/media_router_feature.h"
+#include "chrome/browser/ui/global_media_controls/cast_media_notification_item.h"
+#include "chrome/browser/ui/global_media_controls/cast_media_session_controller.h"
 #include "chrome/browser/ui/global_media_controls/media_notification_container_observer.h"
 #include "chrome/test/base/testing_profile.h"
 #include "chrome/test/views/chrome_views_test_base.h"

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

@@ -62,18 +62,15 @@ DeviceEntryUI::DeviceEntryUI(const std::string& raw_device_id,
                              const std::string& subtext)
     : raw_device_id_(raw_device_id), device_name_(device_name), icon_(icon) {}
 
-AudioDeviceEntryView::AudioDeviceEntryView(
-    views::ButtonListener* button_listener,
+AudioDeviceEntryView::AudioDeviceEntryView(PressedCallback callback,
                                            SkColor foreground_color,
                                            SkColor background_color,
                                            const std::string& raw_device_id,
-    const std::string& device_name,
-    const std::string& subtext)
+                                           const std::string& device_name)
     : DeviceEntryUI(raw_device_id, device_name, &vector_icons::kHeadsetIcon),
-      HoverButton(button_listener,
+      HoverButton(std::move(callback),
                   GetAudioDeviceIcon(),
-                  base::UTF8ToUTF16(device_name),
-                  base::UTF8ToUTF16(subtext)) {
+                  base::UTF8ToUTF16(device_name)) {
   ChangeEntryColor(static_cast<views::ImageView*>(icon_view()), title(),
                    subtitle(), icon_, foreground_color, background_color);
 
@@ -117,14 +114,17 @@ DeviceEntryUIType AudioDeviceEntryView::GetType() const {
   return DeviceEntryUIType::kAudio;
 }
 
-CastDeviceEntryView::CastDeviceEntryView(views::ButtonListener* button_listener,
+CastDeviceEntryView::CastDeviceEntryView(
+    base::RepeatingCallback<void(CastDeviceEntryView*)> callback,
     SkColor foreground_color,
     SkColor background_color,
     const media_router::UIMediaSink& sink)
     : DeviceEntryUI(sink.id,
                     base::UTF16ToUTF8(sink.friendly_name),
                     CastDialogSinkButton::GetVectorIcon(sink.icon_type)),
-      CastDialogSinkButton(PressedCallback(button_listener, this), sink) {
+      CastDialogSinkButton(
+          base::BindRepeating(std::move(callback), base::Unretained(this)),
+          sink) {
   switch (sink.state) {
     // If the sink state is CONNECTING or DISCONNECTING, a throbber icon will
     // show up. The icon's color remains unchanged.

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

@@ -42,12 +42,11 @@ class DeviceEntryUI {
 
 class AudioDeviceEntryView : public DeviceEntryUI, public HoverButton {
  public:
-  AudioDeviceEntryView(views::ButtonListener* button_listener,
+  AudioDeviceEntryView(PressedCallback callback,
                        SkColor foreground_color,
                        SkColor background_color,
                        const std::string& raw_device_id,
-                       const std::string& name,
-                       const std::string& subtext = "");
+                       const std::string& name);
   ~AudioDeviceEntryView() override = default;
 
   // DeviceEntryUI
@@ -64,7 +63,8 @@ class AudioDeviceEntryView : public DeviceEntryUI, public HoverButton {
 class CastDeviceEntryView : public DeviceEntryUI,
                             public media_router::CastDialogSinkButton {
  public:
-  CastDeviceEntryView(views::ButtonListener* button_listener,
+  CastDeviceEntryView(
+      base::RepeatingCallback<void(CastDeviceEntryView*)> callback,
       SkColor foreground_color,
       SkColor background_color,
       const media_router::UIMediaSink& sink);

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

@@ -102,8 +102,9 @@ MediaNotificationDeviceSelectorView::MediaNotificationDeviceSelectorView(
 
   expand_button_ = expand_button_strip_->AddChildView(
       std::make_unique<ExpandDeviceSelectorButton>(this));
-  expand_button_->set_callback(
-      views::Button::PressedCallback(this, expand_button_));
+  expand_button_->set_callback(base::BindRepeating(
+      &MediaNotificationDeviceSelectorView::ExpandButtonPressed,
+      base::Unretained(this)));
 
   device_entry_views_container_ = AddChildView(std::make_unique<views::View>());
   device_entry_views_container_->SetLayoutManager(
@@ -185,8 +186,11 @@ void MediaNotificationDeviceSelectorView::UpdateAvailableAudioDevices(
   bool current_device_still_exists = false;
   for (auto description : device_descriptions) {
     auto device_entry_view = std::make_unique<AudioDeviceEntryView>(
-        this, foreground_color_, background_color_, description.unique_id,
-        description.device_name, "");
+        base::BindRepeating(
+            &MediaNotificationDeviceSelectorViewDelegate::OnAudioSinkChosen,
+            base::Unretained(delegate_), description.unique_id),
+        foreground_color_, background_color_, description.unique_id,
+        description.device_name);
     device_entry_view->set_tag(next_tag_++);
     device_entry_ui_map_[device_entry_view->tag()] = device_entry_view.get();
     device_entry_views_container_->AddChildView(std::move(device_entry_view));
@@ -220,32 +224,6 @@ void MediaNotificationDeviceSelectorView::OnColorsChanged(
   SchedulePaint();
 }
 
-void MediaNotificationDeviceSelectorView::ButtonPressed(
-    views::Button* sender,
-    const ui::Event& event) {
-  if (sender == expand_button_) {
-    if (is_expanded_)
-      HideDevices();
-    else
-      ShowDevices();
-
-    delegate_->OnDeviceSelectorViewSizeChanged();
-  } else {
-    auto* device_entry = GetDeviceEntryUI(sender);
-    switch (device_entry->GetType()) {
-      case DeviceEntryUIType::kAudio:
-        delegate_->OnAudioSinkChosen(device_entry->raw_device_id());
-        break;
-      case DeviceEntryUIType::kCast:
-        StartCastSession(static_cast<CastDeviceEntryView*>(device_entry));
-        break;
-      default:
-        NOTREACHED();
-        break;
-    }
-  }
-}
-
 SkColor MediaNotificationDeviceSelectorView::
     GetIconLabelBubbleSurroundingForegroundColor() const {
   return foreground_color_;
@@ -330,6 +308,14 @@ bool MediaNotificationDeviceSelectorView::ShouldBeVisible() const {
   return device_entry_views_container_->children().size() > 2;
 }
 
+void MediaNotificationDeviceSelectorView::ExpandButtonPressed() {
+  if (is_expanded_)
+    HideDevices();
+  else
+    ShowDevices();
+  delegate_->OnDeviceSelectorViewSizeChanged();
+}
+
 void MediaNotificationDeviceSelectorView::UpdateIsAudioDeviceSwitchingEnabled(
     bool enabled) {
   if (enabled == is_audio_device_switching_enabled_)
@@ -367,7 +353,10 @@ void MediaNotificationDeviceSelectorView::OnModelUpdated(
   has_cast_device_ = !model.media_sinks().empty();
   for (auto sink : model.media_sinks()) {
     auto device_entry_view = std::make_unique<CastDeviceEntryView>(
-        this, foreground_color_, background_color_, sink);
+        base::BindRepeating(
+            &MediaNotificationDeviceSelectorView::StartCastSession,
+            base::Unretained(this)),
+        foreground_color_, background_color_, sink);
     device_entry_view->set_tag(next_tag_++);
     device_entry_ui_map_[device_entry_view->tag()] = device_entry_view.get();
     device_entry_views_container_->AddChildView(std::move(device_entry_view));

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

@@ -26,7 +26,6 @@ class MediaNotificationDeviceSelectorViewDelegate;
 
 class MediaNotificationDeviceSelectorView
     : public views::View,
-      public views::ButtonListener,
       public IconLabelBubbleView::Delegate,
       public media_router::CastDialogController::Observer {
  public:
@@ -49,9 +48,6 @@ class MediaNotificationDeviceSelectorView
   // Called when the audio device switching has become enabled or disabled.
   void UpdateIsAudioDeviceSwitchingEnabled(bool enabled);
 
-  // views::ButtonListener
-  void ButtonPressed(views::Button* sender, const ui::Event& event) override;
-
   // IconLabelBubbleView::Delegate
   SkColor GetIconLabelBubbleSurroundingForegroundColor() const override;
   SkColor GetIconLabelBubbleBackgroundColor() const override;
@@ -88,6 +84,7 @@ class MediaNotificationDeviceSelectorView
 
   void UpdateVisibility();
   bool ShouldBeVisible() const;
+  void ExpandButtonPressed();
   void ShowDevices();
   void HideDevices();
   void RemoveDevicesOfType(DeviceEntryUIType type);

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

@@ -18,6 +18,7 @@
 #include "testing/gmock/include/gmock/gmock.h"
 #include "ui/events/base_event_utils.h"
 #include "ui/gfx/color_palette.h"
+#include "ui/views/test/button_test_api.h"
 
 using media_router::CastDialogController;
 using media_router::CastDialogModel;
@@ -164,10 +165,9 @@ class MediaNotificationDeviceSelectorViewTest : public ChromeViewsTestBase {
   }
 
   void SimulateButtonClick(views::View* view) {
-    view_->ButtonPressed(
-        static_cast<views::Button*>(view),
-        ui::MouseEvent(ui::ET_MOUSE_PRESSED, gfx::Point(), gfx::Point(),
-                       ui::EventTimeForNow(), 0, 0));
+    views::test::ButtonTestApi(static_cast<views::Button*>(view))
+        .NotifyClick(ui::MouseEvent(ui::ET_MOUSE_PRESSED, gfx::Point(),
+                                    gfx::Point(), ui::EventTimeForNow(), 0, 0));
   }
 
   std::string EntryLabelText(views::View* entry_view) {

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

@@ -29,7 +29,8 @@
 #include "ui/views/controls/button/button_controller.h"
 
 MediaToolbarButtonView::MediaToolbarButtonView(BrowserView* browser_view)
-    : ToolbarButton(PressedCallback(this, this)),
+    : ToolbarButton(base::BindRepeating(&MediaToolbarButtonView::ButtonPressed,
+                                        base::Unretained(this))),
       browser_(browser_view->browser()),
       service_(MediaNotificationServiceFactory::GetForProfile(
           browser_view->browser()->profile())),
@@ -65,21 +66,6 @@ void MediaToolbarButtonView::RemoveObserver(
   observers_.RemoveObserver(observer);
 }
 
-void MediaToolbarButtonView::ButtonPressed(views::Button* sender,
-                                           const ui::Event& event) {
-  if (MediaDialogView::IsShowing()) {
-    MediaDialogView::HideDialog();
-  } else {
-    MediaDialogView::ShowDialog(this, service_);
-
-    feature_promo_controller_->CloseBubble(
-        feature_engagement::kIPHLiveCaptionFeature);
-
-    for (auto& observer : observers_)
-      observer.OnMediaDialogOpened();
-  }
-}
-
 void MediaToolbarButtonView::Show() {
   SetVisible(true);
   PreferredSizeChanged();
@@ -124,3 +110,17 @@ void MediaToolbarButtonView::UpdateIcon() {
       touch_ui ? kMediaToolbarButtonTouchIcon : kMediaToolbarButtonIcon;
   UpdateIconsWithStandardColors(icon);
 }
+
+void MediaToolbarButtonView::ButtonPressed() {
+  if (MediaDialogView::IsShowing()) {
+    MediaDialogView::HideDialog();
+  } else {
+    MediaDialogView::ShowDialog(this, service_);
+
+    feature_promo_controller_->CloseBubble(
+        feature_engagement::kIPHLiveCaptionFeature);
+
+    for (auto& observer : observers_)
+      observer.OnMediaDialogOpened();
+  }
+}

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

@@ -20,8 +20,7 @@ class MediaToolbarButtonObserver;
 // of its parent ToolbarView. The icon is made visible when there is an active
 // media session.
 class MediaToolbarButtonView : public ToolbarButton,
-                               public MediaToolbarButtonControllerDelegate,
-                               public views::ButtonListener {
+                               public MediaToolbarButtonControllerDelegate {
  public:
   explicit MediaToolbarButtonView(BrowserView* browser_view);
   ~MediaToolbarButtonView() override;
@@ -35,13 +34,12 @@ class MediaToolbarButtonView : public ToolbarButton,
   void Enable() override;
   void Disable() override;
 
-  // views::ButtonListener implementation.
-  void ButtonPressed(views::Button* sender, const ui::Event& event) override;
-
   // ToolbarButton implementation.
   void UpdateIcon() override;
 
  private:
+  void ButtonPressed();
+
   const Browser* const browser_;
 
   MediaNotificationService* const service_;