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

Bug: 772945
Change-Id: I2de0099c2287b5d9412a9c3ffa190efe29559817
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2485133
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Auto-Submit: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#818762}

ui/views/window/custom_frame_view.cc

@@ -5,6 +5,7 @@
 #include "ui/views/window/custom_frame_view.h"
 
 #include <algorithm>
+#include <utility>
 #include <vector>
 
 #include "base/containers/adapters.h"
@@ -69,18 +70,23 @@ void LayoutButton(ImageButton* button, const gfx::Rect& bounds) {
 
 CustomFrameView::CustomFrameView(Widget* frame)
     : frame_(frame), frame_background_(new FrameBackground()) {
-  close_button_ = InitWindowCaptionButton(IDS_APP_ACCNAME_CLOSE, IDR_CLOSE,
-                                          IDR_CLOSE_H, IDR_CLOSE_P);
+  close_button_ = InitWindowCaptionButton(
+      base::BindRepeating(&Widget::CloseWithReason, base::Unretained(frame_),
+                          views::Widget::ClosedReason::kCloseButtonClicked),
+      IDS_APP_ACCNAME_CLOSE, IDR_CLOSE, IDR_CLOSE_H, IDR_CLOSE_P);
   minimize_button_ = InitWindowCaptionButton(
+      base::BindRepeating(&Widget::Minimize, base::Unretained(frame_)),
       IDS_APP_ACCNAME_MINIMIZE, IDR_MINIMIZE, IDR_MINIMIZE_H, IDR_MINIMIZE_P);
   maximize_button_ = InitWindowCaptionButton(
+      base::BindRepeating(&Widget::Maximize, base::Unretained(frame_)),
       IDS_APP_ACCNAME_MAXIMIZE, IDR_MAXIMIZE, IDR_MAXIMIZE_H, IDR_MAXIMIZE_P);
   restore_button_ = InitWindowCaptionButton(
+      base::BindRepeating(&Widget::Restore, base::Unretained(frame_)),
       IDS_APP_ACCNAME_RESTORE, IDR_RESTORE, IDR_RESTORE_H, IDR_RESTORE_P);
 
   if (frame_->widget_delegate()->ShouldShowWindowIcon()) {
-    window_icon_ = new ImageButton(this);
-    AddChildView(window_icon_);
+    window_icon_ =
+        AddChildView(std::make_unique<ImageButton>(Button::PressedCallback()));
   }
 }
 
@@ -231,17 +237,6 @@ gfx::Size CustomFrameView::GetMaximumSize() const {
                    max_size.height() == 0 ? 0 : converted_size.height());
 }
 
-void CustomFrameView::ButtonPressed(Button* sender, const ui::Event& event) {
-  if (sender == close_button_)
-    frame_->CloseWithReason(views::Widget::ClosedReason::kCloseButtonClicked);
-  else if (sender == minimize_button_)
-    frame_->Minimize();
-  else if (sender == maximize_button_)
-    frame_->Maximize();
-  else if (sender == restore_button_)
-    frame_->Restore();
-}
-
 int CustomFrameView::FrameBorderThickness() const {
   return frame_->IsMaximized() ? 0 : kFrameBorderThickness;
 }
@@ -538,12 +533,14 @@ void CustomFrameView::LayoutClientView() {
 }
 
 ImageButton* CustomFrameView::InitWindowCaptionButton(
+    Button::PressedCallback callback,
     int accessibility_string_id,
     int normal_image_id,
     int hot_image_id,
     int pushed_image_id) {
   ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
-  ImageButton* button = new ImageButton(this);
+  ImageButton* button =
+      AddChildView(std::make_unique<ImageButton>(std::move(callback)));
   button->SetAccessibleName(l10n_util::GetStringUTF16(accessibility_string_id));
   button->SetImage(Button::STATE_NORMAL,
                    rb.GetImageNamed(normal_image_id).ToImageSkia());
@@ -551,7 +548,6 @@ ImageButton* CustomFrameView::InitWindowCaptionButton(
                    rb.GetImageNamed(hot_image_id).ToImageSkia());
   button->SetImage(Button::STATE_PRESSED,
                    rb.GetImageNamed(pushed_image_id).ToImageSkia());
-  AddChildView(button);
   return button;
 }
 

ui/views/window/custom_frame_view.h

@@ -33,8 +33,7 @@ class Widget;
 //  rendering the non-standard window caption, border, and controls.
 //
 ////////////////////////////////////////////////////////////////////////////////
-class VIEWS_EXPORT CustomFrameView : public NonClientFrameView,
-                                     public ButtonListener {
+class VIEWS_EXPORT CustomFrameView : public NonClientFrameView {
  public:
   explicit CustomFrameView(Widget* frame);
   ~CustomFrameView() override;
@@ -57,9 +56,6 @@ class VIEWS_EXPORT CustomFrameView : public NonClientFrameView,
   gfx::Size GetMinimumSize() const override;
   gfx::Size GetMaximumSize() const override;
 
-  // Overridden from ButtonListener:
-  void ButtonPressed(Button* sender, const ui::Event& event) override;
-
   // Returns the font list to use in the window's title bar.
   // TODO(https://crbug.com/968860): Move this into the typography provider.
   static gfx::FontList GetWindowTitleFontList();
@@ -124,7 +120,8 @@ class VIEWS_EXPORT CustomFrameView : public NonClientFrameView,
 
   // Creates, adds and returns a new window caption button (e.g, minimize,
   // maximize, restore).
-  ImageButton* InitWindowCaptionButton(int accessibility_string_id,
+  ImageButton* InitWindowCaptionButton(Button::PressedCallback callback,
+                                       int accessibility_string_id,
                                        int normal_image_id,
                                        int hot_image_id,
                                        int pushed_image_id);

ui/views/window/dialog_client_view.cc

@@ -70,9 +70,6 @@ class DialogClientView::ButtonRowContainer : public View {
   DISALLOW_COPY_AND_ASSIGN(ButtonRowContainer);
 };
 
-///////////////////////////////////////////////////////////////////////////////
-// DialogClientView, public:
-
 DialogClientView::DialogClientView(Widget* owner, View* contents_view)
     : ClientView(owner, contents_view),
       button_row_insets_(
@@ -96,9 +93,6 @@ void DialogClientView::SetButtonRowInsets(const gfx::Insets& insets) {
     UpdateDialogButtons();
 }
 
-////////////////////////////////////////////////////////////////////////////////
-// DialogClientView, View overrides:
-
 gfx::Size DialogClientView::CalculatePreferredSize() const {
   const gfx::Insets& content_margins = GetDialogDelegate()->margins();
 
@@ -208,32 +202,10 @@ void DialogClientView::OnThemeChanged() {
   }
 }
 
-////////////////////////////////////////////////////////////////////////////////
-// DialogClientView, ButtonListener implementation:
-
-void DialogClientView::ButtonPressed(Button* sender, const ui::Event& event) {
-  // Check for a valid delegate to avoid handling events after destruction.
-  if (!GetDialogDelegate())
-    return;
-
-  if (input_protector_.IsPossiblyUnintendedInteraction(event))
-    return;
-
-  if (sender == ok_button_)
-    GetDialogDelegate()->AcceptDialog();
-  else if (sender == cancel_button_)
-    GetDialogDelegate()->CancelDialog();
-  else
-    NOTREACHED();
-}
-
 void DialogClientView::ResetViewShownTimeStampForTesting() {
   input_protector_.ResetForTesting();
 }
 
-////////////////////////////////////////////////////////////////////////////////
-// DialogClientView, private:
-
 DialogDelegate* DialogClientView::GetDialogDelegate() const {
   return GetWidget()->widget_delegate()->AsDialogDelegate();
 }
@@ -276,7 +248,10 @@ void DialogClientView::UpdateDialogButton(LabelButton** member,
     return;
   }
 
-  auto button = std::make_unique<MdTextButton>(this, title);
+  auto button = std::make_unique<MdTextButton>(
+      base::BindRepeating(&DialogClientView::ButtonPressed,
+                          base::Unretained(this), type),
+      title);
   button->SetProminent(is_default);
   button->SetIsDefault(is_default);
   button->SetEnabled(delegate->IsDialogButtonEnabled(type));
@@ -290,6 +265,15 @@ void DialogClientView::UpdateDialogButton(LabelButton** member,
   *member = button_row_container_->AddChildView(std::move(button));
 }
 
+void DialogClientView::ButtonPressed(ui::DialogButton type,
+                                     const ui::Event& event) {
+  DialogDelegate* const delegate = GetDialogDelegate();
+  if (delegate && !input_protector_.IsPossiblyUnintendedInteraction(event)) {
+    (type == ui::DIALOG_BUTTON_OK) ? delegate->AcceptDialog()
+                                   : delegate->CancelDialog();
+  }
+}
+
 int DialogClientView::GetExtraViewSpacing() const {
   if (!ShouldShow(extra_view_) || !(ok_button_ || cancel_button_))
     return 0;

ui/views/window/dialog_client_view.h

@@ -9,7 +9,6 @@
 #include "base/macros.h"
 #include "base/time/time.h"
 #include "ui/base/ui_base_types.h"
-#include "ui/views/controls/button/button.h"
 #include "ui/views/input_event_activation_protector.h"
 #include "ui/views/window/client_view.h"
 #include "ui/views/window/dialog_observer.h"
@@ -34,9 +33,7 @@ class Widget;
 // You must not directly depend on or use DialogClientView; it is internal to
 // //ui/views. Access it through the public interfaces on DialogDelegate. It is
 // only VIEWS_EXPORT to make it available to views_unittests.
-class VIEWS_EXPORT DialogClientView : public ClientView,
-                                      public ButtonListener,
-                                      public DialogObserver {
+class VIEWS_EXPORT DialogClientView : public ClientView, public DialogObserver {
  public:
   METADATA_HEADER(DialogClientView);
 
@@ -62,9 +59,6 @@ class VIEWS_EXPORT DialogClientView : public ClientView,
       const ViewHierarchyChangedDetails& details) override;
   void OnThemeChanged() override;
 
-  // ButtonListener implementation:
-  void ButtonPressed(Button* sender, const ui::Event& event) override;
-
   void set_minimum_size(const gfx::Size& size) { minimum_size_ = size; }
 
   // Resets the time when view has been shown. Tests may need to call this
@@ -97,6 +91,8 @@ class VIEWS_EXPORT DialogClientView : public ClientView,
   // |member| points to a button that already exists.
   void UpdateDialogButton(LabelButton** member, ui::DialogButton type);
 
+  void ButtonPressed(ui::DialogButton type, const ui::Event& event);
+
   // Returns the spacing between the extra view and the ok/cancel buttons. 0 if
   // no extra view. Otherwise uses the default padding.
   int GetExtraViewSpacing() const;

ui/views/window/dialog_client_view_unittest.cc

@@ -4,8 +4,10 @@
 
 #include "ui/views/window/dialog_client_view.h"
 
+#include <algorithm>
 #include <map>
 #include <memory>
+#include <string>
 #include <utility>
 
 #include "base/macros.h"
@@ -21,6 +23,7 @@
 #include "ui/views/controls/button/label_button.h"
 #include "ui/views/metrics.h"
 #include "ui/views/style/platform_style.h"
+#include "ui/views/test/button_test_api.h"
 #include "ui/views/test/test_layout_provider.h"
 #include "ui/views/test/test_views.h"
 #include "ui/views/test/widget_test.h"
@@ -45,7 +48,7 @@ class DialogClientViewTest : public test::WidgetTest,
 
     // Note: not using DialogDelegate::CreateDialogWidget(..), since that can
     // alter the frame type according to the platform.
-    widget_ = new views::Widget;
+    widget_ = new Widget;
     Widget::InitParams params = CreateParams(Widget::InitParams::TYPE_WINDOW);
     params.delegate = this;
     widget_->Init(std::move(params));
@@ -132,9 +135,8 @@ class DialogClientViewTest : public test::WidgetTest,
     DialogModelChanged();
   }
 
-  views::Button* GetButtonByAccessibleName(views::View* root,
-                                           const base::string16& name) {
-    views::Button* button = Button::AsButton(root);
+  Button* GetButtonByAccessibleName(View* root, const base::string16& name) {
+    Button* button = Button::AsButton(root);
     if (button && button->GetAccessibleName() == name)
       return button;
     for (auto* child : root->children()) {
@@ -145,7 +147,7 @@ class DialogClientViewTest : public test::WidgetTest,
     return nullptr;
   }
 
-  views::Button* GetButtonByAccessibleName(const std::string& label) {
+  Button* GetButtonByAccessibleName(const std::string& label) {
     return GetButtonByAccessibleName(widget_->GetRootView(),
                                      base::UTF8ToUTF16(label));
   }
@@ -512,16 +514,16 @@ TEST_F(DialogClientViewTest, IgnorePossiblyUnintendedClicks_ClickAfterShown) {
   // Should ignore clicks right after the dialog is shown.
   ui::MouseEvent mouse_event(ui::ET_MOUSE_PRESSED, gfx::Point(), gfx::Point(),
                              ui::EventTimeForNow(), ui::EF_NONE, ui::EF_NONE);
-  client_view()->ButtonPressed(client_view()->ok_button(), mouse_event);
-  client_view()->ButtonPressed(client_view()->cancel_button(), mouse_event);
+  test::ButtonTestApi(client_view()->ok_button()).NotifyClick(mouse_event);
+  test::ButtonTestApi cancel_button(client_view()->cancel_button());
+  cancel_button.NotifyClick(mouse_event);
   EXPECT_FALSE(widget()->IsClosed());
 
-  client_view()->ButtonPressed(
-      client_view()->cancel_button(),
-      ui::MouseEvent(ui::ET_MOUSE_PRESSED, gfx::Point(), gfx::Point(),
-                     ui::EventTimeForNow() + base::TimeDelta::FromMilliseconds(
-                                                 GetDoubleClickInterval()),
-                     ui::EF_NONE, ui::EF_NONE));
+  cancel_button.NotifyClick(ui::MouseEvent(
+      ui::ET_MOUSE_PRESSED, gfx::Point(), gfx::Point(),
+      ui::EventTimeForNow() +
+          base::TimeDelta::FromMilliseconds(GetDoubleClickInterval()),
+      ui::EF_NONE, ui::EF_NONE));
   EXPECT_TRUE(widget()->IsClosed());
 }
 
@@ -538,8 +540,9 @@ TEST_F(DialogClientViewTest, IgnorePossiblyUnintendedClicks_RepeatedClicks) {
   // Should ignore clicks right after the dialog is shown.
   ui::MouseEvent mouse_event(ui::ET_MOUSE_PRESSED, gfx::Point(), gfx::Point(),
                              kNow, ui::EF_NONE, ui::EF_NONE);
-  client_view()->ButtonPressed(client_view()->ok_button(), mouse_event);
-  client_view()->ButtonPressed(client_view()->cancel_button(), mouse_event);
+  test::ButtonTestApi(client_view()->ok_button()).NotifyClick(mouse_event);
+  test::ButtonTestApi cancel_button(client_view()->cancel_button());
+  cancel_button.NotifyClick(mouse_event);
   EXPECT_FALSE(widget()->IsClosed());
 
   // Should ignore repeated clicks with short intervals, even though enough time
@@ -549,20 +552,18 @@ TEST_F(DialogClientViewTest, IgnorePossiblyUnintendedClicks_RepeatedClicks) {
   ASSERT_TRUE(kNumClicks * kRepeatedClickInterval > kShortClickInterval);
   base::TimeTicks event_time = kNow;
   for (size_t i = 0; i < kNumClicks; i++) {
-    client_view()->ButtonPressed(
-        client_view()->cancel_button(),
-        ui::MouseEvent(ui::ET_MOUSE_PRESSED, gfx::Point(), gfx::Point(),
-                       event_time, ui::EF_NONE, ui::EF_NONE));
+    cancel_button.NotifyClick(ui::MouseEvent(ui::ET_MOUSE_PRESSED, gfx::Point(),
+                                             gfx::Point(), event_time,
+                                             ui::EF_NONE, ui::EF_NONE));
     EXPECT_FALSE(widget()->IsClosed());
     event_time += kRepeatedClickInterval;
   }
 
   // Sufficient time passed, events are now allowed.
   event_time += kShortClickInterval;
-  client_view()->ButtonPressed(
-      client_view()->cancel_button(),
-      ui::MouseEvent(ui::ET_MOUSE_PRESSED, gfx::Point(), gfx::Point(),
-                     event_time, ui::EF_NONE, ui::EF_NONE));
+  cancel_button.NotifyClick(ui::MouseEvent(ui::ET_MOUSE_PRESSED, gfx::Point(),
+                                           gfx::Point(), event_time,
+                                           ui::EF_NONE, ui::EF_NONE));
   EXPECT_TRUE(widget()->IsClosed());
 }