Omnibox: Make the Controller Reorder Matches for Inlining

chrome/browser/autocomplete/search_provider.cc

Index: chrome/browser/autocomplete/search_provider.cc
diff --git a/chrome/browser/autocomplete/search_provider.cc b/chrome/browser/autocomplete/search_provider.cc
index 2f304504aedc799592c70a674e19c82aaa784837..104e888ffa95658290746b57ca0fefcee6bbf83b 100644
--- a/chrome/browser/autocomplete/search_provider.cc
+++ b/chrome/browser/autocomplete/search_provider.cc
@@ -255,7 +255,9 @@ SearchProvider::SearchProvider(AutocompleteProviderListener* listener,
       prevent_search_history_inlining_(
           OmniboxFieldTrial::SearchHistoryPreventInlining()),
       disable_search_history_(
-          OmniboxFieldTrial::SearchHistoryDisable()) {
+          OmniboxFieldTrial::SearchHistoryDisable()),
+      omnibox_will_reorder_for_legal_default_match_(
+          OmniboxFieldTrial::InReorderForLegalDefaultMatchGroup()) {
 }
 
 // static
@@ -315,6 +317,7 @@ AutocompleteMatch SearchProvider::CreateSearchSuggestion(
     // completion.
     match.contents_class.push_back(
         ACMatchClassification(0, ACMatchClassification::NONE));
+    match.allowed_to_be_default_match = true;
   }
 
   // When the user forced a query, we need to make sure all the fill_into_edit
@@ -328,6 +331,7 @@ AutocompleteMatch SearchProvider::CreateSearchSuggestion(
       StartsWith(query_string, input_text, false)) {
     match.inline_autocomplete_offset =
         match.fill_into_edit.length() + input_text.length();
+    match.allowed_to_be_default_match = true;
   }
   match.fill_into_edit.append(query_string);
 
@@ -517,6 +521,7 @@ void SearchProvider::Start(const AutocompleteInput& input,
       match.contents_class.push_back(
           ACMatchClassification(0, ACMatchClassification::NONE));
       match.keyword = providers_.default_provider();
+      match.allowed_to_be_default_match = true;
       matches_.push_back(match);
     }
     Stop(false);
@@ -1072,15 +1077,17 @@ bool SearchProvider::IsTopMatchHighRankSearchForURL() const {
           matches_.front().type == AutocompleteMatchType::SEARCH_OTHER_ENGINE);
 }
 
-bool SearchProvider::IsTopMatchNotInlinable() const {

[msw, 2013/07/18 06:23:57]

This function is almost what I'd expect AutocompleteResult to check to ensure
that matches marked as allowed really are allowed.

[Mark P, 2013/07/21 20:31:05]

Comment moot given the discussion you and Peter had in autocomplete_result.cc

-  // Note: this test assumes the SEARCH_OTHER_ENGINE match corresponds to
-  // the verbatim search query on the keyword engine.  SearchProvider should
-  // not create any other match of type SEARCH_OTHER_ENGINE.
-  return
-      matches_.front().type != AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED &&
-      matches_.front().type != AutocompleteMatchType::SEARCH_OTHER_ENGINE &&
-      matches_.front().inline_autocomplete_offset == string16::npos &&
-      matches_.front().fill_into_edit != input_.text();
+bool SearchProvider::IsTopMatchNotAllowedToBeDefaultMatch() const {

[msw, 2013/07/18 06:23:57]

Can you combine these two functions? It'd make more sense if the logic to check
only the top match versus checking anywhere within the set of matches were more
tightly related to the omnibox_will_reorder_for_legal_default_match_ flag that
controls this nuanced detail.

[Mark P, 2013/07/21 20:31:05]

I see your desire, but I'd like to keep them separate.  Combining them would
make something check the first if reorder is false and the latter if reorder is
true, but I like having them separate so I can write DCHECKs below that allow me
to assert the later regardless of the former.  Or maybe I could easily collect
statistics on how often the former fails when reorder is true.

[msw, 2013/07/23 21:55:32]

A combined function would check that either (1) or (2) is true:
(1) The top match may be default. <OR> (2) Some match may be default.
(1) implies (2), so if (1) or (2) is true, then (2) must be true.
DCHECK(Combined) will automatically "assert the later regardless of the former".
Combine them :)

[Mark P, 2013/07/26 16:48:13]

Please suggest a good combined name.  I can't think of one, which is why my
default is separate-makes-more-sense.

[msw, 2013/07/26 20:04:49]

LacksValidDefaultMatch or HasValidDefaultMatch seems good.

[Mark P, 2013/07/26 23:55:24]

Used the first to parallel the other constraint testing functions.

+  return !matches_.front().allowed_to_be_default_match;
+}
+
+bool SearchProvider::LacksDefaultMatch() const {
+  for (ACMatches::const_iterator it = matches_.begin(); it != matches_.end();
+       ++it) {
+    if (it->allowed_to_be_default_match)
+      return false;
+  }
+  return true;
 }
 
 void SearchProvider::UpdateMatches() {
@@ -1105,11 +1112,15 @@ void SearchProvider::UpdateMatches() {
       ConvertResultsToAutocompleteMatches();
       DCHECK(!IsTopMatchNavigationInKeywordMode());
     }
-    if (IsTopMatchScoreTooLow()) {
+    if (!omnibox_will_reorder_for_legal_default_match_ &&
+        IsTopMatchScoreTooLow()) {
       // Disregard the suggested verbatim relevance if the top score is below
       // the usual verbatim value. For example, a BarProvider may rely on
-      // SearchProvider's verbatim or inlineable matches for input "foo" to
-      // always outrank its own lowly-ranked non-inlineable "bar" match.
+      // SearchProvider's verbatim or inlineable matches for input "foo" (all
+      // allowed to be default match) to always outrank its own lowly-ranked
+      // "bar" matches that shouldn't be the default match.  This only needs
+      // to be enforced when the omnibox will not reorder results to make a
+      // legal default match first.
       default_results_.verbatim_relevance = -1;
       keyword_results_.verbatim_relevance = -1;
       ConvertResultsToAutocompleteMatches();
@@ -1125,18 +1136,31 @@ void SearchProvider::UpdateMatches() {
       keyword_results_.verbatim_relevance = -1;
       ConvertResultsToAutocompleteMatches();
     }
-    if (IsTopMatchNotInlinable()) {
-      // Disregard suggested relevances if the top match is not a verbatim match
-      // or inlinable.  For example, input "foo" should not invoke a search for
-      // "bar", which would happen if the "bar" search match outranked all other
-      // matches.
+    if (omnibox_will_reorder_for_legal_default_match_ ?
+        LacksDefaultMatch() : IsTopMatchNotAllowedToBeDefaultMatch()) {
+      // Disregard suggested relevances if either:
+      // * No matches are allowed to be the default match.  SearchProvider is
+      //   supposed to return a legal default match for any input.  We need
+      //   to make sure it does so.  This constraint should be true always,
+      //   but we only need to check it if
+      //   |omnibox_will_reorder_for_legal_default_match_|.  (If that were
+      //   false, the first part of the test above will suffice.)

[msw, 2013/07/18 06:23:57]

I don't follow "the first part of the test above will suffice", can you be more
explicit? Or move this logic into the combined LacksDefaultMatch() :
IsTopMatchNotAllowedToBeDefaultMatch() I suggested above, and I think it'll be a
lot more self-evident from the code.

[Mark P, 2013/07/21 20:31:05]

I rewrote this comment.  It was confusing because the original comment's
structure was explaining some code that was structured differently in the past
that I revised (to use the ? : construct) per Peter's suggestion.  I didn't
notice the comment became confusing as a result.

+      // * The top match is not allowed to be the default match (i.e., it's not
+      //   either a verbatim match or inlinable).  For example, input "foo"
+      //   should not invoke a search for "bar", which would happen if the
+      //   "bar" search match outranked all other matches.  We only care
+      //   about this condition if the omnibox itself will not reorder
+      //   matches to put a legal default match on top.
       ApplyCalculatedRelevance();
       ConvertResultsToAutocompleteMatches();
     }
     DCHECK(!IsTopMatchNavigationInKeywordMode());
-    DCHECK(!IsTopMatchScoreTooLow());
+    DCHECK(omnibox_will_reorder_for_legal_default_match_ ||
+           !IsTopMatchScoreTooLow());
     DCHECK(!IsTopMatchHighRankSearchForURL());
-    DCHECK(!IsTopMatchNotInlinable());
+    DCHECK(omnibox_will_reorder_for_legal_default_match_ ||
+           !IsTopMatchNotAllowedToBeDefaultMatch());
+    DCHECK(!LacksDefaultMatch());
   }
 
   UpdateStarredStateOfMatches();
@@ -1449,8 +1473,11 @@ AutocompleteMatch SearchProvider::NavigationToMatch(
           net::FormatUrl(navigation.url(), languages, format_types,
                          net::UnescapeRule::SPACES, NULL, NULL,
                          &inline_autocomplete_offset));
-  if (!input_.prevent_inline_autocomplete())
+  if (!input_.prevent_inline_autocomplete()) {
+    match.allowed_to_be_default_match =
+        (inline_autocomplete_offset != string16::npos);
     match.inline_autocomplete_offset = inline_autocomplete_offset;
+  }
   DCHECK((match.inline_autocomplete_offset == string16::npos) ||
          (match.inline_autocomplete_offset <= match.fill_into_edit.length()));