Change ButtonPressed overrides to callbacks: ui/views/controls/

Bug: 772945
Change-Id: I58e8a596aeeca4a78b981600170c134b52c3a214
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2488909
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Auto-Submit: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#819446}

ui/views/controls/combobox/combobox.cc

@@ -68,7 +68,8 @@ gfx::ImageSkia GetImageSkiaFromImageModel(const ui::ImageModel* model,
 // The transparent button which holds a button state but is not rendered.
 class TransparentButton : public Button {
  public:
-  explicit TransparentButton(ButtonListener* listener) : Button(listener) {
+  explicit TransparentButton(PressedCallback callback)
+      : Button(std::move(callback)) {
     SetFocusBehavior(FocusBehavior::NEVER);
     button_controller()->set_notify_action(
         ButtonController::NotifyAction::kOnPress);
@@ -243,7 +244,9 @@ Combobox::Combobox(std::unique_ptr<ui::ComboboxModel> model,
 Combobox::Combobox(ui::ComboboxModel* model, int text_context, int text_style)
     : text_context_(text_context),
       text_style_(text_style),
-      arrow_button_(new TransparentButton(this)) {
+      arrow_button_(new TransparentButton(
+          base::BindRepeating(&Combobox::ArrowButtonPressed,
+                              base::Unretained(this)))) {
   SetModel(model);
 #if defined(OS_APPLE)
   SetFocusBehavior(FocusBehavior::ACCESSIBLE_ONLY);
@@ -551,16 +554,6 @@ bool Combobox::HandleAccessibleAction(const ui::AXActionData& action_data) {
   return View::HandleAccessibleAction(action_data);
 }
 
-void Combobox::ButtonPressed(Button* sender, const ui::Event& event) {
-  if (!GetEnabled())
-    return;
-
-  // TODO(hajimehoshi): Fix the problem that the arrow button blinks when
-  // cliking this while the dropdown menu is opened.
-  if ((base::TimeTicks::Now() - closed_time_) > kMinimumTimeBetweenButtonClicks)
-    ShowDropDownMenu(ui::GetMenuSourceTypeForEvent(event));
-}
-
 void Combobox::OnComboboxModelChanged(ui::ComboboxModel* model) {
   DCHECK_EQ(model_, model);
 
@@ -648,6 +641,16 @@ void Combobox::PaintIconAndText(gfx::Canvas* canvas) {
   PaintComboboxArrow(text_color, arrow_bounds, canvas);
 }
 
+void Combobox::ArrowButtonPressed(const ui::Event& event) {
+  if (!GetEnabled())
+    return;
+
+  // TODO(hajimehoshi): Fix the problem that the arrow button blinks when
+  // cliking this while the dropdown menu is opened.
+  if ((base::TimeTicks::Now() - closed_time_) > kMinimumTimeBetweenButtonClicks)
+    ShowDropDownMenu(ui::GetMenuSourceTypeForEvent(event));
+}
+
 void Combobox::ShowDropDownMenu(ui::MenuSourceType source_type) {
   constexpr int kMenuBorderWidthTop = 1;
   // Menu's requested position's width should be the same as local bounds so the

ui/views/controls/combobox/combobox.h

@@ -40,7 +40,6 @@ class PrefixSelector;
 // Combobox has two distinct parts, the drop down arrow and the text.
 class VIEWS_EXPORT Combobox : public View,
                               public PrefixDelegate,
-                              public ButtonListener,
                               public ui::ComboboxModelObserver {
  public:
   METADATA_HEADER(Combobox);
@@ -119,9 +118,6 @@ class VIEWS_EXPORT Combobox : public View,
   void SetSelectedRow(int row) override;
   base::string16 GetTextForRow(int row) override;
 
-  // Overridden from ButtonListener:
-  void ButtonPressed(Button* sender, const ui::Event& event) override;
-
  protected:
   // Overridden from ComboboxModelObserver:
   void OnComboboxModelChanged(ui::ComboboxModel* model) override;
@@ -144,6 +140,9 @@ class VIEWS_EXPORT Combobox : public View,
   // Draws the selected value of the drop down list
   void PaintIconAndText(gfx::Canvas* canvas);
 
+  // Opens the dropdown menu in response to |event|.
+  void ArrowButtonPressed(const ui::Event& event);
+
   // Show the drop down list
   void ShowDropDownMenu(ui::MenuSourceType source_type);
 

ui/views/controls/editable_combobox/editable_combobox.cc

@@ -64,7 +64,7 @@ namespace {
 
 class Arrow : public Button {
  public:
-  explicit Arrow(ButtonListener* listener) : Button(listener) {
+  explicit Arrow(PressedCallback callback) : Button(std::move(callback)) {
     // Similar to Combobox's TransparentButton.
     SetFocusBehavior(FocusBehavior::NEVER);
     button_controller()->set_notify_action(
@@ -333,7 +333,8 @@ EditableCombobox::EditableCombobox(
     textfield_->SetExtraInsets(gfx::Insets(
         /*top=*/0, /*left=*/0, /*bottom=*/0,
         /*right=*/kComboboxArrowContainerWidth - kComboboxArrowPaddingWidth));
-    arrow_ = AddChildView(std::make_unique<Arrow>(this));
+    arrow_ = AddChildView(std::make_unique<Arrow>(base::BindRepeating(
+        &EditableCombobox::ArrowButtonPressed, base::Unretained(this))));
   }
   SetLayoutManager(std::make_unique<views::FillLayout>());
 }
@@ -446,14 +447,6 @@ void EditableCombobox::OnViewBlurred(View* observed_view) {
   CloseMenu();
 }
 
-void EditableCombobox::ButtonPressed(Button* sender, const ui::Event& event) {
-  textfield_->RequestFocus();
-  if (menu_runner_ && menu_runner_->IsRunning())
-    CloseMenu();
-  else
-    ShowDropDownMenu(ui::GetMenuSourceTypeForEvent(event));
-}
-
 void EditableCombobox::OnLayoutIsAnimatingChanged(
     views::AnimatingLayoutManager* source,
     bool is_animating) {
@@ -496,6 +489,14 @@ void EditableCombobox::HandleNewContent(const base::string16& new_content) {
   menu_model_->UpdateItemsShown();
 }
 
+void EditableCombobox::ArrowButtonPressed(const ui::Event& event) {
+  textfield_->RequestFocus();
+  if (menu_runner_ && menu_runner_->IsRunning())
+    CloseMenu();
+  else
+    ShowDropDownMenu(ui::GetMenuSourceTypeForEvent(event));
+}
+
 void EditableCombobox::ShowDropDownMenu(ui::MenuSourceType source_type) {
   constexpr int kMenuBorderWidthTop = 1;
 

ui/views/controls/editable_combobox/editable_combobox.h

@@ -14,7 +14,6 @@
 #include "base/strings/string16.h"
 #include "build/build_config.h"
 #include "ui/base/ui_base_types.h"
-#include "ui/views/controls/button/button.h"
 #include "ui/views/controls/textfield/textfield_controller.h"
 #include "ui/views/layout/animating_layout_manager.h"
 #include "ui/views/style/typography.h"
@@ -33,6 +32,7 @@ class Event;
 }  // namespace ui
 
 namespace views {
+class Button;
 class EditableComboboxMenuModel;
 class EditableComboboxPreTargetHandler;
 class MenuRunner;
@@ -43,7 +43,6 @@ class VIEWS_EXPORT EditableCombobox
     : public View,
       public TextfieldController,
       public ViewObserver,
-      public ButtonListener,
       public views::AnimatingLayoutManager::Observer {
  public:
   METADATA_HEADER(EditableCombobox);
@@ -123,6 +122,9 @@ class VIEWS_EXPORT EditableCombobox
   // Notifies listener of new content and updates the menu items to show.
   void HandleNewContent(const base::string16& new_content);
 
+  // Toggles the dropdown menu in response to |event|.
+  void ArrowButtonPressed(const ui::Event& event);
+
   // Shows the drop-down menu.
   void ShowDropDownMenu(ui::MenuSourceType source_type = ui::MENU_SOURCE_NONE);
 
@@ -142,9 +144,6 @@ class VIEWS_EXPORT EditableCombobox
   // Overridden from ViewObserver:
   void OnViewBlurred(View* observed_view) override;
 
-  // Overridden from ButtonListener:
-  void ButtonPressed(Button* sender, const ui::Event& event) override;
-
   // Overridden from views::AnimatingLayoutManager::Observer:
   void OnLayoutIsAnimatingChanged(views::AnimatingLayoutManager* source,
                                   bool is_animating) override;

ui/views/controls/scrollbar/base_scroll_bar_button.cc

@@ -4,6 +4,8 @@
 
 #include "ui/views/controls/scrollbar/base_scroll_bar_button.h"
 
+#include <utility>
+
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "ui/display/screen.h"
@@ -12,9 +14,9 @@
 
 namespace views {
 
-BaseScrollBarButton::BaseScrollBarButton(ButtonListener* listener,
+BaseScrollBarButton::BaseScrollBarButton(PressedCallback callback,
                                          const base::TickClock* tick_clock)
-    : Button(listener),
+    : Button(std::move(callback)),
       repeater_(base::BindRepeating(&BaseScrollBarButton::RepeaterNotifyClick,
                                     base::Unretained(this)),
                 tick_clock) {

ui/views/controls/scrollbar/base_scroll_bar_button.h

@@ -30,7 +30,7 @@ class VIEWS_EXPORT BaseScrollBarButton : public Button {
  public:
   METADATA_HEADER(BaseScrollBarButton);
 
-  explicit BaseScrollBarButton(ButtonListener* listener,
+  explicit BaseScrollBarButton(PressedCallback callback,
                                const base::TickClock* tick_clock = nullptr);
   ~BaseScrollBarButton() override;
 

ui/views/controls/scrollbar/base_scroll_bar_button_unittest.cc

@@ -24,31 +24,28 @@ using testing::_;
 using testing::AtLeast;
 using testing::AtMost;
 
-class MockButtonListener : public ButtonListener {
+class MockButtonCallback {
  public:
-  MockButtonListener() = default;
-  MockButtonListener(const MockButtonListener&) = delete;
-  MockButtonListener& operator=(const MockButtonListener&) = delete;
-  ~MockButtonListener() override = default;
-
-  // ButtonListener:
-  MOCK_METHOD(void,
-              ButtonPressed,
-              (Button * sender, const ui::Event& event),
-              (override));
+  MockButtonCallback() = default;
+  MockButtonCallback(const MockButtonCallback&) = delete;
+  MockButtonCallback& operator=(const MockButtonCallback&) = delete;
+  ~MockButtonCallback() = default;
+
+  MOCK_METHOD(void, ButtonPressed, ());
 };
 
 class BaseScrollBarButtonTest : public testing::Test {
  public:
   BaseScrollBarButtonTest()
       : button_(std::make_unique<BaseScrollBarButton>(
-            &listener_,
+            base::BindRepeating(&MockButtonCallback::ButtonPressed,
+                                base::Unretained(&callback_)),
             task_environment_.GetMockTickClock())) {}
 
   ~BaseScrollBarButtonTest() override = default;
 
  protected:
-  testing::StrictMock<MockButtonListener>& listener() { return listener_; }
+  testing::StrictMock<MockButtonCallback>& callback() { return callback_; }
   Button* button() { return button_.get(); }
 
   void AdvanceTime(base::TimeDelta time_delta) {
@@ -61,7 +58,7 @@ class BaseScrollBarButtonTest : public testing::Test {
   display::test::TestScreen test_screen_;
   display::test::ScopedScreenOverride screen_override{&test_screen_};
 
-  testing::StrictMock<MockButtonListener> listener_;
+  testing::StrictMock<MockButtonCallback> callback_;
   const std::unique_ptr<Button> button_;
 };
 
@@ -76,18 +73,18 @@ TEST_F(BaseScrollBarButtonTest, FocusBehavior) {
 }
 
 TEST_F(BaseScrollBarButtonTest, CallbackFiresOnMouseDown) {
-  EXPECT_CALL(listener(), ButtonPressed(_, _));
+  EXPECT_CALL(callback(), ButtonPressed());
 
-  // By default the button should notify its listener on mouse release.
+  // By default the button should notify its callback on mouse release.
   button()->OnMousePressed(ui::MouseEvent(
       ui::ET_MOUSE_PRESSED, gfx::Point(), gfx::Point(), ui::EventTimeForNow(),
       ui::EF_LEFT_MOUSE_BUTTON, ui::EF_LEFT_MOUSE_BUTTON));
 }
 
-TEST_F(BaseScrollBarButtonTest, CallbackFilesMultipleTimesMouseHeldDown) {
-  EXPECT_CALL(listener(), ButtonPressed(_, _)).Times(AtLeast(2));
+TEST_F(BaseScrollBarButtonTest, CallbackFiresMultipleTimesMouseHeldDown) {
+  EXPECT_CALL(callback(), ButtonPressed()).Times(AtLeast(2));
 
-  // By default the button should notify its listener on mouse release.
+  // By default the button should notify its callback on mouse release.
   button()->OnMousePressed(ui::MouseEvent(
       ui::ET_MOUSE_PRESSED, gfx::Point(), gfx::Point(), ui::EventTimeForNow(),
       ui::EF_LEFT_MOUSE_BUTTON, ui::EF_LEFT_MOUSE_BUTTON));
@@ -96,16 +93,16 @@ TEST_F(BaseScrollBarButtonTest, CallbackFilesMultipleTimesMouseHeldDown) {
 }
 
 TEST_F(BaseScrollBarButtonTest, CallbackStopsFiringAfterMouseReleased) {
-  EXPECT_CALL(listener(), ButtonPressed(_, _)).Times(AtLeast(2));
+  EXPECT_CALL(callback(), ButtonPressed()).Times(AtLeast(2));
 
-  // By default the button should notify its listener on mouse release.
+  // By default the button should notify its callback on mouse release.
   button()->OnMousePressed(ui::MouseEvent(
       ui::ET_MOUSE_PRESSED, gfx::Point(), gfx::Point(), ui::EventTimeForNow(),
       ui::EF_LEFT_MOUSE_BUTTON, ui::EF_LEFT_MOUSE_BUTTON));
 
   AdvanceTime(RepeatController::GetInitialWaitForTesting() * 10);
 
-  testing::Mock::VerifyAndClearExpectations(&listener());
+  testing::Mock::VerifyAndClearExpectations(&callback());
 
   button()->OnMouseReleased(ui::MouseEvent(
       ui::ET_MOUSE_RELEASED, gfx::Point(), gfx::Point(), ui::EventTimeForNow(),
@@ -113,26 +110,26 @@ TEST_F(BaseScrollBarButtonTest, CallbackStopsFiringAfterMouseReleased) {
 
   AdvanceTime(RepeatController::GetInitialWaitForTesting() * 10);
 
-  EXPECT_CALL(listener(), ButtonPressed(_, _)).Times(AtMost(0));
+  EXPECT_CALL(callback(), ButtonPressed()).Times(AtMost(0));
 }
 
 TEST_F(BaseScrollBarButtonTest, CallbackStopsFiringAfterMouseCaptureReleased) {
-  EXPECT_CALL(listener(), ButtonPressed(_, _)).Times(AtLeast(2));
+  EXPECT_CALL(callback(), ButtonPressed()).Times(AtLeast(2));
 
-  // By default the button should notify its listener on mouse release.
+  // By default the button should notify its callback on mouse release.
   button()->OnMousePressed(ui::MouseEvent(
       ui::ET_MOUSE_PRESSED, gfx::Point(), gfx::Point(), ui::EventTimeForNow(),
       ui::EF_LEFT_MOUSE_BUTTON, ui::EF_LEFT_MOUSE_BUTTON));
 
   AdvanceTime(RepeatController::GetInitialWaitForTesting() * 10);
 
-  testing::Mock::VerifyAndClearExpectations(&listener());
+  testing::Mock::VerifyAndClearExpectations(&callback());
 
   button()->OnMouseCaptureLost();
 
   AdvanceTime(RepeatController::GetInitialWaitForTesting() * 10);
 
-  EXPECT_CALL(listener(), ButtonPressed(_, _)).Times(AtMost(0));
+  EXPECT_CALL(callback(), ButtonPressed()).Times(AtMost(0));
 }
 
 }  // namespace views

ui/views/controls/scrollbar/scroll_bar_views.cc

@@ -35,7 +35,7 @@ class ScrollBarButton : public BaseScrollBarButton {
     kRight,
   };
 
-  ScrollBarButton(ButtonListener* listener, Type type);
+  ScrollBarButton(PressedCallback callback, Type type);
   ~ScrollBarButton() override;
 
   gfx::Size CalculatePreferredSize() const override;
@@ -70,11 +70,8 @@ class ScrollBarThumb : public BaseScrollBarThumb {
   ScrollBar* scroll_bar_;
 };
 
-/////////////////////////////////////////////////////////////////////////////
-// ScrollBarButton
-
-ScrollBarButton::ScrollBarButton(ButtonListener* listener, Type type)
-    : BaseScrollBarButton(listener), type_(type) {
+ScrollBarButton::ScrollBarButton(PressedCallback callback, Type type)
+    : BaseScrollBarButton(std::move(callback)), type_(type) {
   EnableCanvasFlippingForRTLUI(true);
   DCHECK_EQ(FocusBehavior::NEVER, GetFocusBehavior());
 }
@@ -142,9 +139,6 @@ ui::NativeTheme::State ScrollBarButton::GetNativeThemeState() const {
   return ui::NativeTheme::kNormal;
 }
 
-/////////////////////////////////////////////////////////////////////////////
-// ScrollBarThumb
-
 ScrollBarThumb::ScrollBarThumb(ScrollBar* scroll_bar)
     : BaseScrollBarThumb(scroll_bar), scroll_bar_(scroll_bar) {}
 
@@ -201,34 +195,24 @@ ui::NativeTheme::State ScrollBarThumb::GetNativeThemeState() const {
 
 }  // namespace
 
-////////////////////////////////////////////////////////////////////////////////
-// ScrollBarViews, public:
-
 ScrollBarViews::ScrollBarViews(bool horizontal) : ScrollBar(horizontal) {
   EnableCanvasFlippingForRTLUI(true);
   state_ = ui::NativeTheme::kNormal;
 
   auto* layout = SetLayoutManager(std::make_unique<views::FlexLayout>());
-
-  std::unique_ptr<ScrollBarButton> prev_button, next_button;
-  using Type = ScrollBarButton::Type;
-  if (horizontal) {
-    prev_button = std::make_unique<ScrollBarButton>(this, Type::kLeft);
-    next_button = std::make_unique<ScrollBarButton>(this, Type::kRight);
-
-    part_ = ui::NativeTheme::kScrollbarHorizontalTrack;
-  } else {
+  if (!horizontal)
     layout->SetOrientation(views::LayoutOrientation::kVertical);
 
-    prev_button = std::make_unique<ScrollBarButton>(this, Type::kUp);
-    next_button = std::make_unique<ScrollBarButton>(this, Type::kDown);
-
-    part_ = ui::NativeTheme::kScrollbarVerticalTrack;
-  }
-  prev_button->set_context_menu_controller(this);
-  next_button->set_context_menu_controller(this);
+  const auto scroll_func = [](ScrollBarViews* scrollbar, ScrollAmount amount) {
+    scrollbar->ScrollByAmount(amount);
+  };
+  using Type = ScrollBarButton::Type;
+  prev_button_ = AddChildView(std::make_unique<ScrollBarButton>(
+      base::BindRepeating(scroll_func, base::Unretained(this),
+                          ScrollAmount::kPrevLine),
+      horizontal ? Type::kLeft : Type::kUp));
+  prev_button_->set_context_menu_controller(this);
 
-  prev_button_ = AddChildView(std::move(prev_button));
   SetThumb(new ScrollBarThumb(this));
   // Allow the thumb to take up the whole size of the scrollbar, save for the
   // prev/next buttons.  Layout need only set the thumb cross-axis coordinate;
@@ -237,7 +221,14 @@ ScrollBarViews::ScrollBarViews(bool horizontal) : ScrollBar(horizontal) {
       views::kFlexBehaviorKey,
       views::FlexSpecification(views::MinimumFlexSizeRule::kPreferred,
                                views::MaximumFlexSizeRule::kUnbounded));
-  next_button_ = AddChildView(std::move(next_button));
+
+  next_button_ = AddChildView(std::make_unique<ScrollBarButton>(
+      base::BindRepeating(scroll_func, base::Unretained(this),
+                          ScrollBar::ScrollAmount::kNextLine),
+      horizontal ? Type::kRight : Type::kDown));
+  next_button_->set_context_menu_controller(this);
+  part_ = horizontal ? ui::NativeTheme::kScrollbarHorizontalTrack
+                     : ui::NativeTheme::kScrollbarVerticalTrack;
 }
 
 ScrollBarViews::~ScrollBarViews() = default;
@@ -259,9 +250,6 @@ int ScrollBarViews::GetVerticalScrollBarWidth(const ui::NativeTheme* theme) {
   return std::max(track_size.width(), button_size.width());
 }
 
-////////////////////////////////////////////////////////////////////////////////
-// ScrollBarViews, View overrides:
-
 void ScrollBarViews::OnPaint(gfx::Canvas* canvas) {
   gfx::Rect bounds = GetTrackBounds();
   if (bounds.IsEmpty())
@@ -301,19 +289,6 @@ int ScrollBarViews::GetThickness() const {
   return IsHorizontal() ? size.height() : size.width();
 }
 
-//////////////////////////////////////////////////////////////////////////////
-// BaseButton::ButtonListener overrides:
-
-void ScrollBarViews::ButtonPressed(Button* sender, const ui::Event& event) {
-  const bool is_prev = sender == prev_button_;
-  DCHECK(is_prev || sender == next_button_);
-  ScrollByAmount(is_prev ? ScrollBar::ScrollAmount::kPrevLine
-                         : ScrollBar::ScrollAmount::kNextLine);
-}
-
-////////////////////////////////////////////////////////////////////////////////
-// ScrollBarViews, private:
-
 gfx::Rect ScrollBarViews::GetTrackBounds() const {
   gfx::Rect bounds = GetLocalBounds();
   gfx::Size size = prev_button_->GetPreferredSize();

ui/views/controls/scrollbar/scroll_bar_views.h

@@ -8,7 +8,6 @@
 #include "base/compiler_specific.h"
 #include "base/macros.h"
 #include "ui/gfx/geometry/point.h"
-#include "ui/native_theme/native_theme.h"
 #include "ui/views/controls/button/button.h"
 #include "ui/views/controls/scrollbar/scroll_bar.h"
 #include "ui/views/view.h"
@@ -20,7 +19,7 @@ class Canvas;
 namespace views {
 
 // Views implementation for the scrollbar.
-class VIEWS_EXPORT ScrollBarViews : public ScrollBar, public ButtonListener {
+class VIEWS_EXPORT ScrollBarViews : public ScrollBar {
  public:
   METADATA_HEADER(ScrollBarViews);
 
@@ -37,9 +36,6 @@ class VIEWS_EXPORT ScrollBarViews : public ScrollBar, public ButtonListener {
   // ScrollBar overrides:
   int GetThickness() const override;
 
-  // BaseButton::ButtonListener overrides:
-  void ButtonPressed(Button* sender, const ui::Event& event) override;
-
   // Returns the area for the track. This is the area of the scrollbar minus
   // the size of the arrow buttons.
   gfx::Rect GetTrackBounds() const override;