use callbacks instead of ButtonPressed for /page_info

Bug: 772945
Change-Id: I852f6f68d1faac5940b3d8a38acca7a5e180d66e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2447097
Commit-Queue: Elaine Chien <elainec@chromium.org>
Reviewed-by: Balazs Engedy <engedy@chromium.org>
Reviewed-by: Joe DeBlasio <jdeblasio@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#814517}

chrome/browser/ui/views/page_info/chosen_object_view.cc

@@ -7,6 +7,7 @@
 #include <memory>
 #include <utility>
 
+#include "base/bind.h"
 #include "chrome/browser/ui/views/chrome_layout_provider.h"
 #include "chrome/browser/ui/views/chrome_typography.h"
 #include "chrome/browser/ui/views/page_info/chosen_object_view_observer.h"
@@ -78,7 +79,9 @@ ChosenObjectView::ChosenObjectView(
 
   // Create the delete button.
   std::unique_ptr<views::ImageButton> delete_button =
-      views::CreateVectorImageButton(this);
+      views::CreateVectorImageButton(base::BindRepeating(
+          [](ChosenObjectView* view) { view->ExecuteDeleteCommand(); }, this));
+
   views::SetImageFromVectorIcon(
       delete_button.get(), vector_icons::kCloseRoundedIcon,
       views::style::GetColor(*this, views::style::CONTEXT_DIALOG_BODY_TEXT,
@@ -133,8 +136,7 @@ void ChosenObjectView::AddObserver(ChosenObjectViewObserver* observer) {
 
 ChosenObjectView::~ChosenObjectView() {}
 
-void ChosenObjectView::ButtonPressed(views::Button* sender,
-                                     const ui::Event& event) {
+void ChosenObjectView::ExecuteDeleteCommand() {
   // Change the icon to reflect the selected setting.
   UpdateIconImage(/*is_deleted=*/true);
 

chrome/browser/ui/views/page_info/chosen_object_view.h

@@ -8,7 +8,6 @@
 #include "base/macros.h"
 #include "base/strings/string16.h"
 #include "components/page_info/page_info_ui.h"
-#include "ui/views/controls/button/button.h"
 #include "ui/views/view.h"
 
 namespace views {
@@ -21,7 +20,7 @@ class ChosenObjectViewObserver;
 // A ChosenObjectView is a row in the Page Info bubble that shows an individual
 // object (e.g. a Bluetooth device, a USB device) that the current site has
 // access to.
-class ChosenObjectView : public views::View, public views::ButtonListener {
+class ChosenObjectView : public views::View {
  public:
   explicit ChosenObjectView(std::unique_ptr<PageInfoUI::ChosenObjectInfo> info,
                             base::string16 display_name);
@@ -32,14 +31,13 @@ class ChosenObjectView : public views::View, public views::ButtonListener {
   // views:View:
   void OnThemeChanged() override;
 
-  // views::ButtonListener implementation.
-  void ButtonPressed(views::Button* sender, const ui::Event& event) override;
-
  private:
   SkColor GetObjectIconColor() const;
 
   void UpdateIconImage(bool is_deleted) const;
 
+  void ExecuteDeleteCommand();
+
   views::ImageView* icon_;             // Owned by the views hierarchy.
   views::ImageButton* delete_button_;  // Owned by the views hierarchy.
 

chrome/browser/ui/views/page_info/page_info_bubble_view.cc

@@ -115,18 +115,6 @@ void AddColumnWithSideMargin(views::GridLayout* layout, int margin, int id) {
   column_set->AddPaddingColumn(views::GridLayout::kFixedSize, margin);
 }
 
-std::unique_ptr<views::View> CreateSiteSettingsLink(
-    const int side_margin,
-    views::ButtonListener* listener) {
-  const base::string16& tooltip =
-      l10n_util::GetStringUTF16(IDS_PAGE_INFO_SITE_SETTINGS_TOOLTIP);
-  return std::make_unique<PageInfoHoverButton>(
-      listener, PageInfoUI::GetSiteSettingsIcon(GetRelatedTextColor()),
-      IDS_PAGE_INFO_SITE_SETTINGS_LINK, base::string16(),
-      PageInfoBubbleView::VIEW_ID_PAGE_INFO_LINK_OR_BUTTON_SITE_SETTINGS,
-      tooltip, base::string16());
-}
-
 }  // namespace
 
 // BubbleHeaderView is the UI element (view) that represents the header of a
@@ -151,7 +139,10 @@ class BubbleHeaderView : public views::View {
   // Adds the change password and mark site as legitimate buttons.
   // If |is_saved_password|, adds a check password button instead of
   // change password button.
-  void AddPasswordReuseButtons(bool is_saved_password);
+  void AddPasswordReuseButtons(
+      bool is_saved_password,
+      views::Button::PressedCallback change_password_callback,
+      views::Button::PressedCallback password_reuse_callback);
 
  private:
   // Owns |this|.
@@ -295,7 +286,10 @@ void BubbleHeaderView::AddResetDecisionsLabel() {
   InvalidateLayout();
 }
 
-void BubbleHeaderView::AddPasswordReuseButtons(bool is_saved_password) {
+void BubbleHeaderView::AddPasswordReuseButtons(
+    bool is_saved_password,
+    views::Button::PressedCallback change_password_callback,
+    views::Button::PressedCallback password_reuse_callback) {
   if (!password_reuse_button_container_->children().empty()) {
     // Ensure all old content is removed from the container before re-adding it.
     password_reuse_button_container_->RemoveAllChildViews(true /* delete */);
@@ -308,13 +302,14 @@ void BubbleHeaderView::AddPasswordReuseButtons(bool is_saved_password) {
   std::unique_ptr<views::MdTextButton> change_password_button;
   if (change_password_template) {
     change_password_button = std::make_unique<views::MdTextButton>(
-        bubble_, l10n_util::GetStringUTF16(change_password_template));
+        change_password_callback,
+        l10n_util::GetStringUTF16(change_password_template));
     change_password_button->SetProminent(true);
     change_password_button->SetID(
         PageInfoBubbleView::VIEW_ID_PAGE_INFO_BUTTON_CHANGE_PASSWORD);
   }
   auto allowlist_password_reuse_button = std::make_unique<views::MdTextButton>(
-      bubble_,
+      password_reuse_callback,
       l10n_util::GetStringUTF16(IDS_PAGE_INFO_ALLOWLIST_PASSWORD_REUSE_BUTTON));
   allowlist_password_reuse_button->SetID(
       PageInfoBubbleView::VIEW_ID_PAGE_INFO_BUTTON_ALLOWLIST_PASSWORD_REUSE);
@@ -543,7 +538,19 @@ PageInfoBubbleView::PageInfoBubbleView(
   if (!profile->IsGuestSession()) {
     layout->StartRowWithPadding(views::GridLayout::kFixedSize, kColumnId,
                                 views::GridLayout::kFixedSize, 0);
-    layout->AddView(CreateSiteSettingsLink(side_margin, this));
+
+    const base::string16& tooltip =
+        l10n_util::GetStringUTF16(IDS_PAGE_INFO_SITE_SETTINGS_TOOLTIP);
+    site_settings_link = layout->AddView(std::make_unique<PageInfoHoverButton>(
+        base::BindRepeating(
+            [](PageInfoBubbleView* view) {
+              view->HandleMoreInfoRequest(view->site_settings_link);
+            },
+            this),
+        PageInfoUI::GetSiteSettingsIcon(GetRelatedTextColor()),
+        IDS_PAGE_INFO_SITE_SETTINGS_LINK, base::string16(),
+        PageInfoBubbleView::VIEW_ID_PAGE_INFO_LINK_OR_BUTTON_SITE_SETTINGS,
+        tooltip, base::string16()));
   }
 
 #if defined(OS_WIN) && BUILDFLAG(ENABLE_VR)
@@ -595,38 +602,6 @@ void PageInfoBubbleView::OnWidgetDestroying(views::Widget* widget) {
     std::move(closing_callback_).Run(widget->closed_reason(), reload_prompt);
 }
 
-void PageInfoBubbleView::ButtonPressed(views::Button* button,
-                                       const ui::Event& event) {
-  switch (button->GetID()) {
-    case PageInfoBubbleView::VIEW_ID_PAGE_INFO_BUTTON_CLOSE:
-      GetWidget()->Close();
-      break;
-    case PageInfoBubbleView::VIEW_ID_PAGE_INFO_BUTTON_CHANGE_PASSWORD:
-      presenter_->OnChangePasswordButtonPressed(web_contents());
-      break;
-    case PageInfoBubbleView::VIEW_ID_PAGE_INFO_BUTTON_ALLOWLIST_PASSWORD_REUSE:
-      GetWidget()->Close();
-      presenter_->OnWhitelistPasswordReuseButtonPressed(web_contents());
-      break;
-    case PageInfoBubbleView::VIEW_ID_PAGE_INFO_LINK_OR_BUTTON_SITE_SETTINGS:
-    case PageInfoBubbleView::VIEW_ID_PAGE_INFO_LINK_OR_BUTTON_COOKIE_DIALOG:
-    case PageInfoBubbleView::
-        VIEW_ID_PAGE_INFO_LINK_OR_BUTTON_CERTIFICATE_VIEWER:
-      HandleMoreInfoRequest(button);
-      break;
-    case PageInfoBubbleView::VIEW_ID_PAGE_INFO_HOVER_BUTTON_VR_PRESENTATION:
-      // Ignore clicks on the "VR is presenting" row.
-      break;
-    case PageInfoBubbleView::VIEW_ID_PAGE_INFO_BUTTON_END_VR:
-      GetWidget()->Close();
-#if BUILDFLAG(ENABLE_VR)
-      vr::VrTabHelper::ExitVrPresentation();
-#endif
-      break;
-    default:
-      NOTREACHED();
-  }
-}
 
 gfx::Size PageInfoBubbleView::CalculatePreferredSize() const {
   if (header_ == nullptr && site_settings_view_ == nullptr) {
@@ -673,7 +648,12 @@ void PageInfoBubbleView::SetCookieInfo(const CookieInfoList& cookie_info_list) {
 
     cookie_button_ =
         std::make_unique<PageInfoHoverButton>(
-            this, icon, IDS_PAGE_INFO_COOKIES_BUTTON_TEXT, num_cookies_text,
+            base::BindRepeating(
+                [](PageInfoBubbleView* view) {
+                  view->HandleMoreInfoRequest(view->cookie_button_);
+                },
+                this),
+            icon, IDS_PAGE_INFO_COOKIES_BUTTON_TEXT, num_cookies_text,
             VIEW_ID_PAGE_INFO_LINK_OR_BUTTON_COOKIE_DIALOG, tooltip,
             base::string16())
             .release();
@@ -857,7 +837,12 @@ void PageInfoBubbleView::SetIdentityInfo(const IdentityInfo& identity_info) {
     }
     certificate_button_ = site_settings_view_->AddChildView(
         std::make_unique<PageInfoHoverButton>(
-            this, icon, IDS_PAGE_INFO_CERTIFICATE_BUTTON_TEXT, secondary_text,
+            base::BindRepeating(
+                [](PageInfoBubbleView* view) {
+                  view->HandleMoreInfoRequest(view->certificate_button_);
+                },
+                this),
+            icon, IDS_PAGE_INFO_CERTIFICATE_BUTTON_TEXT, secondary_text,
             VIEW_ID_PAGE_INFO_LINK_OR_BUTTON_CERTIFICATE_VIEWER, tooltip,
             subtitle_text)
             .release());
@@ -866,8 +851,21 @@ void PageInfoBubbleView::SetIdentityInfo(const IdentityInfo& identity_info) {
   if (identity_info.show_change_password_buttons) {
     header_->AddPasswordReuseButtons(
         identity_info.safe_browsing_status ==
-        PageInfo::SafeBrowsingStatus::
-            SAFE_BROWSING_STATUS_SAVED_PASSWORD_REUSE);
+            PageInfo::SafeBrowsingStatus::
+                SAFE_BROWSING_STATUS_SAVED_PASSWORD_REUSE,
+        base::BindRepeating(
+            [](PageInfoBubbleView* view) {
+              view->presenter_->OnChangePasswordButtonPressed(
+                  view->web_contents());
+            },
+            this),
+        base::BindRepeating(
+            [](PageInfoBubbleView* view) {
+              view->GetWidget()->Close();
+              view->presenter_->OnWhitelistPasswordReuseButtonPressed(
+                  view->web_contents());
+            },
+            this));
   }
   details_text_ = security_description->details;
   header_->SetDetails(security_description->details);
@@ -890,14 +888,21 @@ void PageInfoBubbleView::SetPageFeatureInfo(const PageFeatureInfo& info) {
 
   auto icon = std::make_unique<NonAccessibleImageView>();
   icon->SetImage(PageInfoUI::GetVrSettingsIcon(GetRelatedTextColor()));
-
   auto exit_button = std::make_unique<views::MdTextButton>(
-      this, l10n_util::GetStringUTF16(IDS_PAGE_INFO_VR_TURN_OFF_BUTTON_TEXT));
+      base::BindRepeating(
+          [](PageInfoBubbleView* view) {
+            view->GetWidget()->Close();
+#if BUILDFLAG(ENABLE_VR)
+            vr::VrTabHelper::ExitVrPresentation();
+#endif
+          },
+          this),
+      l10n_util::GetStringUTF16(IDS_PAGE_INFO_VR_TURN_OFF_BUTTON_TEXT));
   exit_button->SetID(VIEW_ID_PAGE_INFO_BUTTON_END_VR);
   exit_button->SetProminent(true);
 
   auto button = std::make_unique<HoverButton>(
-      this, std::move(icon),
+      views::Button::PressedCallback(), std::move(icon),
       l10n_util::GetStringUTF16(IDS_PAGE_INFO_VR_PRESENTING_TEXT),
       base::string16(), std::move(exit_button),
       false,  // Try not to change the row height while adding secondary view

chrome/browser/ui/views/page_info/page_info_bubble_view.h

@@ -25,7 +25,6 @@
 #include "components/safe_browsing/buildflags.h"
 #include "components/security_state/core/security_state.h"
 #include "ui/gfx/native_widget_types.h"
-#include "ui/views/controls/button/button.h"
 #include "ui/views/controls/separator.h"
 #include "ui/views/controls/styled_label.h"
 #include "ui/views/widget/widget.h"
@@ -58,7 +57,6 @@ class View;
 class PageInfoBubbleView : public PageInfoBubbleViewBase,
                            public PermissionSelectorRowObserver,
                            public ChosenObjectViewObserver,
-                           public views::ButtonListener,
                            public PageInfoUI {
  public:
   // The width of the column size for permissions and chosen object icons.
@@ -70,7 +68,6 @@ class PageInfoBubbleView : public PageInfoBubbleViewBase,
 
   enum PageInfoBubbleViewID {
     VIEW_ID_NONE = 0,
-    VIEW_ID_PAGE_INFO_BUTTON_CLOSE,
     VIEW_ID_PAGE_INFO_BUTTON_CHANGE_PASSWORD,
     VIEW_ID_PAGE_INFO_BUTTON_ALLOWLIST_PASSWORD_REUSE,
     VIEW_ID_PAGE_INFO_LABEL_EV_CERTIFICATE_DETAILS,
@@ -127,9 +124,6 @@ class PageInfoBubbleView : public PageInfoBubbleViewBase,
   // ChosenObjectViewObserver:
   void OnChosenObjectDeleted(const PageInfoUI::ChosenObjectInfo& info) override;
 
-  // views::ButtonListener:
-  void ButtonPressed(views::Button* button, const ui::Event& event) override;
-
   // PageInfoUI:
   void SetCookieInfo(const CookieInfoList& cookie_info_list) override;
   void SetPermissionInfo(const PermissionInfoList& permission_info_list,
@@ -181,6 +175,9 @@ class PageInfoBubbleView : public PageInfoBubbleViewBase,
   // The button that opens the "Certificate" dialog.
   PageInfoHoverButton* certificate_button_ = nullptr;
 
+  // The button that opens up "Site Settings".
+  views::View* site_settings_link = nullptr;
+
   // The view that contains the "Permissions" table of the bubble.
   views::View* permissions_view_ = nullptr;
 

chrome/browser/ui/views/page_info/page_info_bubble_view_unittest.cc

@@ -49,6 +49,7 @@
 #include "ui/views/controls/combobox/combobox.h"
 #include "ui/views/controls/label.h"
 #include "ui/views/controls/link.h"
+#include "ui/views/test/button_test_api.h"
 #include "ui/views/test/scoped_views_test_helper.h"
 #include "ui/views/test/test_views_delegate.h"
 
@@ -403,8 +404,7 @@ TEST_F(PageInfoBubbleViewTest, SetPermissionInfoWithUsbDevice) {
   views::Button* button = static_cast<views::Button*>(children[2]);
   const ui::MouseEvent event(ui::ET_MOUSE_PRESSED, gfx::Point(), gfx::Point(),
                              ui::EventTimeForNow(), 0, 0);
-  static_cast<views::ButtonListener*>(object_view)
-      ->ButtonPressed(button, event);
+  views::test::ButtonTestApi(button).NotifyClick(event);
   api_->SetPermissionInfo(list);
   EXPECT_EQ(kExpectedChildren, api_->permissions_view()->children().size());
   EXPECT_FALSE(store->HasDevicePermission(origin, origin, *device_info));
@@ -461,9 +461,7 @@ TEST_F(PageInfoBubbleViewTest, SetPermissionInfoWithPolicyUsbDevices) {
   // Policy granted USB permissions should not be able to be deleted.
   const ui::MouseEvent event(ui::ET_MOUSE_PRESSED, gfx::Point(), gfx::Point(),
                              ui::EventTimeForNow(), 0, 0);
-  views::ButtonListener* button_listener =
-      static_cast<views::ButtonListener*>(object_view);
-  button_listener->ButtonPressed(button, event);
+  views::test::ButtonTestApi(button).NotifyClick(event);
   api_->SetPermissionInfo(list);
   EXPECT_EQ(kExpectedChildren + 1, api_->permissions_view()->children().size());
 }
@@ -519,9 +517,7 @@ TEST_F(PageInfoBubbleViewTest, SetPermissionInfoWithUserAndPolicyUsbDevices) {
     views::Label* desc_label = static_cast<views::Label*>(children[3]);
     EXPECT_EQ(base::ASCIIToUTF16("USB device"), desc_label->GetText());
 
-    views::ButtonListener* button_listener =
-        static_cast<views::ButtonListener*>(object_view);
-    button_listener->ButtonPressed(button, event);
+    views::test::ButtonTestApi(button).NotifyClick(event);
     api_->SetPermissionInfo(list);
     EXPECT_EQ(kExpectedChildren + 1,
               api_->permissions_view()->children().size());
@@ -547,9 +543,7 @@ TEST_F(PageInfoBubbleViewTest, SetPermissionInfoWithUserAndPolicyUsbDevices) {
     EXPECT_EQ(base::ASCIIToUTF16("USB device allowed by your administrator"),
               desc_label->GetText());
 
-    views::ButtonListener* button_listener =
-        static_cast<views::ButtonListener*>(object_view);
-    button_listener->ButtonPressed(button, event);
+    views::test::ButtonTestApi(button).NotifyClick(event);
     api_->SetPermissionInfo(list);
     EXPECT_EQ(kExpectedChildren + 1,
               api_->permissions_view()->children().size());

chrome/browser/ui/views/page_info/page_info_hover_button.cc

@@ -17,14 +17,15 @@
 #include "ui/views/layout/grid_layout.h"
 #include "ui/views/style/typography.h"
 
-PageInfoHoverButton::PageInfoHoverButton(views::ButtonListener* listener,
-                                         const gfx::ImageSkia& image_icon,
-                                         int title_resource_id,
-                                         const base::string16& secondary_text,
-                                         int click_target_id,
-                                         const base::string16& tooltip_text,
-                                         const base::string16& subtitle_text)
-    : HoverButton(listener, base::string16()) {
+PageInfoHoverButton::PageInfoHoverButton(
+    views::Button::PressedCallback callback,
+    const gfx::ImageSkia& image_icon,
+    int title_resource_id,
+    const base::string16& secondary_text,
+    int click_target_id,
+    const base::string16& tooltip_text,
+    const base::string16& subtitle_text)
+    : HoverButton(std::move(callback), base::string16()) {
   label()->SetHandlesTooltips(false);
   auto icon = std::make_unique<NonAccessibleImageView>();
   icon->SetImage(image_icon);

chrome/browser/ui/views/page_info/page_info_hover_button.h

@@ -18,7 +18,6 @@ class PageInfoBubbleViewTestApi;
 }  // namespace test
 
 namespace views {
-class ButtonListener;
 class Label;
 class StyledLabel;
 class View;
@@ -43,7 +42,7 @@ class PageInfoHoverButton : public HoverButton {
   // |-----------------------------------------------------------------|
   // |      | |subtitle_text|                                          |
   // *-----------------------------------------------------------------*
-  PageInfoHoverButton(views::ButtonListener* listener,
+  PageInfoHoverButton(views::Button::PressedCallback callback,
                       const gfx::ImageSkia& image_icon,
                       int title_resource_id,
                       const base::string16& secondary_text,

chrome/browser/ui/views/page_info/safety_tip_page_info_bubble_view.cc

@@ -181,10 +181,13 @@ SafetyTipPageInfoBubbleView::SafetyTipPageInfoBubbleView(
   info_link->AddStyleRange(details_range, link_style);
   info_link->SizeToFit(0);
   info_button_ = button_layout->AddView(std::move(info_link));
-
   // Leave site button.
   auto leave_button = std::make_unique<views::MdTextButton>(
-      this,
+      base::BindRepeating(
+          [](SafetyTipPageInfoBubbleView* view) {
+            view->ExecuteLeaveCommand();
+          },
+          this),
       l10n_util::GetStringUTF16(GetSafetyTipLeaveButtonId(safety_tip_status)));
   leave_button->SetProminent(true);
   leave_button->SetID(PageInfoBubbleView::VIEW_ID_PAGE_INFO_BUTTON_LEAVE_SITE);
@@ -233,11 +236,7 @@ void SafetyTipPageInfoBubbleView::OnWidgetDestroying(views::Widget* widget) {
   std::move(close_callback_).Run(action_taken_);
 }
 
-void SafetyTipPageInfoBubbleView::ButtonPressed(views::Button* button,
-                                                const ui::Event& event) {
-  DCHECK_EQ(button->GetID(),
-            PageInfoBubbleView::VIEW_ID_PAGE_INFO_BUTTON_LEAVE_SITE);
-
+void SafetyTipPageInfoBubbleView::ExecuteLeaveCommand() {
   action_taken_ = SafetyTipInteraction::kLeaveSite;
   LeaveSiteFromSafetyTip(
       web_contents(),

chrome/browser/ui/views/page_info/safety_tip_page_info_bubble_view.h

@@ -27,8 +27,7 @@ class Widget;
 // When Chrome displays a safety tip, we create a stripped-down bubble view
 // without all of the details. Safety tip info is still displayed in the usual
 // PageInfoBubbleView, just less prominently.
-class SafetyTipPageInfoBubbleView : public PageInfoBubbleViewBase,
-                                    public views::ButtonListener {
+class SafetyTipPageInfoBubbleView : public PageInfoBubbleViewBase {
  public:
   // If |anchor_view| is nullptr, or has no Widget, |parent_window| may be
   // provided to ensure this bubble is closed when the parent closes.
@@ -48,14 +47,13 @@ class SafetyTipPageInfoBubbleView : public PageInfoBubbleViewBase,
   // views::WidgetObserver:
   void OnWidgetDestroying(views::Widget* widget) override;
 
-  // views::ButtonListener:
-  void ButtonPressed(views::Button* button, const ui::Event& event) override;
-
  private:
   friend class SafetyTipPageInfoBubbleViewBrowserTest;
 
   void OpenHelpCenter();
 
+  void ExecuteLeaveCommand();
+
   views::Button* GetLeaveButtonForTesting() { return leave_button_; }
 
   const security_state::SafetyTipStatus safety_tip_status_;