[omnibox] Support the the no-default-match case

Currently, AutocompleteResult doesn't really support the no-default-match
case. The first match is set to be the default match, even if none of the
matches are |allowed_to_be_default_match|.

This is problematic, because there's three known cases where there's
genuinely no default match:
 - NTP ZeroSuggest
 - Enterprise policy where there's no default search provider
 - ChromeOS launcher

ChromeOS launcher already supports the no-default-match case properly.
The other two are subtly broken.

This CL does two things:
 - It updates AutocompleteResult to set default_match() == end() when
   there's no valid default match.
 - It updates a bunch of logic OmniboxPopupModel (runs on Views and iOS)
   to support the no-default-match case.

Bug: 1016845, 363656
Change-Id: Ib684efba92b05cb677bdabf9265e35e2bfbeab56
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1906882
Commit-Queue: Tommy Li <tommycli@chromium.org>
Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Cr-Commit-Position: refs/heads/master@{#714664}

components/omnibox/browser/autocomplete_classifier.cc

@@ -82,14 +82,13 @@ void AutocompleteClassifier::Classify(
   controller_->Start(input);
   DCHECK(controller_->done());
   const AutocompleteResult& result = controller_->result();
-  if (result.empty()) {
+  if (result.empty() || result.default_match() == result.end()) {
     if (alternate_nav_url)
       *alternate_nav_url = GURL();
     return;
   }
 
-  DCHECK(result.default_match() != result.end());
-  *match = *result.default_match();
+  *match = *result.begin();
   if (alternate_nav_url)
     *alternate_nav_url = result.alternate_nav_url();
 }

components/omnibox/browser/autocomplete_controller.h

@@ -163,6 +163,8 @@ class AutocompleteController : public AutocompleteProviderListener,
   FRIEND_TEST_ALL_PREFIXES(OmniboxViewViewsUIATest, AccessibleOmnibox);
 #endif  // OS_WIN
   FRIEND_TEST_ALL_PREFIXES(OmniboxPopupModelTest, SetSelectedLine);
+  FRIEND_TEST_ALL_PREFIXES(OmniboxPopupModelTest,
+                           SetSelectedLineWithNoDefaultMatches);
   FRIEND_TEST_ALL_PREFIXES(OmniboxPopupModelTest, TestFocusFixing);
   FRIEND_TEST_ALL_PREFIXES(OmniboxPopupModelTest, PopupPositionChanging);
   FRIEND_TEST_ALL_PREFIXES(OmniboxPopupContentsViewTest,

components/omnibox/browser/autocomplete_result.cc

@@ -273,18 +273,36 @@ void AutocompleteResult::SortAndCull(
     GroupSuggestionsBySearchVsURL(next, matches_.end());
   }
 
-  // There is no default match for chromeOS launcher zero prefix query
-  // suggestions.
-  if (input.text().empty() && (input.current_page_classification() ==
-                               metrics::OmniboxEventProto::CHROMEOS_APP_LIST)) {
+  // Early exit when there is no default match. This can occur in these cases:
+  //  1. There are no matches.
+  //  2. The first match doesn't have |allowed_to_be_default_match| as true.
+  //     This implies that NONE of the matches were allowed to be the default.
+  //  3. Hardcoded for ChromeOS Launcher empty-textfield on-focus suggestions.
+  //     TODO(tommycli): We should remove the ChromeOS launcher special case.
+  //     Instead, ensure none of the launcher matches are allowed to be default.
+  if (matches_.empty() || !matches_.begin()->allowed_to_be_default_match ||
+      (input.text().empty() &&
+       (input.current_page_classification() ==
+        metrics::OmniboxEventProto::CHROMEOS_APP_LIST))) {
     default_match_ = end();
     alternate_nav_url_ = GURL();
     return;
   }
 
+  // Since we didn't early exit, the first match must be the default match.
+  // TODO(tommycli): Once we eliminate the ChromeOS Launcher hardcoding above,
+  // we can delete |default_match_|, since if matches.begin() has a true
+  // |allowed_to_be_default_match|, it will always be the default match.
   default_match_ = matches_.begin();
 
-  if (default_match_ != matches_.end()) {
+  // TODO(tommycli): Simplify our state by not pre-computing this.
+  alternate_nav_url_ = ComputeAlternateNavUrl(input, *default_match_);
+
+  // Almost all matches are "navigable": they have a valid |destination_url|.
+  // One example exception is the user tabbing into keyword search mode,
+  // but not having typed a query yet. In that case, the default match should
+  // rightfully be non-navigable, and pressing Enter should do nothing.
+  if (default_match_->destination_url.is_valid()) {
     const base::string16 debug_info =
         base::ASCIIToUTF16("fill_into_edit=") + default_match_->fill_into_edit +
         base::ASCIIToUTF16(", provider=") +
@@ -293,49 +311,22 @@ void AutocompleteResult::SortAndCull(
              : base::string16()) +
         base::ASCIIToUTF16(", input=") + input.text();
 
-    // It's unusual if |default_match_| is not |allowed_to_be_default_match|.
-    // This can occur in two situations:
-    //  - Empty-textfield on-focus suggestions (i.e. NTP, ChromeOS launcher)
-    //  - Enterprise policy prohibiting a default search provider
-    //
-    // In those cases, hitting Enter should do nothing, so there should be
-    // legitimately no default match.
-    //
-    // TODO(tommycli): It seems odd that we are still setting |default_match_|
-    // in that case. We should fix that.
-    if (!default_match_->allowed_to_be_default_match) {
-      bool default_search_provider_exists =
-          template_url_service &&
-          template_url_service->GetDefaultSearchProvider();
-      DCHECK(input.text().empty() || !default_search_provider_exists)
-          << debug_info;
-    }
-
-    // For navigable default matches, make sure the destination type is what the
-    // user would expect given the input.
-    if (default_match_->allowed_to_be_default_match &&
-        default_match_->destination_url.is_valid()) {
-      if (AutocompleteMatch::IsSearchType(default_match_->type)) {
-        // We shouldn't get query matches for URL inputs.
-        DCHECK_NE(metrics::OmniboxInputType::URL, input.type()) << debug_info;
-      } else {
-        // If the user explicitly typed a scheme, the default match should
-        // have the same scheme.
-        if ((input.type() == metrics::OmniboxInputType::URL) &&
-            input.parts().scheme.is_nonempty()) {
-          const std::string& in_scheme = base::UTF16ToUTF8(input.scheme());
-          const std::string& dest_scheme =
-              default_match_->destination_url.scheme();
-          DCHECK(url_formatter::IsEquivalentScheme(in_scheme, dest_scheme))
-              << debug_info;
-        }
+    if (AutocompleteMatch::IsSearchType(default_match_->type)) {
+      // We shouldn't get query matches for URL inputs.
+      DCHECK_NE(metrics::OmniboxInputType::URL, input.type()) << debug_info;
+    } else {
+      // If the user explicitly typed a scheme, the default match should
+      // have the same scheme.
+      if ((input.type() == metrics::OmniboxInputType::URL) &&
+          input.parts().scheme.is_nonempty()) {
+        const std::string& in_scheme = base::UTF16ToUTF8(input.scheme());
+        const std::string& dest_scheme =
+            default_match_->destination_url.scheme();
+        DCHECK(url_formatter::IsEquivalentScheme(in_scheme, dest_scheme))
+            << debug_info;
       }
     }
   }
-
-  // Set the alternate nav URL.
-  alternate_nav_url_ = (default_match_ == matches_.end()) ?
-      GURL() : ComputeAlternateNavUrl(input, *default_match_);
 }
 
 void AutocompleteResult::DemoteOnDeviceSearchSuggestions() {

components/omnibox/browser/omnibox_edit_model.cc

@@ -237,7 +237,6 @@ AutocompleteMatch OmniboxEditModel::CurrentMatch(
     GURL* alternate_nav_url) const {
   // If we have a valid match use it. Otherwise get one for the current text.
   AutocompleteMatch match = omnibox_controller_->current_match();
-
   if (!match.destination_url.is_valid()) {
     GetInfoForCurrentText(&match, alternate_nav_url);
   } else if (alternate_nav_url) {
@@ -1196,7 +1195,8 @@ void OmniboxEditModel::OnUpOrDownKeyPressed(int count) {
     // (user_input_in_progress_ is false) unless the first result is a
     // verbatim match of the omnibox input (on-focus query refinements on SERP).
     const size_t line_no = GetNewSelectedLine(count);
-    if (has_temporary_text_ && line_no == 0 &&
+    if (result().default_match() != result().end() && has_temporary_text_ &&
+        line_no == 0 &&
         (user_input_in_progress_ ||
          result().default_match()->IsVerbatimType())) {
       RevertTemporaryTextAndPopup();
@@ -1506,31 +1506,29 @@ void OmniboxEditModel::GetInfoForCurrentText(AutocompleteMatch* match,
                                              GURL* alternate_nav_url) const {
   DCHECK(match);
 
+  // If there's a query in progress or the popup is open, pick out the default
+  // match or selected match, if there is one.
+  bool found_match_for_text = false;
   if (query_in_progress() || PopupIsOpen()) {
-    if (query_in_progress()) {
-      // It's technically possible for |result| to be empty if no provider
-      // returns a synchronous result but the query has not completed
-      // synchronously; practically, however, that should never actually happen.
-      if (result().empty())
-        return;
+    if (query_in_progress() && result().default_match() != result().end()) {
       // The user cannot have manually selected a match, or the query would have
       // stopped. So the default match must be the desired selection.
       *match = *result().default_match();
-    } else {
-      // If there are no results, the popup should be closed, so we shouldn't
-      // have gotten here.
-      CHECK(!result().empty());
-      CHECK(popup_model()->selected_line() < result().size());
+      found_match_for_text = true;
+    } else if (popup_model()->selected_line() != OmniboxPopupModel::kNoMatch) {
       const AutocompleteMatch& selected_match =
           result().match_at(popup_model()->selected_line());
       *match =
           (popup_model()->selected_line_state() == OmniboxPopupModel::KEYWORD) ?
               *selected_match.associated_keyword : selected_match;
+      found_match_for_text = true;
     }
-    if (alternate_nav_url &&
+    if (found_match_for_text && alternate_nav_url &&
         (!popup_model() || !popup_model()->has_selected_match()))
       *alternate_nav_url = result().alternate_nav_url();
-  } else {
+  }
+
+  if (!found_match_for_text) {
     client_->GetAutocompleteClassifier()->Classify(
         MaybePrependKeyword(view_->GetText()), is_keyword_selected(), true,
         GetPageClassification(), match, alternate_nav_url);
@@ -1545,12 +1543,16 @@ void OmniboxEditModel::RevertTemporaryTextAndPopup() {
   has_temporary_text_ = false;
 
   if (popup_model())
-    popup_model()->ResetToDefaultMatch();
+    popup_model()->ResetToInitialState();
 
-  // If user input is not in progress, we are reverting an on-focus suggestion.
-  // Set the window text back to the original input, rather than the top match.
+  // There are two cases in which resetting to the default match doesn't restore
+  // the proper original text:
+  //  1. If user input is not in progress, we are reverting an on-focus
+  //     suggestion. These may be unrelated to the original input.
+  //  2. If there's no default match at all.
+  //
   // The original selection will be restored in OnRevertTemporaryText() below.
-  if (!user_input_in_progress_) {
+  if (!user_input_in_progress_ || result().default_match() == result().end()) {
     view_->SetWindowTextAndCaretPos(input_.text(), /*caret_pos=*/0,
                                     /*update_popup=*/false,
                                     /*notify_text_changed=*/true);

components/omnibox/browser/omnibox_edit_model.h

@@ -437,14 +437,14 @@ class OmniboxEditModel {
   base::string16 MaybeStripKeyword(const base::string16& text) const;
   base::string16 MaybePrependKeyword(const base::string16& text) const;
 
-  // If there's a selected match, copies it into |match|. Else, returns the
-  // default match for the current text, as well as the alternate nav URL, if
-  // |alternate_nav_url| is non-NULL and there is such a URL.
+  // Copies a match corresponding to the current text into |match|, and
+  // populates |alternate_nav_url| as well if it's not nullptr. If the popup
+  // is closed, the match is generated from the autocomplete classifier.
   void GetInfoForCurrentText(AutocompleteMatch* match,
                              GURL* alternate_nav_url) const;
 
   // Reverts the edit box from a temporary text back to the original user text.
-  // Also resets the popup to the default match.
+  // Also resets the popup to the initial state.
   void RevertTemporaryTextAndPopup();
 
   // Accepts current keyword if the user just typed a space at the end of

components/omnibox/browser/omnibox_popup_model.cc

@@ -118,8 +118,8 @@ void OmniboxPopupModel::SetSelectedLine(size_t line,
   // Cancel the query so the matches don't change on the user.
   autocomplete_controller()->Stop(false);
 
-  line = std::min(line, result.size() - 1);
-  const AutocompleteMatch& match = result.match_at(line);
+  if (line != kNoMatch)
+    line = std::min(line, result.size() - 1);
   has_selected_match_ = !reset_to_default;
 
   if (line == selected_line_ && !force)
@@ -130,18 +130,19 @@ void OmniboxPopupModel::SetSelectedLine(size_t line,
   // draw.  We also need to update |selected_line_| before calling
   // OnPopupDataChanged(), so that when the edit notifies its controller that
   // something has changed, the controller can get the correct updated data.
-  //
-  // NOTE: We should never reach here with no selected line; the same code that
-  // opened the popup and made it possible to get here should have also set a
-  // selected line.
-  CHECK(selected_line_ != kNoMatch);
   const size_t prev_selected_line = selected_line_;
   selected_line_state_ = NORMAL;
   selected_line_ = line;
-  view_->InvalidateLine(prev_selected_line);
-  view_->InvalidateLine(selected_line_);
+  if (prev_selected_line != kNoMatch) {
+    view_->InvalidateLine(prev_selected_line);
+  }
+  if (selected_line_ != kNoMatch) {
+    view_->InvalidateLine(selected_line_);
+    view_->OnLineSelected(selected_line_);
+  }
 
-  view_->OnLineSelected(selected_line_);
+  if (line == kNoMatch)
+    return;
 
   // Update the edit with the new data for this match.
   // TODO(pkasting): If |selected_line_| moves to the controller, this can be
@@ -149,6 +150,7 @@ void OmniboxPopupModel::SetSelectedLine(size_t line,
   base::string16 keyword;
   bool is_keyword_hint;
   TemplateURLService* service = edit_model_->client()->GetTemplateURLService();
+  const AutocompleteMatch& match = result.match_at(line);
   match.GetKeywordUIState(service, &keyword, &is_keyword_hint);
 
   if (reset_to_default) {
@@ -162,10 +164,12 @@ void OmniboxPopupModel::SetSelectedLine(size_t line,
   }
 }
 
-void OmniboxPopupModel::ResetToDefaultMatch() {
+void OmniboxPopupModel::ResetToInitialState() {
   const AutocompleteResult& result = this->result();
-  CHECK(!result.empty());
-  SetSelectedLine(result.default_match() - result.begin(), true, false);
+  size_t new_line = kNoMatch;
+  if (result.default_match() != result.end())
+    new_line = result.default_match() - result.begin();
+  SetSelectedLine(new_line, true, false);
   view_->OnDragCanceled();
 }
 
@@ -242,14 +246,19 @@ void OmniboxPopupModel::OnResultChanged() {
   rich_suggestion_bitmaps_.clear();
   const AutocompleteResult& result = this->result();
   size_t old_selected_line = selected_line_;
-  selected_line_ = result.default_match() == result.end() ?
-      kNoMatch : static_cast<size_t>(result.default_match() - result.begin());
-  // There had better not be a nonempty result set with no default match.
-  CHECK((selected_line_ != kNoMatch) || result.empty());
   has_selected_match_ = false;
-  // If selected line state was |BUTTON_FOCUSED| and nothing has changed, leave
-  // it.
-  if (selected_line_ != kNoMatch) {
+
+  if (result.default_match() == result.end()) {
+    selected_line_ = kNoMatch;
+    selected_line_state_ = NORMAL;
+  } else {
+    // TODO(tommycli): The default match is always in the first position. After
+    // we cement these semantics, we should just set selected_line_ to 0.
+    selected_line_ =
+        static_cast<size_t>(result.default_match() - result.begin());
+
+    // If selected line state was |BUTTON_FOCUSED| and nothing has changed,
+    // leave it.
     const bool has_focused_match =
         selected_line_state_ == BUTTON_FOCUSED &&
         result.match_at(selected_line_).has_tab_match;
@@ -258,8 +267,6 @@ void OmniboxPopupModel::OnResultChanged() {
         result.match_at(selected_line_).destination_url != old_focused_url_;
     if (!has_focused_match || has_changed)
       selected_line_state_ = NORMAL;
-  } else {
-    selected_line_state_ = NORMAL;
   }
 
   bool popup_was_open = view_->IsOpen();

components/omnibox/browser/omnibox_popup_model.h

@@ -83,17 +83,17 @@ class OmniboxPopupModel {
   // the necessary parts of the window, as well as updating the edit with the
   // new temporary text.  |line| will be clamped to the range of valid lines.
   // |reset_to_default| is true when the selection is being reset back to the
-  // default match, and thus there is no temporary text (and not
+  // initial state, and thus there is no temporary text (and not
   // |has_selected_match_|). If |force| is true then the selected line will
   // be updated forcibly even if the |line| is same as the current selected
   // line.
-  // NOTE: This assumes the popup is open, and thus both old and new values for
-  // the selected line should not be kNoMatch.
+  // NOTE: This assumes the popup is open, although both the old and new values
+  // for the selected line can be kNoMatch.
   void SetSelectedLine(size_t line, bool reset_to_default, bool force);
 
   // Called when the user hits escape after arrowing around the popup.  This
-  // will change the selected line back to the default match and redraw.
-  void ResetToDefaultMatch();
+  // will reset the popup to the initial state.
+  void ResetToInitialState();
 
   // Immediately updates and opens the popup if necessary, then moves the
   // current selection to the respective line. If the line is unchanged, the
@@ -186,6 +186,8 @@ class OmniboxPopupModel {
   GURL old_focused_url_;
 
   // The user has manually selected a match.
+  // TODO(tommycli): We can _probably_ eliminate this variable. It seems to be
+  // mostly rendundant with selected_line() and result()->default_match().
   bool has_selected_match_;
 
   // True if the popup should close on omnibox blur. This defaults to true, and

components/omnibox/browser/omnibox_popup_model_unittest.cc

@@ -96,6 +96,39 @@ TEST_F(OmniboxPopupModelTest, SetSelectedLine) {
   EXPECT_TRUE(popup_model()->has_selected_match());
 }
 
+TEST_F(OmniboxPopupModelTest, SetSelectedLineWithNoDefaultMatches) {
+  // Creates a set of matches with NO matches allowed to be default.
+  ACMatches matches;
+  for (size_t i = 0; i < 2; ++i) {
+    AutocompleteMatch match(nullptr, 1000, false,
+                            AutocompleteMatchType::URL_WHAT_YOU_TYPED);
+    match.keyword = base::ASCIIToUTF16("match");
+    matches.push_back(match);
+  }
+  auto* result = &model()->autocomplete_controller()->result_;
+  AutocompleteInput input(base::UTF8ToUTF16("match"),
+                          metrics::OmniboxEventProto::NTP,
+                          TestSchemeClassifier());
+  result->AppendMatches(input, matches);
+  result->SortAndCull(input, nullptr);
+
+  popup_model()->OnResultChanged();
+  EXPECT_EQ(OmniboxPopupModel::kNoMatch, popup_model()->selected_line());
+  EXPECT_FALSE(popup_model()->has_selected_match());
+
+  popup_model()->SetSelectedLine(0, false, false);
+  EXPECT_EQ(0U, popup_model()->selected_line());
+  EXPECT_TRUE(popup_model()->has_selected_match());
+
+  popup_model()->SetSelectedLine(1, false, false);
+  EXPECT_EQ(1U, popup_model()->selected_line());
+  EXPECT_TRUE(popup_model()->has_selected_match());
+
+  popup_model()->ResetToInitialState();
+  EXPECT_EQ(OmniboxPopupModel::kNoMatch, popup_model()->selected_line());
+  EXPECT_FALSE(popup_model()->has_selected_match());
+}
+
 TEST_F(OmniboxPopupModelTest, PopupPositionChanging) {
   ACMatches matches;
   for (size_t i = 0; i < 3; ++i) {