[omnibox] Move more logic to OmniboxSuggestionRowButton

Moved logic out of the OmniboxSuggestionButtonRowView to the button
class. Button row class had some duplicated code to handle each button
(creating focus ring, updating icon on theme change). It makes more
sense to encapsulate logic related to button visual representation in
the button class.

Bug: 1104264
Change-Id: Ie55b9488a7300cfd36bb7a52ef89f8f659b01613
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2326170
Commit-Queue: Olesia Marukhno <olesiamarukhno@google.com>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Reviewed-by: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#794985}

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

@@ -29,10 +29,21 @@
 class OmniboxSuggestionRowButton : public views::MdTextButton {
  public:
   OmniboxSuggestionRowButton(views::ButtonListener* listener,
-                             const base::string16& text)
-      : MdTextButton(listener, CONTEXT_OMNIBOX_PRIMARY) {
+                             const base::string16& text,
+                             const gfx::VectorIcon& icon,
+                             const views::FocusRing::ViewPredicate& predicate)
+      : MdTextButton(listener, CONTEXT_OMNIBOX_PRIMARY), icon_(icon) {
     SetText(text);
+    views::InstallPillHighlightPathGenerator(this);
+    SetImageLabelSpacing(ChromeLayoutProvider::Get()->GetDistanceMetric(
+        DISTANCE_RELATED_LABEL_HORIZONTAL_LIST));
+    SetCustomPadding(ChromeLayoutProvider::Get()->GetInsetsMetric(
+        INSETS_OMNIBOX_PILL_BUTTON));
+    SetCornerRadius(GetInsets().height() +
+                    GetLayoutConstant(LOCATION_BAR_ICON_SIZE));
+
     set_ink_drop_highlight_opacity(CalculateInkDropHighlightOpacity());
+    focus_ring()->SetHasFocusPredicate(predicate);
   }
 
   OmniboxSuggestionRowButton(const OmniboxSuggestionRowButton&) = delete;
@@ -45,6 +56,8 @@ class OmniboxSuggestionRowButton : public views::MdTextButton {
     return color_utils::GetColorWithMaxContrast(background()->get_color());
   }
 
+  void OnStyleRefresh() { focus_ring()->SchedulePaint(); }
+
   std::unique_ptr<views::InkDropHighlight> CreateInkDropHighlight()
       const override {
     // MdTextButton uses custom colors when creating ink drop highlight.
@@ -53,7 +66,19 @@ class OmniboxSuggestionRowButton : public views::MdTextButton {
     return views::InkDropHostView::CreateInkDropHighlight();
   }
 
+  void OnThemeChanged() override {
+    MdTextButton::OnThemeChanged();
+    SkColor color =
+        GetOmniboxColor(GetThemeProvider(), OmniboxPart::RESULTS_ICON,
+                        OmniboxPartState::NORMAL);
+    SetImage(views::Button::STATE_NORMAL,
+             gfx::CreateVectorIcon(
+                 icon_, GetLayoutConstant(LOCATION_BAR_ICON_SIZE), color));
+  }
+
  private:
+  const gfx::VectorIcon& icon_;
+
   float CalculateInkDropHighlightOpacity() {
     // Ink drop highlight opacity is result of mixing a layer with hovered
     // opacity and a layer with selected opacity. OmniboxPartState::SELECTED
@@ -70,19 +95,13 @@ namespace {
 
 OmniboxSuggestionRowButton* CreatePillButton(
     OmniboxSuggestionButtonRowView* button_row,
-    const char* message) {
+    const char* message,
+    const gfx::VectorIcon& icon,
+    const views::FocusRing::ViewPredicate& predicate) {
   OmniboxSuggestionRowButton* button =
       button_row->AddChildView(std::make_unique<OmniboxSuggestionRowButton>(
-          button_row, base::ASCIIToUTF16(message)));
+          button_row, base::ASCIIToUTF16(message), icon, predicate));
   button->SetVisible(false);
-  button->SetImageLabelSpacing(ChromeLayoutProvider::Get()->GetDistanceMetric(
-      DISTANCE_RELATED_LABEL_HORIZONTAL_LIST));
-  button->SetCustomPadding(
-      ChromeLayoutProvider::Get()->GetInsetsMetric(INSETS_OMNIBOX_PILL_BUTTON));
-  button->SetCornerRadius(button->GetInsets().height() +
-                          GetLayoutConstant(LOCATION_BAR_ICON_SIZE));
-  views::HighlightPathGenerator::Install(
-      button, std::make_unique<views::PillHighlightPathGenerator>());
   return button;
 }
 
@@ -105,11 +124,6 @@ OmniboxSuggestionButtonRowView::OmniboxSuggestionButtonRowView(
           gfx::Insets(0, ChromeLayoutProvider::Get()->GetDistanceMetric(
                              views::DISTANCE_RELATED_BUTTON_HORIZONTAL)));
 
-  // TODO(orinj): Use the real translated string table values here instead.
-  keyword_button_ = CreatePillButton(this, "Keyword search");
-  pedal_button_ = CreatePillButton(this, "Pedal");
-  tab_switch_button_ = CreatePillButton(this, "Switch to this tab");
-
   const auto make_predicate = [=](auto state) {
     return [=](View* view) {
       return view->GetVisible() &&
@@ -117,14 +131,16 @@ OmniboxSuggestionButtonRowView::OmniboxSuggestionButtonRowView(
                  OmniboxPopupModel::Selection(model_index_, state);
     };
   };
-  keyword_button_focus_ring_ = views::FocusRing::Install(keyword_button_);
-  keyword_button_focus_ring_->SetHasFocusPredicate(
+
+  // TODO(orinj): Use the real translated string table values here instead.
+  keyword_button_ = CreatePillButton(
+      this, "Keyword search", vector_icons::kSearchIcon,
       make_predicate(OmniboxPopupModel::FOCUSED_BUTTON_KEYWORD));
-  pedal_button_focus_ring_ = views::FocusRing::Install(pedal_button_);
-  pedal_button_focus_ring_->SetHasFocusPredicate(
-      make_predicate(OmniboxPopupModel::FOCUSED_BUTTON_PEDAL));
-  tab_switch_button_focus_ring_ = views::FocusRing::Install(tab_switch_button_);
-  tab_switch_button_focus_ring_->SetHasFocusPredicate(
+  pedal_button_ =
+      CreatePillButton(this, "Pedal", omnibox::kProductIcon,
+                       make_predicate(OmniboxPopupModel::FOCUSED_BUTTON_PEDAL));
+  tab_switch_button_ = CreatePillButton(
+      this, "Switch to this tab", omnibox::kSwitchIcon,
       make_predicate(OmniboxPopupModel::FOCUSED_BUTTON_TAB_SWITCH));
 }
 
@@ -163,26 +179,9 @@ void OmniboxSuggestionButtonRowView::UpdateFromModel() {
 }
 
 void OmniboxSuggestionButtonRowView::OnStyleRefresh() {
-  keyword_button_focus_ring_->SchedulePaint();
-  pedal_button_focus_ring_->SchedulePaint();
-  tab_switch_button_focus_ring_->SchedulePaint();
-}
-
-void OmniboxSuggestionButtonRowView::OnThemeChanged() {
-  View::OnThemeChanged();
-  SkColor color = GetOmniboxColor(GetThemeProvider(), OmniboxPart::RESULTS_ICON,
-                                  OmniboxPartState::NORMAL);
-  const int icon_size = GetLayoutConstant(LOCATION_BAR_ICON_SIZE);
-
-  keyword_button_->SetImage(
-      views::Button::STATE_NORMAL,
-      gfx::CreateVectorIcon(vector_icons::kSearchIcon, icon_size, color));
-  pedal_button_->SetImage(
-      views::Button::STATE_NORMAL,
-      gfx::CreateVectorIcon(omnibox::kProductIcon, icon_size, color));
-  tab_switch_button_->SetImage(
-      views::Button::STATE_NORMAL,
-      gfx::CreateVectorIcon(omnibox::kSwitchIcon, icon_size, color));
+  keyword_button_->OnStyleRefresh();
+  pedal_button_->OnStyleRefresh();
+  tab_switch_button_->OnStyleRefresh();
 }
 
 void OmniboxSuggestionButtonRowView::ButtonPressed(views::Button* button,
@@ -229,7 +228,7 @@ const AutocompleteMatch& OmniboxSuggestionButtonRowView::match() const {
 }
 
 void OmniboxSuggestionButtonRowView::SetPillButtonVisibility(
-    views::MdTextButton* button,
+    OmniboxSuggestionRowButton* button,
     OmniboxPopupModel::LineState state) {
   button->SetVisible(model()->IsControlPresentOnMatch(
       OmniboxPopupModel::Selection(model_index_, state)));

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

@@ -32,9 +32,6 @@ class OmniboxSuggestionButtonRowView : public views::View,
   // views::ButtonListener:
   void ButtonPressed(views::Button* sender, const ui::Event& event) override;
 
-  // views::View:
-  void OnThemeChanged() override;
-
  private:
   // Get the popup model from the view.
   const OmniboxPopupModel* model() const;
@@ -42,7 +39,7 @@ class OmniboxSuggestionButtonRowView : public views::View,
   // Digs into the model with index to get the match for owning result view.
   const AutocompleteMatch& match() const;
 
-  void SetPillButtonVisibility(views::MdTextButton* button,
+  void SetPillButtonVisibility(OmniboxSuggestionRowButton* button,
                                OmniboxPopupModel::LineState state);
 
   OmniboxPopupContentsView* const popup_contents_view_;
@@ -51,9 +48,6 @@ class OmniboxSuggestionButtonRowView : public views::View,
   OmniboxSuggestionRowButton* keyword_button_ = nullptr;
   OmniboxSuggestionRowButton* pedal_button_ = nullptr;
   OmniboxSuggestionRowButton* tab_switch_button_ = nullptr;
-  views::FocusRing* keyword_button_focus_ring_ = nullptr;
-  views::FocusRing* pedal_button_focus_ring_ = nullptr;
-  views::FocusRing* tab_switch_button_focus_ring_ = nullptr;
 
   DISALLOW_COPY_AND_ASSIGN(OmniboxSuggestionButtonRowView);
 };