Change ButtonPressed overrides to callbacks: c/b/ui/views/bookmarks/

Bug: 772945
Change-Id: Ifbdd19b49dc253d188eab3b3dafecb3beaf51f0f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2456151Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Auto-Submit: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#814766}

chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc

@@ -150,9 +150,8 @@ std::unique_ptr<LabelButtonBorder> CreateBookmarkButtonBorder() {
 
 class BookmarkButtonBase : public views::LabelButton {
  public:
-  BookmarkButtonBase(BookmarkBarView::ButtonListener* listener,
-                     const base::string16& title)
-      : LabelButton(listener, title) {
+  BookmarkButtonBase(PressedCallback callback, const base::string16& title)
+      : LabelButton(std::move(callback), title) {
     SetImageLabelSpacing(ChromeLayoutProvider::Get()->GetDistanceMetric(
         DISTANCE_RELATED_LABEL_HORIZONTAL_LIST));
     views::InstallPillHighlightPathGenerator(this);
@@ -213,10 +212,10 @@ class BookmarkButton : public BookmarkButtonBase {
   // The internal view class name.
   static const char kViewClassName[];
 
-  BookmarkButton(BookmarkBarView::ButtonListener* listener,
+  BookmarkButton(PressedCallback callback,
                  const GURL& url,
                  const base::string16& title)
-      : BookmarkButtonBase(listener, title), url_(url) {}
+      : BookmarkButtonBase(std::move(callback), title), url_(url) {}
 
   // views::View:
   base::string16 GetTooltipText(const gfx::Point& p) const override {
@@ -264,9 +263,8 @@ class ShortcutButton : public BookmarkButtonBase {
   // The internal view class name.
   static const char kViewClassName[];
 
-  ShortcutButton(BookmarkBarView::ButtonListener* listener,
-                 const base::string16& title)
-      : BookmarkButtonBase(listener, title) {}
+  ShortcutButton(PressedCallback callback, const base::string16& title)
+      : BookmarkButtonBase(std::move(callback), title) {}
 
   const char* GetClassName() const override { return kViewClassName; }
 
@@ -282,9 +280,10 @@ const char ShortcutButton::kViewClassName[] = "ShortcutButton";
 // Base class for menu hosting buttons used on the bookmark bar.
 class BookmarkMenuButtonBase : public MenuButton {
  public:
-  BookmarkMenuButtonBase(const base::string16& title,
-                         BookmarkBarView::MenuButtonListener* button_listener)
-      : MenuButton(button_listener, title) {
+  explicit BookmarkMenuButtonBase(
+      PressedCallback callback,
+      const base::string16& title = base::string16())
+      : MenuButton(std::move(callback), title) {
     SetImageLabelSpacing(ChromeLayoutProvider::Get()->GetDistanceMetric(
         DISTANCE_RELATED_LABEL_HORIZONTAL_LIST));
     views::InstallPillHighlightPathGenerator(this);
@@ -320,9 +319,9 @@ class BookmarkMenuButtonBase : public MenuButton {
 // button.
 class BookmarkFolderButton : public BookmarkMenuButtonBase {
  public:
-  BookmarkFolderButton(const base::string16& title,
-                       BookmarkBarView::MenuButtonListener* button_listener)
-      : BookmarkMenuButtonBase(title, button_listener) {
+  explicit BookmarkFolderButton(PressedCallback callback,
+                                const base::string16& title = base::string16())
+      : BookmarkMenuButtonBase(std::move(callback), title) {
     show_animation_ = std::make_unique<gfx::SlideAnimation>(this);
     if (!animations_enabled) {
       // For some reason during testing the events generated by animating
@@ -363,10 +362,8 @@ class BookmarkFolderButton : public BookmarkMenuButtonBase {
 
 class OverflowButton : public BookmarkMenuButtonBase {
  public:
-  OverflowButton(BookmarkBarView* owner,
-                 BookmarkBarView::MenuButtonListener* button_listener)
-      : BookmarkMenuButtonBase(base::string16(), button_listener),
-        owner_(owner) {}
+  OverflowButton(PressedCallback callback, BookmarkBarView* owner)
+      : BookmarkMenuButtonBase(std::move(callback)), owner_(owner) {}
 
   bool OnMousePressed(const ui::MouseEvent& e) override {
     owner_->StopThrobbing(true);
@@ -468,23 +465,6 @@ class BookmarkBarView::ButtonSeparatorView : public views::Separator {
 
 // BookmarkBarView ------------------------------------------------------------
 
-BookmarkBarView::ButtonListener::ButtonListener(BookmarkBarView* parent)
-    : parent_(parent) {}
-
-void BookmarkBarView::ButtonListener::ButtonPressed(views::Button* source,
-                                                    const ui::Event& event) {
-  parent_->OnButtonPressed(source, event);
-}
-
-BookmarkBarView::MenuButtonListener::MenuButtonListener(BookmarkBarView* parent)
-    : parent_(parent) {}
-
-void BookmarkBarView::MenuButtonListener::ButtonPressed(
-    views::Button* source,
-    const ui::Event& event) {
-  parent_->OnMenuButtonPressed(source, event);
-}
-
 // static
 const char BookmarkBarView::kViewClassName[] = "BookmarkBarView";
 
@@ -1313,60 +1293,17 @@ bool BookmarkBarView::CanStartDragForView(views::View* sender,
   return true;
 }
 
-void BookmarkBarView::OnMenuButtonPressed(views::Button* view,
-                                          const ui::Event& event) {
-  const BookmarkNode* node;
-
-  size_t start_index = 0;
-  if (view == other_bookmarks_button_) {
-    node = model_->other_node();
-  } else if (view == managed_bookmarks_button_) {
-    node = managed_->managed_node();
-  } else if (view == overflow_button_) {
-    node = model_->bookmark_bar_node();
-    start_index = GetFirstHiddenNodeIndex();
-  } else {
-    size_t button_index = GetIndexForButton(view);
-    DCHECK_NE(size_t{-1}, button_index);
-    node = model_->bookmark_bar_node()->children()[button_index].get();
-  }
-
-  // Clicking the middle mouse button or clicking with Control/Command key down
-  // opens all bookmarks in the folder in new tabs.
-  if ((event.flags() & ui::EF_MIDDLE_MOUSE_BUTTON) ||
-      (event.flags() & ui::EF_PLATFORM_ACCELERATOR)) {
-    WindowOpenDisposition disposition_from_event_flags =
-        ui::DispositionFromEventFlags(event.flags());
-    RecordBookmarkFolderLaunch(BOOKMARK_LAUNCH_LOCATION_ATTACHED_BAR);
-    chrome::OpenAll(GetWidget()->GetNativeWindow(), page_navigator_, node,
-                    disposition_from_event_flags, browser_->profile());
-  } else {
-    RecordBookmarkFolderOpen(BOOKMARK_LAUNCH_LOCATION_ATTACHED_BAR);
-    bookmark_menu_ = new BookmarkMenuController(
-        browser_, page_navigator_, GetWidget(), node, start_index, false);
-    bookmark_menu_->set_observer(this);
-    bookmark_menu_->RunMenuAt(this);
-  }
+void BookmarkBarView::AppsPageShortcutPressed(const ui::Event& event) {
+  content::OpenURLParams params(GURL(chrome::kChromeUIAppsURL),
+                                content::Referrer(),
+                                ui::DispositionFromEventFlags(event.flags()),
+                                ui::PAGE_TRANSITION_AUTO_BOOKMARK, false);
+  page_navigator_->OpenURL(params);
+  RecordBookmarkAppsPageOpen(BOOKMARK_LAUNCH_LOCATION_ATTACHED_BAR);
 }
 
-void BookmarkBarView::OnButtonPressed(views::Button* sender,
+void BookmarkBarView::OnButtonPressed(const bookmarks::BookmarkNode* node,
                                       const ui::Event& event) {
-  WindowOpenDisposition disposition_from_event_flags =
-      ui::DispositionFromEventFlags(event.flags());
-
-  if (sender == apps_page_shortcut_) {
-    content::OpenURLParams params(
-        GURL(chrome::kChromeUIAppsURL), content::Referrer(),
-        disposition_from_event_flags, ui::PAGE_TRANSITION_AUTO_BOOKMARK, false);
-    page_navigator_->OpenURL(params);
-    RecordBookmarkAppsPageOpen(BOOKMARK_LAUNCH_LOCATION_ATTACHED_BAR);
-    return;
-  }
-
-  size_t index = GetIndexForButton(sender);
-  DCHECK_NE(size_t{-1}, index);
-  const BookmarkNode* node =
-      model_->bookmark_bar_node()->children()[index].get();
   DCHECK(page_navigator_);
 
   // Only URL nodes have regular buttons on the bookmarks bar; folder clicks
@@ -1374,7 +1311,7 @@ void BookmarkBarView::OnButtonPressed(views::Button* sender,
   DCHECK(node->is_url());
   RecordAppLaunch(browser_->profile(), node->url());
   content::OpenURLParams params(node->url(), content::Referrer(),
-                                disposition_from_event_flags,
+                                ui::DispositionFromEventFlags(event.flags()),
                                 ui::PAGE_TRANSITION_AUTO_BOOKMARK, false);
   page_navigator_->OpenURL(params);
   RecordBookmarkLaunch(
@@ -1382,6 +1319,27 @@ void BookmarkBarView::OnButtonPressed(views::Button* sender,
       ProfileMetrics::GetBrowserProfileType(browser_->profile()));
 }
 
+void BookmarkBarView::OnMenuButtonPressed(const bookmarks::BookmarkNode* node,
+                                          const ui::Event& event) {
+  // Clicking the middle mouse button or clicking with Control/Command key down
+  // opens all bookmarks in the folder in new tabs.
+  if ((event.flags() & ui::EF_MIDDLE_MOUSE_BUTTON) ||
+      (event.flags() & ui::EF_PLATFORM_ACCELERATOR)) {
+    RecordBookmarkFolderLaunch(BOOKMARK_LAUNCH_LOCATION_ATTACHED_BAR);
+    chrome::OpenAll(GetWidget()->GetNativeWindow(), page_navigator_, node,
+                    ui::DispositionFromEventFlags(event.flags()),
+                    browser_->profile());
+  } else {
+    RecordBookmarkFolderOpen(BOOKMARK_LAUNCH_LOCATION_ATTACHED_BAR);
+    const size_t start_index =
+        (node == model_->bookmark_bar_node()) ? GetFirstHiddenNodeIndex() : 0;
+    bookmark_menu_ = new BookmarkMenuController(
+        browser_, page_navigator_, GetWidget(), node, start_index, false);
+    bookmark_menu_->set_observer(this);
+    bookmark_menu_->RunMenuAt(this);
+  }
+}
+
 void BookmarkBarView::ShowContextMenuForViewImpl(
     views::View* source,
     const gfx::Point& point,
@@ -1492,8 +1450,11 @@ size_t BookmarkBarView::GetFirstHiddenNodeIndex() {
 
 std::unique_ptr<MenuButton> BookmarkBarView::CreateOtherBookmarksButton() {
   // Title is set in Loaded.
-  auto button = std::make_unique<BookmarkFolderButton>(base::string16(),
-                                                       &menu_button_listener_);
+  auto button = std::make_unique<BookmarkFolderButton>(base::BindRepeating(
+      [](BookmarkBarView* bar, const ui::Event& event) {
+        bar->OnMenuButtonPressed(bar->model_->other_node(), event);
+      },
+      base::Unretained(this)));
   button->SetID(VIEW_ID_OTHER_BOOKMARKS);
   button->set_context_menu_controller(this);
   return button;
@@ -1501,15 +1462,24 @@ std::unique_ptr<MenuButton> BookmarkBarView::CreateOtherBookmarksButton() {
 
 std::unique_ptr<MenuButton> BookmarkBarView::CreateManagedBookmarksButton() {
   // Title is set in Loaded.
-  auto button = std::make_unique<BookmarkFolderButton>(base::string16(),
-                                                       &menu_button_listener_);
+  auto button = std::make_unique<BookmarkFolderButton>(base::BindRepeating(
+      [](BookmarkBarView* bar, const ui::Event& event) {
+        bar->OnMenuButtonPressed(bar->managed_->managed_node(), event);
+      },
+      base::Unretained(this)));
   button->SetID(VIEW_ID_MANAGED_BOOKMARKS);
   button->set_context_menu_controller(this);
   return button;
 }
 
 std::unique_ptr<MenuButton> BookmarkBarView::CreateOverflowButton() {
-  auto button = std::make_unique<OverflowButton>(this, &menu_button_listener_);
+  auto button = std::make_unique<OverflowButton>(
+      base::BindRepeating(
+          [](BookmarkBarView* bar, const ui::Event& event) {
+            bar->OnMenuButtonPressed(bar->model_->bookmark_bar_node(), event);
+          },
+          base::Unretained(this)),
+      this);
 
   // The overflow button's image contains an arrow and therefore it is a
   // direction sensitive image and we need to flip it if the UI layout is
@@ -1535,14 +1505,18 @@ std::unique_ptr<views::View> BookmarkBarView::CreateBookmarkButton(
   int index = node->parent()->GetIndexOf(node);
   std::unique_ptr<views::LabelButton> button;
   if (node->is_url()) {
-    button = std::make_unique<BookmarkButton>(&button_listener_, node->url(),
-                                              node->GetTitle());
+    button = std::make_unique<BookmarkButton>(
+        base::BindRepeating(&BookmarkBarView::OnButtonPressed,
+                            base::Unretained(this), node),
+        node->url(), node->GetTitle());
     button->GetViewAccessibility().OverrideDescription(url_formatter::FormatUrl(
         node->url(), url_formatter::kFormatUrlOmitDefaults,
         net::UnescapeRule::SPACES, nullptr, nullptr, nullptr));
   } else {
-    button = std::make_unique<BookmarkFolderButton>(node->GetTitle(),
-                                                    &menu_button_listener_);
+    button = std::make_unique<BookmarkFolderButton>(
+        base::BindRepeating(&BookmarkBarView::OnMenuButtonPressed,
+                            base::Unretained(this), node),
+        node->GetTitle());
     button->GetViewAccessibility().OverrideDescription("");
   }
   ConfigureButton(node, button.get());
@@ -1553,7 +1527,8 @@ std::unique_ptr<views::View> BookmarkBarView::CreateBookmarkButton(
 std::unique_ptr<views::LabelButton>
 BookmarkBarView::CreateAppsPageShortcutButton() {
   auto button = std::make_unique<ShortcutButton>(
-      &button_listener_,
+      base::BindRepeating(&BookmarkBarView::AppsPageShortcutPressed,
+                          base::Unretained(this)),
       l10n_util::GetStringUTF16(IDS_BOOKMARK_BAR_APPS_SHORTCUT_NAME));
   button->SetTooltipText(
       l10n_util::GetStringUTF16(IDS_BOOKMARK_BAR_APPS_SHORTCUT_TOOLTIP));

chrome/browser/ui/views/bookmarks/bookmark_bar_view.h

@@ -23,7 +23,6 @@
 #include "ui/views/accessible_pane_view.h"
 #include "ui/views/animation/animation_delegate_views.h"
 #include "ui/views/context_menu_controller.h"
-#include "ui/views/controls/button/button.h"
 #include "ui/views/controls/menu/menu_types.h"
 #include "ui/views/drag_controller.h"
 
@@ -70,30 +69,6 @@ class BookmarkBarView : public views::AccessiblePaneView,
                         public BookmarkMenuControllerObserver,
                         public bookmarks::BookmarkBubbleObserver {
  public:
-  // TODO(pbos): Get rid of these proxy classes by unifying a single
-  // ButtonPressed to handle all buttons. This class only exists to forward
-  // events into ::OnButtonPressed.
-  class ButtonListener : public views::ButtonListener {
-   public:
-    explicit ButtonListener(BookmarkBarView* parent);
-    void ButtonPressed(views::Button* source, const ui::Event& event) override;
-
-   private:
-    BookmarkBarView* const parent_;
-  };
-
-  // TODO(pbos): Get rid of these proxy classes by unifying a single
-  // ButtonPressed to handle all buttons. This class only exists to forward
-  // events into ::OnMenuButtonPressed.
-  class MenuButtonListener : public views::ButtonListener {
-   public:
-    explicit MenuButtonListener(BookmarkBarView* parent);
-    void ButtonPressed(views::Button* source, const ui::Event& event) override;
-
-   private:
-    BookmarkBarView* const parent_;
-  };
-
   // The internal view class name.
   static const char kViewClassName[];
 
@@ -266,8 +241,11 @@ class BookmarkBarView : public views::AccessiblePaneView,
   // calculating the preferred height.
   void Init();
 
-  void OnButtonPressed(views::Button* sender, const ui::Event& event);
-  void OnMenuButtonPressed(views::Button* sender, const ui::Event& event);
+  void AppsPageShortcutPressed(const ui::Event& event);
+  void OnButtonPressed(const bookmarks::BookmarkNode* node,
+                       const ui::Event& event);
+  void OnMenuButtonPressed(const bookmarks::BookmarkNode* node,
+                           const ui::Event& event);
 
   // NOTE: unless otherwise stated all methods that take an index are in terms
   // of the bookmark bar view. Typically the view index and model index are the
@@ -384,10 +362,6 @@ class BookmarkBarView : public views::AccessiblePaneView,
   // or size_t{-1} if |button| is not a bookmark button from this bar.
   size_t GetIndexForButton(views::View* button);
 
-  // These forward button callbacks into ::On{Menu}ButtonPressed.
-  ButtonListener button_listener_{this};
-  MenuButtonListener menu_button_listener_{this};
-
   // Needed to react to kShowAppsShortcutInBookmarkBar changes.
   PrefChangeRegistrar profile_pref_registrar_;
 

chrome/browser/ui/views/bookmarks/bookmark_editor_view.cc

@@ -44,20 +44,6 @@ using bookmarks::BookmarkExpandedStateTracker;
 using bookmarks::BookmarkModel;
 using bookmarks::BookmarkNode;
 
-namespace {
-
-std::unique_ptr<views::LabelButton> CreateNewFolderButton(
-    views::ButtonListener* listener,
-    bool enabled) {
-  auto new_folder_button = std::make_unique<views::MdTextButton>(
-      listener,
-      l10n_util::GetStringUTF16(IDS_BOOKMARK_EDITOR_NEW_FOLDER_BUTTON));
-  new_folder_button->SetEnabled(enabled);
-  return new_folder_button;
-}
-
-}  // namespace
-
 BookmarkEditorView::BookmarkEditorView(
     Profile* profile,
     const BookmarkNode* parent,
@@ -79,8 +65,11 @@ BookmarkEditorView::BookmarkEditorView(
   SetTitle(details_.GetWindowTitleId());
   SetButtonLabel(ui::DIALOG_BUTTON_OK, l10n_util::GetStringUTF16(IDS_SAVE));
   if (show_tree_) {
-    new_folder_button_ =
-        SetExtraView(CreateNewFolderButton(this, bb_model_->loaded()));
+    new_folder_button_ = SetExtraView(std::make_unique<views::MdTextButton>(
+        base::BindRepeating(&BookmarkEditorView::NewFolderButtonPressed,
+                            base::Unretained(this)),
+        l10n_util::GetStringUTF16(IDS_BOOKMARK_EDITOR_NEW_FOLDER_BUTTON)));
+    new_folder_button_->SetEnabled(bb_model_->loaded());
   }
   set_margins(ChromeLayoutProvider::Get()->GetDialogInsetsForContentType(
       views::CONTROL, views::CONTROL));
@@ -142,13 +131,6 @@ void BookmarkEditorView::GetAccessibleNodeData(ui::AXNodeData* node_data) {
   node_data->SetName(l10n_util::GetStringUTF8(IDS_BOOKMARK_EDITOR_TITLE));
 }
 
-void BookmarkEditorView::ButtonPressed(views::Button* sender,
-                                       const ui::Event& event) {
-  DCHECK_EQ(new_folder_button_, sender);
-  DCHECK(tree_view_->GetSelectedNode());
-  NewFolder(tree_model_->AsNode(tree_view_->GetSelectedNode()));
-}
-
 bool BookmarkEditorView::IsCommandIdChecked(int command_id) const {
   return false;
 }
@@ -422,13 +404,14 @@ void BookmarkEditorView::UserInputChanged() {
   DialogModelChanged();
 }
 
+void BookmarkEditorView::NewFolderButtonPressed() {
+  DCHECK(tree_view_->GetSelectedNode());
+  NewFolder(tree_model_->AsNode(tree_view_->GetSelectedNode()));
+}
+
 void BookmarkEditorView::NewFolder(EditorNode* parent) {
   // Create a new entry parented to the given item.
-  if (!parent) {
-    NOTREACHED();
-    return;
-  }
-
+  DCHECK(parent);
   tree_view_->StartEditing(AddNewFolder(parent));
 }
 

chrome/browser/ui/views/bookmarks/bookmark_editor_view.h

@@ -19,7 +19,6 @@
 #include "ui/base/models/simple_menu_model.h"
 #include "ui/base/models/tree_node_model.h"
 #include "ui/views/context_menu_controller.h"
-#include "ui/views/controls/button/button.h"
 #include "ui/views/controls/textfield/textfield.h"
 #include "ui/views/controls/textfield/textfield_controller.h"
 #include "ui/views/controls/tree/tree_view_controller.h"
@@ -46,7 +45,6 @@ class Profile;
 // To use BookmarkEditorView invoke the static show method.
 
 class BookmarkEditorView : public BookmarkEditor,
-                           public views::ButtonListener,
                            public views::TreeViewController,
                            public views::DialogDelegateView,
                            public views::TextfieldController,
@@ -95,9 +93,6 @@ class BookmarkEditorView : public BookmarkEditor,
   bool HandleKeyEvent(views::Textfield* sender,
                       const ui::KeyEvent& key_event) override;
 
-  // views::ButtonListener:
-  void ButtonPressed(views::Button* sender, const ui::Event& event) override;
-
   // ui::SimpleMenuModel::Delegate:
   bool IsCommandIdChecked(int command_id) const override;
   bool IsCommandIdEnabled(int command_id) const override;
@@ -198,6 +193,8 @@ class BookmarkEditorView : public BookmarkEditor,
   // of Textfields and ok button appropriately.
   void UserInputChanged();
 
+  void NewFolderButtonPressed();
+
   // Creates a new folder as a child of the given node. Starts editing on the
   // new group as well.
   void NewFolder(EditorNode* parent);