[omnibox] Fix tab to focus Suggestion Removal button, and add test.

The Tab to Focus Suggestion Removal button was regressed in this CL, which
I reviewed:
https://chromium-review.googlesource.com/c/chromium/src/+/2131021

The above CL is a great refactoring CL, which we don't want to revert.
It just broke a piece of functionality.

This CL fixes the functionality and adds a test to prevent regressions.

We will need to backmerge this into M83.

Bug: 1071542
Change-Id: Ib458c1404deccfb85f06015ccb2cae751db9744f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2153323
Auto-Submit: Tommy Li <tommycli@chromium.org>
Reviewed-by: manuk hovanesian <manukh@chromium.org>
Commit-Queue: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#759829}

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

@@ -687,29 +687,6 @@ bool OmniboxViewViews::TextAndUIDirectionMatch() const {
           base::i18n::RIGHT_TO_LEFT) == base::i18n::IsRTL();
 }
 
-views::Button* OmniboxViewViews::GetSecondaryButtonForSelectedLine() const {
-  // TODO(tommycli): If we have a WebUI omnibox popup, we should move the
-  // secondary button logic out of the View and into the OmniboxPopupModel.
-  if (base::FeatureList::IsEnabled(omnibox::kWebUIOmniboxPopup))
-    return nullptr;
-
-  OmniboxPopupModel* popup_model = model()->popup_model();
-  if (!popup_model)
-    return nullptr;
-
-  size_t selected_line = popup_model->selected_line();
-  if (selected_line == OmniboxPopupModel::kNoMatch)
-    return nullptr;
-
-  // TODO(tommycli): https://crbug.com/1063071
-  // Diving into |popup_view_| was a mistake. Here's a hotfix to stop the crash,
-  // but the ultimate fix should be to move this logic into OmniboxPopupModel.
-  if (!popup_view_ || popup_view_->result_view_at(selected_line) == nullptr)
-    return nullptr;
-
-  return popup_view_->result_view_at(selected_line)->GetSecondaryButton();
-}
-
 bool OmniboxViewViews::DirectionAwareSelectionAtEnd() const {
   // When text and UI direction match, 'end' is as expected,
   // otherwise we use beginning.

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

@@ -47,10 +47,6 @@ namespace ui {
 class OSExchangeData;
 }  // namespace ui
 
-namespace views {
-class Button;
-}  // namespace views
-
 // Views-implementation of OmniboxView.
 class OmniboxViewViews : public OmniboxView,
                          public views::Textfield,
@@ -203,10 +199,6 @@ class OmniboxViewViews : public OmniboxView,
   // flip.)
   bool TextAndUIDirectionMatch() const;
 
-  // Gets the secondary button (like the tab switch or remove suggestion)
-  // for the selected line. Returns nullptr if there is no secondary button.
-  views::Button* GetSecondaryButtonForSelectedLine() const;
-
   // Like SelectionAtEnd(), but accounts for RTL.
   bool DirectionAwareSelectionAtEnd() const;
 

components/omnibox/browser/omnibox_popup_model.cc

@@ -470,7 +470,16 @@ bool OmniboxPopupModel::IsSelectionAvailable(Selection selection) const {
       // TODO(orinj): Here is an opportunity to clean up the presentational
       //  logic that pkasting wanted to take out of AutocompleteMatch. The view
       //  should be driven by the model, so this is really the place to decide.
-      return match.ShouldShowTabMatchButton();
+      //  In other words, this duplicates logic within OmniboxResultView.
+      //  This is the proper place. OmniboxResultView should refer to here.
+      if (match.ShouldShowTabMatchButton())
+        return true;
+      if (base::FeatureList::IsEnabled(
+              omnibox::kOmniboxSuggestionTransparencyOptions) &&
+          match.SupportsDeletion()) {
+        return true;
+      }
+      return false;
     case FOCUSED_BUTTON_KEYWORD:
       return match.associated_keyword != nullptr;
     case FOCUSED_BUTTON_TAB_SWITCH:

components/omnibox/browser/omnibox_popup_model_unittest.cc

@@ -166,9 +166,12 @@ TEST_F(OmniboxPopupModelTest, PopupStepSelection) {
     match.allowed_to_be_default_match = true;
     matches.push_back(match);
   }
-  // Give one match an associated keyword for irregular state stepping.
+  // Give the last match an associated keyword for irregular state stepping.
   matches.back().associated_keyword =
       std::make_unique<AutocompleteMatch>(matches.back());
+  // Make the middle match deletable to verify we can step to that.
+  matches[1].deletable = true;
+
   auto* result = &model()->autocomplete_controller()->result_;
   AutocompleteInput input(base::UTF8ToUTF16("match"),
                           metrics::OmniboxEventProto::NTP,
@@ -193,6 +196,8 @@ TEST_F(OmniboxPopupModelTest, PopupStepSelection) {
   // Step by states forward.
   for (auto selection : {
            OmniboxPopupModel::Selection(1, OmniboxPopupModel::NORMAL),
+           // Focused button is the Suggestion Removal button.
+           OmniboxPopupModel::Selection(1, OmniboxPopupModel::BUTTON_FOCUSED),
            OmniboxPopupModel::Selection(2, OmniboxPopupModel::NORMAL),
            OmniboxPopupModel::Selection(2, OmniboxPopupModel::KEYWORD),
            OmniboxPopupModel::Selection(0, OmniboxPopupModel::NORMAL),
@@ -206,6 +211,8 @@ TEST_F(OmniboxPopupModelTest, PopupStepSelection) {
   // should land on KEYWORD, but stepping backward should not.
   for (auto selection : {
            OmniboxPopupModel::Selection(2, OmniboxPopupModel::NORMAL),
+           // Focused button is the Suggestion Removal button.
+           OmniboxPopupModel::Selection(1, OmniboxPopupModel::BUTTON_FOCUSED),
            OmniboxPopupModel::Selection(1, OmniboxPopupModel::NORMAL),
            OmniboxPopupModel::Selection(0, OmniboxPopupModel::NORMAL),
            OmniboxPopupModel::Selection(2, OmniboxPopupModel::NORMAL),