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 d4245fd34e8719b14b02a9213e22a51447052195..90feb356d44497f7c4b05f9622e5c080dc6ca494 100644
--- a/chrome/browser/autocomplete/search_provider.cc
+++ b/chrome/browser/autocomplete/search_provider.cc
@@ -307,10 +307,11 @@ AutocompleteMatch SearchProvider::CreateSearchSuggestion(
       }
     }
   } else {
-    // Otherwise, we're dealing with the "default search" result which has no
-    // completion.
+    // Otherwise, |match| is a verbatim (what-you-typed) match, either for the
+    // default provider or a keyword search provider.
     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
@@ -323,6 +324,7 @@ AutocompleteMatch SearchProvider::CreateSearchSuggestion(
   if (!input.prevent_inline_autocomplete() &&
       StartsWith(query_string, input_text, false)) {
     match.inline_autocompletion = query_string.substr(input_text.length());
+    match.allowed_to_be_default_match = true;
   }
   match.fill_into_edit.append(query_string);
 
@@ -512,6 +514,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);
@@ -1065,15 +1068,21 @@ bool SearchProvider::IsTopMatchSearchWithURLInput() const {
          matches_.front().type != AutocompleteMatchType::NAVSUGGEST;
 }
 
-bool SearchProvider::IsTopMatchNotInlinable() const {
-  // 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_autocompletion.empty() &&
-      matches_.front().fill_into_edit != input_.text();
+bool SearchProvider::LacksValidDefaultMatch(

[Peter Kasting, 2013/08/06 22:56:16]

Nit: I think this function's body would read more clearly if you reversed it to
be "HasValidDefaultMatch()".  I found myself repeatedly confused about how we
were returning a bool indicating the lack of something.

[Mark P, 2013/08/07 00:44:31]

I chose Lacks...() for consistency with the other constraint functions.  In all
those cases, returning true means the constraint is violated.  Frankly, I think
some of the other names are confusing because of the way they're negated, but
that's outside the scope of this change (which is big enough already).

[Peter Kasting, 2013/08/07 00:49:53]

I don't see any particular benefit to being consistent in that sense.

I'd make this function Has...(), then have a followup change that reverses
whichever other functions would make more sense reversed.  But if you wanted to
do it all at once that'd be OK too; I just don't really want to leave this
function this way long term because my brain hurts.

[Mark P, 2013/08/07 00:58:52]

Done.

+    bool omnibox_will_reorder_for_legal_default_match) const {
+  // If the omnibox will reorder matches to put something allowed to be the
+  // default match first, then lacking a valid default match means there is
+  // no match allowed to be default.  If the omnibox will not reorder matches,
+  // then lacking a valid default match means the top match is not allowed
+  // to be the default match.

[Peter Kasting, 2013/08/06 22:56:16]

Nit: This comment is somewhat confusing.  How about:

  One of the SearchProvider matches may need to be the overall default.  If
AutocompleteResult is allowed to reorder matches, this means we simply need at
least one match in the list to be |allowed_to_be_default|.  If no reordering is
possible, however, then our first match needs to set this flag.

[Mark P, 2013/08/07 00:44:31]

Yes, that is clearer.  Used that comment.

+  for (ACMatches::const_iterator it = matches_.begin(); it != matches_.end();
+       ++it) {
+    if (it->allowed_to_be_default_match)
+      return false;
+    if (!omnibox_will_reorder_for_legal_default_match)
+      return true;
+  }
+  return true;
 }
 
 void SearchProvider::UpdateMatches() {
@@ -1098,11 +1107,20 @@ void SearchProvider::UpdateMatches() {
       ConvertResultsToAutocompleteMatches();
       DCHECK(!IsTopMatchNavigationInKeywordMode());
     }
-    if (IsTopMatchScoreTooLow()) {
+    // True if the omnibox will reorder matches as necessary to make the top
+    // one something that is allowed to be the default match.
+    const bool omnibox_will_reorder_for_legal_default_match =
+        OmniboxFieldTrial::ReorderForLegalDefaultMatch(
+            input_.current_page_classification());
+    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();
@@ -1118,18 +1136,26 @@ 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 (LacksValidDefaultMatch(omnibox_will_reorder_for_legal_default_match)) {
+      // If the omnibox is not going to reorder results to put a legal default
+      // match at the top, then this provider needs to guarantee that its top
+      // scoring result is a legal default match (i.e., it's 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.  On the other hand, if the omnibox will
+      // reorder matches as necessary to put a legal default match at the top,
+      // all we need to guarantee is that SearchProvider returns a legal
+      // default match.  (The omnibox always needs at least one legal default
+      // match, and it relies on SearchProvider to always return one.)

[Peter Kasting, 2013/08/06 22:56:16]

Nit: Given the comments in LacksValidDefaultMatch(), most or all of this comment
can go away.

[Mark P, 2013/08/07 00:44:31]

I know it's mostly redundant but I think it's useful here to make reading down
the list of constraints easier.  Again, it's mostly for consistency with
already-existing code.

       ApplyCalculatedRelevance();
       ConvertResultsToAutocompleteMatches();
     }
     DCHECK(!IsTopMatchNavigationInKeywordMode());
-    DCHECK(!IsTopMatchScoreTooLow());
+    DCHECK(omnibox_will_reorder_for_legal_default_match ||
+           !IsTopMatchScoreTooLow());

[Peter Kasting, 2013/08/06 22:56:16]

Nit: Indent 4, not even

[Mark P, 2013/08/07 00:44:31]

Done.

     DCHECK(!IsTopMatchSearchWithURLInput());
-    DCHECK(!IsTopMatchNotInlinable());
+    DCHECK(!LacksValidDefaultMatch(
+        omnibox_will_reorder_for_legal_default_match));
   }
 
   UpdateStarredStateOfMatches();
@@ -1450,6 +1476,8 @@ AutocompleteMatch SearchProvider::NavigationToMatch(
   if (!input_.prevent_inline_autocomplete() &&
       (inline_autocomplete_offset != string16::npos)) {
     DCHECK(inline_autocomplete_offset <= match.fill_into_edit.length());
+    match.allowed_to_be_default_match =
+        (inline_autocomplete_offset != string16::npos);

[Peter Kasting, 2013/08/06 22:56:16]

Eh?  The conditional above has already guaranteed this holds.

[Mark P, 2013/08/07 00:44:31]

Replaced with "true" :-).

     match.inline_autocompletion =
         match.fill_into_edit.substr(inline_autocomplete_offset);
   }