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

Bug: 772945
Change-Id: I680e763fa212bbceaded6b337c29323f59bae9ea
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2439439
Auto-Submit: Peter Kasting <pkasting@chromium.org>
Reviewed-by: manuk hovanesian <manukh@chromium.org>
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#812036}

chrome/browser/ui/views/omnibox/omnibox_result_view.cc

@@ -38,6 +38,7 @@
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/events/event.h"
 #include "ui/gfx/paint_vector_icon.h"
+#include "ui/views/controls/button/image_button.h"
 #include "ui/views/controls/button/image_button_factory.h"
 #include "ui/views/controls/focus_ring.h"
 #include "ui/views/controls/highlight_path_generator.h"
@@ -50,8 +51,8 @@ namespace {
 
 class OmniboxRemoveSuggestionButton : public views::ImageButton {
  public:
-  explicit OmniboxRemoveSuggestionButton(views::ButtonListener* listener)
-      : ImageButton(listener) {
+  explicit OmniboxRemoveSuggestionButton(PressedCallback callback)
+      : ImageButton(std::move(callback)) {
     views::ConfigureVectorImageButton(this);
   }
 
@@ -86,6 +87,9 @@ OmniboxResultView::OmniboxResultView(
 
   suggestion_tab_switch_button_ =
       AddChildView(std::make_unique<OmniboxTabSwitchButton>(
+          base::BindRepeating(&OmniboxResultView::ButtonPressed,
+                              base::Unretained(this),
+                              OmniboxPopupModel::FOCUSED_BUTTON_TAB_SWITCH),
           popup_contents_view_, this,
           l10n_util::GetStringUTF16(IDS_OMNIBOX_TAB_SUGGEST_HINT),
           l10n_util::GetStringUTF16(IDS_OMNIBOX_TAB_SUGGEST_SHORT_HINT),
@@ -96,8 +100,10 @@ OmniboxResultView::OmniboxResultView(
   // priority button, which already has a Shift+Delete shortcut.
   // TODO(tommycli): Make sure we announce the Shift+Delete capability in the
   // accessibility node data for removable suggestions.
-  remove_suggestion_button_ =
-      AddChildView(std::make_unique<OmniboxRemoveSuggestionButton>(this));
+  remove_suggestion_button_ = AddChildView(
+      std::make_unique<OmniboxRemoveSuggestionButton>(base::BindRepeating(
+          &OmniboxResultView::ButtonPressed, base::Unretained(this),
+          OmniboxPopupModel::FOCUSED_BUTTON_REMOVE_SUGGESTION)));
   mouse_enter_exit_handler_.ObserveMouseEnterExitOn(remove_suggestion_button_);
   views::InstallCircleHighlightPathGenerator(remove_suggestion_button_);
   remove_suggestion_button_->SetTooltipText(
@@ -332,23 +338,10 @@ void OmniboxResultView::SetRichSuggestionImage(const gfx::ImageSkia& image) {
   suggestion_view_->SetImage(image);
 }
 
-////////////////////////////////////////////////////////////////////////////////
-// views::ButtonListener overrides:
-
-void OmniboxResultView::ButtonPressed(views::Button* button,
+void OmniboxResultView::ButtonPressed(OmniboxPopupModel::LineState state,
                                       const ui::Event& event) {
-  if (button == suggestion_tab_switch_button_) {
-    popup_contents_view_->model()->TriggerSelectionAction(
-        OmniboxPopupModel::Selection(
-            model_index_, OmniboxPopupModel::FOCUSED_BUTTON_TAB_SWITCH),
-        event.time_stamp());
-  } else if (button == remove_suggestion_button_) {
-    popup_contents_view_->model()->TriggerSelectionAction(
-        OmniboxPopupModel::Selection(
-            model_index_, OmniboxPopupModel::FOCUSED_BUTTON_REMOVE_SUGGESTION));
-  } else {
-    NOTREACHED();
-  }
+  popup_contents_view_->model()->TriggerSelectionAction(
+      OmniboxPopupModel::Selection(model_index_, state), event.time_stamp());
 }
 
 ////////////////////////////////////////////////////////////////////////////////

chrome/browser/ui/views/omnibox/omnibox_result_view.h

@@ -13,6 +13,7 @@
 #include "base/memory/weak_ptr.h"
 #include "chrome/browser/ui/views/omnibox/omnibox_mouse_enter_exit_handler.h"
 #include "components/omnibox/browser/autocomplete_match.h"
+#include "components/omnibox/browser/omnibox_popup_model.h"
 #include "components/omnibox/browser/suggestion_answer.h"
 #include "third_party/skia/include/core/SkColor.h"
 #include "ui/base/window_open_disposition.h"
@@ -21,8 +22,6 @@
 #include "ui/gfx/geometry/rect.h"
 #include "ui/views/animation/animation_delegate_views.h"
 #include "ui/views/background.h"
-#include "ui/views/controls/button/button.h"
-#include "ui/views/controls/button/image_button.h"
 #include "ui/views/controls/image_view.h"
 #include "ui/views/view.h"
 
@@ -40,11 +39,11 @@ class Image;
 namespace views {
 class Button;
 class FocusRing;
+class ImageButton;
 }  // namespace views
 
 class OmniboxResultView : public views::View,
-                          public views::AnimationDelegateViews,
-                          public views::ButtonListener {
+                          public views::AnimationDelegateViews {
  public:
   OmniboxResultView(OmniboxPopupContentsView* popup_contents_view,
                     size_t model_index);
@@ -89,8 +88,8 @@ class OmniboxResultView : public views::View,
   // Stores the image in a local data member and schedules a repaint.
   void SetRichSuggestionImage(const gfx::ImageSkia& image);
 
-  // views::ButtonListener:
-  void ButtonPressed(views::Button* sender, const ui::Event& event) override;
+  void ButtonPressed(OmniboxPopupModel::LineState state,
+                     const ui::Event& event);
 
   // Helper to emit accessibility events (may only emit if conditions are met).
   void EmitTextChangedAccessiblityEvent();

chrome/browser/ui/views/omnibox/omnibox_row_view.cc

@@ -33,8 +33,7 @@
 #include "ui/views/layout/box_layout.h"
 #include "ui/views/style/typography.h"
 
-class OmniboxRowView::HeaderView : public views::View,
-                                   public views::ButtonListener {
+class OmniboxRowView::HeaderView : public views::View {
  public:
   explicit HeaderView(OmniboxRowView* row_view)
       : row_view_(row_view),
@@ -57,7 +56,8 @@ class OmniboxRowView::HeaderView : public views::View,
     header_label_->SetFontList(font);
 
     header_toggle_button_ =
-        AddChildView(views::CreateVectorToggleImageButton(this));
+        AddChildView(views::CreateVectorToggleImageButton(base::BindRepeating(
+            &HeaderView::HeaderToggleButtonPressed, base::Unretained(this))));
     mouse_enter_exit_handler_.ObserveMouseEnterExitOn(header_toggle_button_);
     views::InstallCircleHighlightPathGenerator(header_toggle_button_);
 
@@ -138,13 +138,6 @@ class OmniboxRowView::HeaderView : public views::View,
                                                  : ax::mojom::State::kExpanded);
   }
 
-  // views::ButtonListener:
-  void ButtonPressed(views::Button* sender, const ui::Event& event) override {
-    DCHECK_EQ(sender, header_toggle_button_);
-    row_view_->popup_model_->TriggerSelectionAction(HeaderSelection());
-    // The PrefChangeRegistrar will update the actual button toggle state.
-  }
-
   // Updates the UI state for the new hover or selection state.
   void UpdateUI() {
     OmniboxPartState part_state = OmniboxPartState::NORMAL;
@@ -197,6 +190,11 @@ class OmniboxRowView::HeaderView : public views::View,
   views::Button* header_toggle_button() const { return header_toggle_button_; }
 
  private:
+  void HeaderToggleButtonPressed() {
+    row_view_->popup_model_->TriggerSelectionAction(HeaderSelection());
+    // The PrefChangeRegistrar will update the actual button toggle state.
+  }
+
   // Updates the hide button's toggle state.
   void OnPrefChanged() {
     DCHECK(row_view_->pref_service_);

chrome/browser/ui/views/omnibox/omnibox_suggestion_button_row_view.cc

@@ -25,6 +25,7 @@
 #include "ui/views/animation/ink_drop_highlight.h"
 #include "ui/views/background.h"
 #include "ui/views/controls/button/label_button_border.h"
+#include "ui/views/controls/button/md_text_button.h"
 #include "ui/views/controls/focus_ring.h"
 #include "ui/views/controls/highlight_path_generator.h"
 #include "ui/views/layout/flex_layout.h"
@@ -32,12 +33,12 @@
 
 class OmniboxSuggestionRowButton : public views::MdTextButton {
  public:
-  OmniboxSuggestionRowButton(views::ButtonListener* listener,
+  OmniboxSuggestionRowButton(PressedCallback callback,
                              const base::string16& text,
                              const gfx::VectorIcon& icon,
                              OmniboxPopupContentsView* popup_contents_view,
                              OmniboxPopupModel::Selection selection)
-      : MdTextButton(listener, text, CONTEXT_OMNIBOX_PRIMARY),
+      : MdTextButton(std::move(callback), text, CONTEXT_OMNIBOX_PRIMARY),
         icon_(icon),
         popup_contents_view_(popup_contents_view),
         selection_(selection) {
@@ -135,19 +136,28 @@ OmniboxSuggestionButtonRowView::OmniboxSuggestionButtonRowView(
   // it's called directly from OmniboxResultView::SetMatch(). If this flickers,
   // then so does everything else in the result view.
   keyword_button_ = AddChildView(std::make_unique<OmniboxSuggestionRowButton>(
-      this, base::string16(), vector_icons::kSearchIcon, popup_contents_view_,
+      base::BindRepeating(&OmniboxSuggestionButtonRowView::ButtonPressed,
+                          base::Unretained(this),
+                          OmniboxPopupModel::KEYWORD_MODE),
+      base::string16(), vector_icons::kSearchIcon, popup_contents_view_,
       OmniboxPopupModel::Selection(model_index_,
                                    OmniboxPopupModel::KEYWORD_MODE)));
   tab_switch_button_ =
       AddChildView(std::make_unique<OmniboxSuggestionRowButton>(
-          this, l10n_util::GetStringUTF16(IDS_OMNIBOX_TAB_SUGGEST_HINT),
+          base::BindRepeating(&OmniboxSuggestionButtonRowView::ButtonPressed,
+                              base::Unretained(this),
+                              OmniboxPopupModel::FOCUSED_BUTTON_TAB_SWITCH),
+          l10n_util::GetStringUTF16(IDS_OMNIBOX_TAB_SUGGEST_HINT),
           omnibox::kSwitchIcon, popup_contents_view_,
           OmniboxPopupModel::Selection(
               model_index_, OmniboxPopupModel::FOCUSED_BUTTON_TAB_SWITCH)));
   tab_switch_button_->SetAccessibleName(
       l10n_util::GetStringUTF16(IDS_ACC_TAB_SWITCH_BUTTON));
   pedal_button_ = AddChildView(std::make_unique<OmniboxSuggestionRowButton>(
-      this, base::string16(), omnibox::kProductIcon, popup_contents_view_,
+      base::BindRepeating(&OmniboxSuggestionButtonRowView::ButtonPressed,
+                          base::Unretained(this),
+                          OmniboxPopupModel::FOCUSED_BUTTON_PEDAL),
+      base::string16(), omnibox::kProductIcon, popup_contents_view_,
       OmniboxPopupModel::Selection(model_index_,
                                    OmniboxPopupModel::FOCUSED_BUTTON_PEDAL)));
 }
@@ -194,45 +204,6 @@ void OmniboxSuggestionButtonRowView::OnStyleRefresh() {
   tab_switch_button_->OnStyleRefresh();
 }
 
-void OmniboxSuggestionButtonRowView::ButtonPressed(views::Button* button,
-                                                   const ui::Event& event) {
-  OmniboxPopupModel* popup_model = popup_contents_view_->model();
-  if (!popup_model)
-    return;
-
-  if (button == tab_switch_button_) {
-    popup_model->TriggerSelectionAction(
-        OmniboxPopupModel::Selection(
-            model_index_, OmniboxPopupModel::FOCUSED_BUTTON_TAB_SWITCH),
-        event.time_stamp());
-  } else if (button == keyword_button_) {
-    // TODO(yoangela): Port to PopupModel and merge with keyEvent
-    // TODO(orinj): Clear out existing suggestions, particularly this one, as
-    // once we AcceptKeyword, we are really in a new scope state and holding
-    // onto old suggestions is confusing and error prone. Without this check,
-    // a second click of the button violates assumptions in |AcceptKeyword|.
-    // Note: Since keyword mode logic depends on state of the edit model, the
-    // selection must first be set to prepare for keyword mode before accepting.
-    popup_model->SetSelection(OmniboxPopupModel::Selection(
-        model_index_, OmniboxPopupModel::KEYWORD_MODE));
-    if (model()->edit_model()->is_keyword_hint()) {
-      auto method = metrics::OmniboxEventProto::INVALID;
-      if (event.IsMouseEvent()) {
-        method = metrics::OmniboxEventProto::CLICK_HINT_VIEW;
-      } else if (event.IsGestureEvent()) {
-        method = metrics::OmniboxEventProto::TAP_HINT_VIEW;
-      }
-      DCHECK_NE(method, metrics::OmniboxEventProto::INVALID);
-      model()->edit_model()->AcceptKeyword(method);
-    }
-  } else if (button == pedal_button_) {
-    popup_model->TriggerSelectionAction(
-        OmniboxPopupModel::Selection(model_index_,
-                                     OmniboxPopupModel::FOCUSED_BUTTON_PEDAL),
-        event.time_stamp());
-  }
-}
-
 views::Button* OmniboxSuggestionButtonRowView::GetActiveButton() const {
   std::vector<OmniboxSuggestionRowButton*> visible_buttons;
   if (keyword_button_->GetVisible())
@@ -276,3 +247,28 @@ void OmniboxSuggestionButtonRowView::SetPillButtonVisibility(
         OmniboxPopupModel::Selection(model_index_, state)));
   }
 }
+
+void OmniboxSuggestionButtonRowView::ButtonPressed(
+    OmniboxPopupModel::LineState state,
+    const ui::Event& event) {
+  const OmniboxPopupModel::Selection selection(model_index_, state);
+  if (state == OmniboxPopupModel::KEYWORD_MODE) {
+    // TODO(yoangela): Port to PopupModel and merge with keyEvent
+    // TODO(orinj): Clear out existing suggestions, particularly this one, as
+    // once we AcceptKeyword, we are really in a new scope state and holding
+    // onto old suggestions is confusing and error prone. Without this check,
+    // a second click of the button violates assumptions in |AcceptKeyword|.
+    // Note: Since keyword mode logic depends on state of the edit model, the
+    // selection must first be set to prepare for keyword mode before accepting.
+    popup_contents_view_->model()->SetSelection(selection);
+    if (model()->edit_model()->is_keyword_hint()) {
+      const auto entry_method =
+          event.IsMouseEvent() ? metrics::OmniboxEventProto::CLICK_HINT_VIEW
+                               : metrics::OmniboxEventProto::TAP_HINT_VIEW;
+      model()->edit_model()->AcceptKeyword(entry_method);
+    }
+  } else {
+    popup_contents_view_->model()->TriggerSelectionAction(selection,
+                                                          event.time_stamp());
+  }
+}

chrome/browser/ui/views/omnibox/omnibox_suggestion_button_row_view.h

@@ -8,16 +8,17 @@
 #include "base/macros.h"
 #include "components/omnibox/browser/autocomplete_match.h"
 #include "components/omnibox/browser/omnibox_popup_model.h"
-#include "ui/views/controls/button/button.h"
-#include "ui/views/controls/button/md_text_button.h"
 #include "ui/views/view.h"
 
 class OmniboxPopupContentsView;
 class OmniboxSuggestionRowButton;
 
+namespace views {
+class Button;
+}
+
 // A view to contain the button row within a result view.
-class OmniboxSuggestionButtonRowView : public views::View,
-                                       public views::ButtonListener {
+class OmniboxSuggestionButtonRowView : public views::View {
  public:
   explicit OmniboxSuggestionButtonRowView(OmniboxPopupContentsView* view,
                                           int model_index);
@@ -29,9 +30,6 @@ class OmniboxSuggestionButtonRowView : public views::View,
   // Updates the suggestion row buttons based on the model.
   void UpdateFromModel();
 
-  // views::ButtonListener:
-  void ButtonPressed(views::Button* sender, const ui::Event& event) override;
-
   views::Button* GetActiveButton() const;
 
  private:
@@ -44,6 +42,9 @@ class OmniboxSuggestionButtonRowView : public views::View,
   void SetPillButtonVisibility(OmniboxSuggestionRowButton* button,
                                OmniboxPopupModel::LineState state);
 
+  void ButtonPressed(OmniboxPopupModel::LineState state,
+                     const ui::Event& event);
+
   OmniboxPopupContentsView* const popup_contents_view_;
   size_t const model_index_;
 

chrome/browser/ui/views/omnibox/omnibox_tab_switch_button.cc

@@ -29,12 +29,13 @@ int OmniboxTabSwitchButton::short_text_width_;
 int OmniboxTabSwitchButton::full_text_width_;
 
 OmniboxTabSwitchButton::OmniboxTabSwitchButton(
+    PressedCallback callback,
     OmniboxPopupContentsView* popup_contents_view,
     OmniboxResultView* result_view,
     const base::string16& hint,
     const base::string16& hint_short,
     const gfx::VectorIcon& icon)
-    : MdTextButton(result_view,
+    : MdTextButton(std::move(callback),
                    base::string16(),
                    views::style::CONTEXT_BUTTON_MD),
       popup_contents_view_(popup_contents_view),

chrome/browser/ui/views/omnibox/omnibox_tab_switch_button.h

@@ -12,7 +12,8 @@ class OmniboxResultView;
 
 class OmniboxTabSwitchButton : public views::MdTextButton {
  public:
-  OmniboxTabSwitchButton(OmniboxPopupContentsView* popup_contents_view,
+  OmniboxTabSwitchButton(PressedCallback callback,
+                         OmniboxPopupContentsView* popup_contents_view,
                          OmniboxResultView* result_view,
                          const base::string16& hint,
                          const base::string16& hint_short,