Omnibox: Make the Controller Reorder Matches for Inlining

chrome/browser/autocomplete/keyword_provider.cc

Index: chrome/browser/autocomplete/keyword_provider.cc
diff --git a/chrome/browser/autocomplete/keyword_provider.cc b/chrome/browser/autocomplete/keyword_provider.cc
index dd2959e9eb41a0921a1f294c8847d3eeadf919ec..c678e92658cd7f860a22ce0b1641168c7d8a1a0a 100644
--- a/chrome/browser/autocomplete/keyword_provider.cc
+++ b/chrome/browser/autocomplete/keyword_provider.cc
@@ -215,9 +215,12 @@ AutocompleteMatch KeywordProvider::CreateAutocompleteMatch(
     const string16& text,
     const string16& keyword,
     const AutocompleteInput& input) {
+  // This enclosing function is only called by omnibox_controller.cc to create
+  // a what-you-typed match on the keyword input.  It's okay to allow this
+  // match to be the default match.
   return CreateAutocompleteMatch(
       GetTemplateURLService()->GetTemplateURLForKeyword(keyword), input,
-      keyword.length(), SplitReplacementStringFromInput(text, true), 0);
+      keyword.length(), SplitReplacementStringFromInput(text, true), true, 0);
 }
 
 void KeywordProvider::Start(const AutocompleteInput& input,
@@ -318,8 +321,12 @@ void KeywordProvider::Start(const AutocompleteInput& input,
 
     // TODO(pkasting): We should probably check that if the user explicitly
     // typed a scheme, that scheme matches the one in |template_url|.
+
+    // When creating an exact match (either for the keyword itself, no
+    // remaining query or an extension keyword, possibly with remaining
+    // input), allow the match to be the default match.
     matches_.push_back(CreateAutocompleteMatch(
-        template_url, input, keyword.length(), remaining_input, -1));
+        template_url, input, keyword.length(), remaining_input, true, -1));
 
     if (profile_ && is_extension_keyword) {
       if (input.matches_requested() == AutocompleteInput::ALL_MATCHES) {
@@ -365,7 +372,7 @@ void KeywordProvider::Start(const AutocompleteInput& input,
     for (TemplateURLService::TemplateURLVector::const_iterator i(
          matches.begin()); i != matches.end(); ++i) {
       matches_.push_back(CreateAutocompleteMatch(
-          *i, input, keyword.length(), remaining_input, -1));
+          *i, input, keyword.length(), remaining_input, false, -1));
     }
   }
 }
@@ -420,6 +427,7 @@ AutocompleteMatch KeywordProvider::CreateAutocompleteMatch(
     const AutocompleteInput& input,
     size_t prefix_length,
     const string16& remaining_input,
+    bool allowed_to_be_default_match,
     int relevance) {
   DCHECK(template_url);
   const bool supports_replacement =
@@ -442,6 +450,7 @@ AutocompleteMatch KeywordProvider::CreateAutocompleteMatch(
   AutocompleteMatch match(this, relevance, false,
       supports_replacement ? AutocompleteMatchType::SEARCH_OTHER_ENGINE :
                              AutocompleteMatchType::HISTORY_KEYWORD);
+  match.allowed_to_be_default_match = allowed_to_be_default_match;
   match.fill_into_edit = keyword;
   if (!remaining_input.empty() || !keyword_complete || supports_replacement)
     match.fill_into_edit.push_back(L' ');
@@ -567,13 +576,17 @@ void KeywordProvider::Observe(int type,
         // We want to order these suggestions in descending order, so start with
         // the relevance of the first result (added synchronously in Start()),
         // and subtract 1 for each subsequent suggestion from the extension.
-        // We know that |complete| is true, because we wouldn't get results from
-        // the extension unless the full keyword had been typed.
+        // We recompute the first match's relevance; we know that |complete|
+        // is true, because we wouldn't get results from the extension unless
+        // the full keyword had been typed.
         int first_relevance = CalculateRelevance(input.type(), true, true,
             input.prefer_keyword(), input.allow_exact_keyword_match());
+        // Because these matches are sync, we make them score less than the

[msw, 2013/08/07 20:09:21]

I don't follow this statement at all... is the first match not returned
synchronously? why do lower scores make the other matches not legal defaults?
Aren't they actually legal, but simply unlikely to be promoted to the default
match so long as the first match is legal? Ignore me, I clearly don't understand
this...

[Mark P, 2013/08/07 22:13:05]

Oops, I think all that confusion came from a typo: sync vs. async.

I think it's clear this comment is actively harmful.  I rewrote it to be closer
to a
version I originally had before Peter asked me to think about expanding it.

[Peter Kasting, 2013/08/07 22:20:01]

Your newest version seems noticeably worse.  I disagree with your rewriting. 
How about something like this:

"Because these matches are async, we should never let them become the default
match, lest we introduce race conditions in the omnibox' user interaction."

[Mark P, 2013/08/08 15:22:17]

Yeah, that sounds good.  Done.

+        // first match.  This also means they can never become the default
+        // match.  Mark them as such.
         extension_suggest_matches_.push_back(CreateAutocompleteMatch(
             template_url, input, keyword.length(),
-            UTF8ToUTF16(suggestion.content), first_relevance - (i + 1)));
+            UTF8ToUTF16(suggestion.content), false, first_relevance - (i + 1)));
 
         AutocompleteMatch* match = &extension_suggest_matches_.back();
         match->contents.assign(UTF8ToUTF16(suggestion.description));