Use callbacks instead of ButtonPressed overrides: c/b/ui/views/infobars/

Bug: 772945
Change-Id: I81bdb9ed1de679f6b726c672bbd1519c0fcb2c7b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2436413
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#811514}

chrome/browser/ui/views/hung_plugin_tab_helper_unittest.cc

@@ -11,6 +11,8 @@
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
 #include "chrome/test/views/chrome_test_views_delegate.h"
 #include "ui/events/event.h"
+#include "ui/views/controls/button/md_text_button.h"
+#include "ui/views/test/button_test_api.h"
 
 class HungPluginInfoBarObserver : public infobars::InfoBarManager::Observer {
  public:
@@ -38,59 +40,15 @@ void HungPluginInfoBarObserver::OnInfoBarRemoved(infobars::InfoBar* infobar,
   seen_removal_ = true;
 }
 
-class HungPluginMockInfoBar : public ConfirmInfoBar {
- public:
-  explicit HungPluginMockInfoBar(
-      std::unique_ptr<ConfirmInfoBarDelegate> delegate);
-
- private:
-  // ConfirmInfoBar:
-  void ButtonPressed(views::Button* sender, const ui::Event& event) override;
-};
-
-HungPluginMockInfoBar::HungPluginMockInfoBar(
-    std::unique_ptr<ConfirmInfoBarDelegate> delegate)
-    : ConfirmInfoBar(std::move(delegate)) {}
-
-void HungPluginMockInfoBar::ButtonPressed(views::Button* sender,
-                                          const ui::Event& event) {
-  DCHECK(owner());
-  HungPluginInfoBarObserver observer(owner());
-  if (GetDelegate()->Accept()) {
+void RemoveOnlyOnce(ConfirmInfoBar* infobar) {
+  DCHECK(infobar->owner());
+  HungPluginInfoBarObserver observer(infobar->owner());
+  if (infobar->GetDelegate()->Accept()) {
     ASSERT_FALSE(observer.seen_removal());
-    RemoveSelf();
+    infobar->RemoveSelf();
   }
 }
 
-class HungPluginMockInfoBarService : public InfoBarService {
- public:
-  // Creates a HungPluginMockInfoBarService and attaches it as the
-  // InfoBarService for |web_contents|.
-  static void CreateForWebContents(content::WebContents* web_contents);
-
-  std::unique_ptr<infobars::InfoBar> CreateConfirmInfoBar(
-      std::unique_ptr<ConfirmInfoBarDelegate> delegate) override;
-
- private:
-  using InfoBarService::InfoBarService;
-};
-
-void HungPluginMockInfoBarService::CreateForWebContents(
-    content::WebContents* web_contents) {
-  DCHECK(web_contents);
-  const void* user_data_key = UserDataKey();
-  DCHECK(!web_contents->GetUserData(user_data_key));
-  web_contents->SetUserData(
-      user_data_key,
-      base::WrapUnique(new HungPluginMockInfoBarService(web_contents)));
-}
-
-std::unique_ptr<infobars::InfoBar>
-HungPluginMockInfoBarService::CreateConfirmInfoBar(
-    std::unique_ptr<ConfirmInfoBarDelegate> delegate) {
-  return std::make_unique<HungPluginMockInfoBar>(std::move(delegate));
-}
-
 class HungPluginTabHelperTest : public ChromeRenderViewHostTestHarness {
  public:
   void SetUp() override;
@@ -103,24 +61,26 @@ void HungPluginTabHelperTest::SetUp() {
   ChromeRenderViewHostTestHarness::SetUp();
 
   HungPluginTabHelper::CreateForWebContents(web_contents());
-  HungPluginMockInfoBarService::CreateForWebContents(web_contents());
+  InfoBarService::CreateForWebContents(web_contents());
 }
 
 class DummyEvent : public ui::Event {
  public:
-  DummyEvent();
+  DummyEvent() : Event(ui::ET_UNKNOWN, base::TimeTicks(), ui::EF_NONE) {}
 };
 
-DummyEvent::DummyEvent() : Event(ui::ET_UNKNOWN, base::TimeTicks(), 0) {}
-
 // Regression test for https://crbug.com/969099 .
 TEST_F(HungPluginTabHelperTest, DontRemoveTwice) {
   HungPluginTabHelper::FromWebContents(web_contents())
       ->PluginHungStatusChanged(0, base::FilePath(), true);
   InfoBarService* infobar_service =
       InfoBarService::FromWebContents(web_contents());
+  ASSERT_TRUE(infobar_service);
   ASSERT_EQ(1u, infobar_service->infobar_count());
-  static_cast<InfoBarView*>(infobar_service->infobar_at(0))
-      ->ButtonPressed(nullptr, DummyEvent());
+  auto* infobar = static_cast<ConfirmInfoBar*>(infobar_service->infobar_at(0));
+  views::MdTextButton* ok_button = infobar->ok_button_for_testing();
+  ok_button->set_callback(
+      base::BindRepeating(&RemoveOnlyOnce, base::Unretained(infobar)));
+  views::test::ButtonTestApi(ok_button).NotifyClick(DummyEvent());
   EXPECT_EQ(0u, infobar_service->infobar_count());
 }

chrome/browser/ui/views/infobars/confirm_infobar.cc

@@ -36,9 +36,24 @@ ConfirmInfoBar::ConfirmInfoBar(std::unique_ptr<ConfirmInfoBarDelegate> delegate)
   label_->SetElideBehavior(delegate_ptr->GetMessageElideBehavior());
   AddChildView(label_);
 
+  const auto create_button = [this](ConfirmInfoBarDelegate::InfoBarButton type,
+                                    void (ConfirmInfoBar::*click_function)()) {
+    auto* button = AddChildView(std::make_unique<views::MdTextButton>(
+        base::BindRepeating(click_function, base::Unretained(this)),
+        GetDelegate()->GetButtonLabel(type)));
+    button->SetProperty(
+        views::kMarginsKey,
+        gfx::Insets(ChromeLayoutProvider::Get()->GetDistanceMetric(
+                        DISTANCE_TOAST_CONTROL_VERTICAL),
+                    0));
+    button->SizeToPreferredSize();
+    return button;
+  };
+
   const auto buttons = delegate_ptr->GetButtons();
   if (buttons & ConfirmInfoBarDelegate::BUTTON_OK) {
-    ok_button_ = CreateButton(ConfirmInfoBarDelegate::BUTTON_OK);
+    ok_button_ = create_button(ConfirmInfoBarDelegate::BUTTON_OK,
+                               &ConfirmInfoBar::OkButtonPressed);
     ok_button_->SetProminent(true);
     if (delegate_ptr->OKButtonTriggersUACPrompt()) {
       elevation_icon_setter_ = std::make_unique<ElevationIconSetter>(
@@ -48,7 +63,8 @@ ConfirmInfoBar::ConfirmInfoBar(std::unique_ptr<ConfirmInfoBarDelegate> delegate)
   }
 
   if (buttons & ConfirmInfoBarDelegate::BUTTON_CANCEL) {
-    cancel_button_ = CreateButton(ConfirmInfoBarDelegate::BUTTON_CANCEL);
+    cancel_button_ = create_button(ConfirmInfoBarDelegate::BUTTON_CANCEL,
+                                   &ConfirmInfoBar::CancelButtonPressed);
     if (buttons == ConfirmInfoBarDelegate::BUTTON_CANCEL)
       cancel_button_->SetProminent(true);
   }
@@ -92,43 +108,27 @@ void ConfirmInfoBar::Layout() {
   link_->SetPosition(gfx::Point(EndX() - link_->width(), OffsetY(link_)));
 }
 
-void ConfirmInfoBar::ButtonPressed(views::Button* sender,
-                                   const ui::Event& event) {
+void ConfirmInfoBar::OkButtonPressed() {
   if (!owner())
     return;  // We're closing; don't call anything, it might access the owner.
-  ConfirmInfoBarDelegate* delegate = GetDelegate();
-  if (sender == ok_button_) {
-    if (delegate->Accept())
-      RemoveSelf();
-  } else if (sender == cancel_button_) {
-    if (delegate->Cancel())
-      RemoveSelf();
-  } else {
-    InfoBarView::ButtonPressed(sender, event);
-  }
+  if (GetDelegate()->Accept())
+    RemoveSelf();
 }
 
-int ConfirmInfoBar::ContentMinimumWidth() const {
-  return label_->GetMinimumSize().width() + link_->GetMinimumSize().width() +
-         NonLabelWidth();
+void ConfirmInfoBar::CancelButtonPressed() {
+  if (!owner())
+    return;  // We're closing; don't call anything, it might access the owner.
+  if (GetDelegate()->Cancel())
+    RemoveSelf();
 }
 
 ConfirmInfoBarDelegate* ConfirmInfoBar::GetDelegate() {
   return delegate()->AsConfirmInfoBarDelegate();
 }
 
-views::MdTextButton* ConfirmInfoBar::CreateButton(
-    ConfirmInfoBarDelegate::InfoBarButton type) {
-  auto button = std::make_unique<views::MdTextButton>(
-      this, GetDelegate()->GetButtonLabel(type));
-  button->SetProperty(
-      views::kMarginsKey,
-      gfx::Insets(ChromeLayoutProvider::Get()->GetDistanceMetric(
-                      DISTANCE_TOAST_CONTROL_VERTICAL),
-                  0));
-  auto* button_ptr = AddChildView(std::move(button));
-  button_ptr->SizeToPreferredSize();
-  return button_ptr;
+int ConfirmInfoBar::ContentMinimumWidth() const {
+  return label_->GetMinimumSize().width() + link_->GetMinimumSize().width() +
+         NonLabelWidth();
 }
 
 int ConfirmInfoBar::NonLabelWidth() const {

chrome/browser/ui/views/infobars/confirm_infobar.h

@@ -13,7 +13,6 @@
 class ElevationIconSetter;
 
 namespace views {
-class Button;
 class Label;
 class MdTextButton;
 }
@@ -28,17 +27,18 @@ class ConfirmInfoBar : public InfoBarView {
 
   // InfoBarView:
   void Layout() override;
-  void ButtonPressed(views::Button* sender, const ui::Event& event) override;
+
+  ConfirmInfoBarDelegate* GetDelegate();
+
+  views::MdTextButton* ok_button_for_testing() { return ok_button_; }
 
  protected:
   // InfoBarView:
   int ContentMinimumWidth() const override;
 
-  ConfirmInfoBarDelegate* GetDelegate();
-
  private:
-  // Creates a button suitable for use as either OK or Cancel.
-  views::MdTextButton* CreateButton(ConfirmInfoBarDelegate::InfoBarButton type);
+  void OkButtonPressed();
+  void CancelButtonPressed();
 
   // Returns the width of all content other than the label and link.  Layout()
   // uses this to determine how much space the label and link can take.

chrome/browser/ui/views/infobars/infobar_view.cc

@@ -116,7 +116,8 @@ InfoBarView::InfoBarView(std::unique_ptr<infobars::InfoBarDelegate> delegate)
   }
 
   if (this->delegate()->IsCloseable()) {
-    auto close_button = views::CreateVectorImageButton(this);
+    auto close_button = views::CreateVectorImageButton(base::BindRepeating(
+        &InfoBarView::CloseButtonPressed, base::Unretained(this)));
     // This is the wrong color, but allows the button's size to be computed
     // correctly.  We'll reset this with the correct color in OnThemeChanged().
     views::SetImageFromVectorIcon(close_button.get(),
@@ -250,16 +251,6 @@ void InfoBarView::OnThemeChanged() {
   RecalculateHeight();
 }
 
-void InfoBarView::ButtonPressed(views::Button* sender,
-                                const ui::Event& event) {
-  if (!owner())
-    return;  // We're closing; don't call anything, it might access the owner.
-  if (sender == close_button_) {
-    delegate()->InfoBarDismissed();
-    RemoveSelf();
-  }
-}
-
 void InfoBarView::OnWillChangeFocus(View* focused_before, View* focused_now) {
   views::ExternalFocusTracker::OnWillChangeFocus(focused_before, focused_now);
 
@@ -408,3 +399,10 @@ void InfoBarView::LinkClicked(const ui::Event& event) {
   if (delegate()->LinkClicked(ui::DispositionFromEventFlags(event.flags())))
     RemoveSelf();
 }
+
+void InfoBarView::CloseButtonPressed() {
+  if (!owner())
+    return;  // We're closing; don't call anything, it might access the owner.
+  delegate()->InfoBarDismissed();
+  RemoveSelf();
+}

chrome/browser/ui/views/infobars/infobar_view.h

@@ -10,9 +10,9 @@
 #include "components/infobars/core/infobar.h"
 #include "components/infobars/core/infobar_container.h"
 #include "third_party/skia/include/core/SkPath.h"
-#include "ui/views/controls/button/button.h"
 #include "ui/views/controls/menu/menu_types.h"
 #include "ui/views/focus/external_focus_tracker.h"
+#include "ui/views/view.h"
 
 namespace views {
 class ImageButton;
@@ -24,7 +24,6 @@ class MenuRunner;
 
 class InfoBarView : public infobars::InfoBar,
                     public views::View,
-                    public views::ButtonListener,
                     public views::ExternalFocusTracker {
  public:
   explicit InfoBarView(std::unique_ptr<infobars::InfoBarDelegate> delegate);
@@ -42,11 +41,6 @@ class InfoBarView : public infobars::InfoBar,
   void OnPaint(gfx::Canvas* canvas) override;
   void OnThemeChanged() override;
 
-  // views::ButtonListener:
-  // NOTE: This must not be called if we're unowned.  (Subclasses should ignore
-  // calls to ButtonPressed() in this case.)
-  void ButtonPressed(views::Button* sender, const ui::Event& event) override;
-
   // views::ExternalFocusTracker:
   void OnWillChangeFocus(View* focused_before, View* focused_now) override;
 
@@ -111,6 +105,8 @@ class InfoBarView : public infobars::InfoBar,
   // Callback used by the link created by CreateLink().
   void LinkClicked(const ui::Event& event);
 
+  void CloseButtonPressed();
+
   // The optional icon at the left edge of the InfoBar.
   views::ImageView* icon_ = nullptr;