Omnibox: Prevent Asynchronous Suggestions from Changing Default Match

chrome/browser/autocomplete/search_provider.cc

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/autocomplete/search_provider.h"
 
 #include <algorithm>
 #include <cmath>
 
 #include "base/base64.h"
@@ skipping 152 matching lines @@
   // functions are currently in sync, but there's no reason we
   // couldn't decide in the future to score verbatim matches
   // differently for extension and non-extension keywords.  If you
   // make such a change, however, you should update this comment to
   // describe it, so it's clear why the functions diverge.
   if (prefer_keyword)
     return 1500;
   return (type == metrics::OmniboxInputType::QUERY) ? 1450 : 1100;
 }
 
+// static
+void SearchProvider::RemoveOrReviseOldResultsForOneProvider(
+    bool minimal_changes, SearchSuggestionParser::Results* results) {
+  // When called without |minimal_changes|, it likely means the user has
+  // pressed a key.  Revise the cached results appropriately.
+  if (!minimal_changes) {
+    for (SearchSuggestionParser::SuggestResults::iterator sug_it =
+             results->suggest_results.begin();
+         sug_it != results->suggest_results.end(); ++sug_it) {
+      sug_it->set_received_after_last_keystroke(false);
+    }
+    for (SearchSuggestionParser::NavigationResults::iterator nav_it =
+             results->navigation_results.begin();
+         nav_it != results->navigation_results.end(); ++nav_it) {
+      nav_it->set_received_after_last_keystroke(false);
+    }
+  }
+}
+
+// static
+ACMatches::iterator SearchProvider::FindTopMatch(ACMatches* matches) {
+  ACMatches::iterator it = matches->begin();
+  while ((it != matches->end()) && !it->allowed_to_be_default_match)
+    ++it;
+  return it;
+}
+
 void SearchProvider::Start(const AutocompleteInput& input,
                            bool minimal_changes) {
   // Do our best to load the model as early as possible.  This will reduce
   // odds of having the model not ready when really needed (a non-empty input).
   TemplateURLService* model = providers_.template_url_service();
   DCHECK(model);
   model->Load();
 
   matches_.clear();
   field_trial_triggered_ = false;
@@ skipping 202 matching lines @@
       UMA_HISTOGRAM_TIMES("Omnibox.SuggestRequest.Success.GoogleResponseTime",
                           elapsed_time);
     } else {
       UMA_HISTOGRAM_TIMES("Omnibox.SuggestRequest.Failure.GoogleResponseTime",
                           elapsed_time);
     }
   }
 }
 
 void SearchProvider::UpdateMatches() {
+  PersistGoodResultsForOneProvider(&default_results_);
+  PersistGoodResultsForOneProvider(&keyword_results_);
   ConvertResultsToAutocompleteMatches();
 
   // Check constraints that may be violated by suggested relevances.
   if (!matches_.empty() &&
       (default_results_.HasServerProvidedScores() ||
        keyword_results_.HasServerProvidedScores())) {
     // These blocks attempt to repair undesirable behavior by suggested
     // relevances with minimal impact, preserving other suggested relevances.
 
-    if (!HasKeywordDefaultMatchInKeywordMode()) {
+    if ((providers_.GetKeywordProviderURL() != NULL) &&
+        (FindTopMatch() == matches_.end())) {
       // In keyword mode, disregard the keyword verbatim suggested relevance
-      // if necessary so there at least one keyword match that's allowed to
-      // be the default match.
-      keyword_results_.verbatim_relevance = -1;
+      // if necessary, so at least one match is allowed to be default.  Give
+      // it the lowest non-zero score to best reflect what the server desired.
+      keyword_results_.verbatim_relevance = 1;
       ConvertResultsToAutocompleteMatches();
     }
     if (IsTopMatchSearchWithURLInput()) {
       // Disregard the suggested search and verbatim relevances if the input
       // type is URL and the top match is a highly-ranked search suggestion.
       // For example, prevent a search for "foo.com" from outranking another
       // provider's navigation for "foo.com" or "foo.com/url_from_history".
       ApplyCalculatedSuggestRelevance(&keyword_results_.suggest_results);
       ApplyCalculatedSuggestRelevance(&default_results_.suggest_results);
       default_results_.verbatim_relevance = -1;
       keyword_results_.verbatim_relevance = -1;
       ConvertResultsToAutocompleteMatches();
     }
     if (FindTopMatch() == matches_.end()) {
       // Guarantee 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.)
-      ApplyCalculatedRelevance();
+      // on SearchProvider to always return one.)  Give them the lowest
+      // non-zero scores to best reflect what the server desired.
+      default_results_.verbatim_relevance = 1;
+      keyword_results_.verbatim_relevance = 1;
       ConvertResultsToAutocompleteMatches();
     }
-    DCHECK(HasKeywordDefaultMatchInKeywordMode());
     DCHECK(!IsTopMatchSearchWithURLInput());
     DCHECK(FindTopMatch() != matches_.end());
   }
   UMA_HISTOGRAM_CUSTOM_COUNTS(
       "Omnibox.SearchProviderMatches", matches_.size(), 1, 6, 7);
 
-  const TemplateURL* keyword_url = providers_.GetKeywordProviderURL();

[Mark P, 2014/08/15 20:04:38]

This was clobbered in the refactoring changelist.

-  if ((keyword_url != NULL) && HasKeywordDefaultMatchInKeywordMode()) {
-    // If there is a keyword match that is allowed to be the default match,
-    // then prohibit default provider matches from being the default match lest
-    // such matches cause the user to break out of keyword mode.
-    for (ACMatches::iterator it = matches_.begin(); it != matches_.end();
-         ++it) {
-      if (it->keyword != keyword_url->keyword())
-        it->allowed_to_be_default_match = false;
+  // Record the inlined suggestion (if any) for future use.
+  inlined_query_suggestion_match_contents_ = base::string16();
+  inlined_navsuggestion_ = GURL();
+  ACMatches::const_iterator first_match = FindTopMatch();
+  if ((first_match != matches_.end()) &&
+      !first_match->inline_autocompletion.empty()) {
+    // Identify if this match came from a query suggestion or a navsuggestion.
+    // In either case, extracts the identifying feature of the suggestion
+    // (query string or navigation url).
+    if (AutocompleteMatch::IsSearchType(first_match->type)) {
+      inlined_query_suggestion_match_contents_ = first_match->contents;
+    } else {
+      inlined_navsuggestion_ = first_match->destination_url;
     }
   }
+
   UpdateDone();
 }
 
 void SearchProvider::Run() {
   // Start a new request with the current input.
   suggest_results_pending_ = 0;
   time_suggest_request_sent_ = base::TimeTicks::Now();
 
   default_fetcher_.reset(CreateSuggestFetcher(kDefaultProviderURLFetcherID,
       providers_.GetDefaultProviderURL(), input_));
@@ skipping 70 matching lines @@
       (!default_results_.suggest_results.empty() ||
        !default_results_.navigation_results.empty() ||
        !keyword_results_.suggest_results.empty() ||
        !keyword_results_.navigation_results.empty() ||
        (!done_ && input_.want_asynchronous_matches())))
     return;
 
   // We can't keep running any previous query, so halt it.
   StopSuggest();
 
-  // Remove existing results that cannot inline autocomplete the new input.
-  RemoveAllStaleResults();
+  RemoveOrReviseOldResults(minimal_changes);
 
   // Update the content classifications of remaining results so they look good
   // against the current input.
   UpdateMatchContentsClass(input_.text(), &default_results_);
   if (!keyword_input_.text().empty())
     UpdateMatchContentsClass(keyword_input_.text(), &keyword_results_);
 
   // We can't start a new query if we're only allowed synchronous results.
   if (!input_.want_asynchronous_matches())
     return;
@@ skipping 63 matching lines @@
   // Don't send anything for https except the hostname.  Hostnames are OK
   // because they are visible when the TCP connection is established, but the
   // specific path may reveal private information.
   if (LowerCaseEqualsASCII(input_.scheme(), url::kHttpsScheme) &&
       parts.path.is_nonempty())
     return false;
 
   return true;
 }
 
-void SearchProvider::RemoveAllStaleResults() {
+void SearchProvider::RemoveOrReviseOldResults(bool minimal_changes) {
   if (keyword_input_.text().empty()) {
     // User is either in keyword mode with a blank input or out of
     // keyword mode entirely.
     keyword_results_.Clear();
   }
+  RemoveOrReviseOldResultsForOneProvider(minimal_changes, &default_results_);
+  RemoveOrReviseOldResultsForOneProvider(minimal_changes, &keyword_results_);
 }
 
-void SearchProvider::ApplyCalculatedRelevance() {
-  ApplyCalculatedSuggestRelevance(&keyword_results_.suggest_results);
-  ApplyCalculatedSuggestRelevance(&default_results_.suggest_results);
-  ApplyCalculatedNavigationRelevance(&keyword_results_.navigation_results);
-  ApplyCalculatedNavigationRelevance(&default_results_.navigation_results);
-  default_results_.verbatim_relevance = -1;
-  keyword_results_.verbatim_relevance = -1;
+void SearchProvider::PersistGoodResultsForOneProvider(

[msw, 2014/08/15 21:04:37]

Why do we need this at all? Isn't "RemoveOrReviseOldResultsForOneProvider"
sufficient? If this is needed, consider another function name that better
matches the operation here, like: PromoteAsyncInlineSuggestions or similar.

[Mark P, 2014/08/15 22:09:21]

This is definitely needed.  Without this function the behavior is correct when
typing "a", getting suggest results, adding "b", and then displaying cached
results (which we are allowed to inline).  But when the suggest reply for "b"
returns, we suddenly forget that we were inlining things before.  Hence, we need
this function to allow us to keep inlining what we were before.

I kind of like the current name.  PromoteAsyncInlineSuggestions() isn't right
because we're not promoting anything.  Renamed to a combination of the two:
PersistInlineSuggestions()

[msw, 2014/08/15 23:31:10]

The new name is good, thanks for explaining the need for this to me.

+    SearchSuggestionParser::Results* results) {
+  // Mark the result corresponding to the previous inline autocompletion as
+  // having been received on a previous keystroke (as indeed it must have

[msw, 2014/08/15 21:04:37]

nit: s/on a previous/before the last/

[Mark P, 2014/08/15 22:09:21]

Done.

+  // because we prohibit newly-received results from becoming the inline

[msw, 2014/08/15 21:04:37]

I don't know how this (indeed it must have...) comment explains why the result
must is guaranteed to have been received before the last keystroke... Wouldn't
that actually be contingent on when this function is called? Can you try to
clarify this parenthesized portion, maybe move it to the callsite, or perhaps
remove it altogether?

[Mark P, 2014/08/15 22:09:21]

Removed the comment.  I think the comment by the function declaration is clear
enough, especially in combination with the new function name.

+  // autocompletion).
+  if (!inlined_query_suggestion_match_contents_.empty()) {
+    for (SearchSuggestionParser::SuggestResults::iterator sug_it =
+             results->suggest_results.begin();
+         sug_it != results->suggest_results.end(); ++sug_it) {
+      if (sug_it->match_contents() ==
+          inlined_query_suggestion_match_contents_)
+        sug_it->set_received_after_last_keystroke(false);
+    }
+  }
+  if (inlined_navsuggestion_.is_valid()) {
+    for (SearchSuggestionParser::NavigationResults::iterator nav_it =
+             results->navigation_results.begin();
+         nav_it != results->navigation_results.end(); ++nav_it) {
+      if (nav_it->url() == inlined_navsuggestion_)
+        nav_it->set_received_after_last_keystroke(false);
+    }
+  }
 }
 
 void SearchProvider::ApplyCalculatedSuggestRelevance(
     SearchSuggestionParser::SuggestResults* list) {
   for (size_t i = 0; i < list->size(); ++i) {
     SearchSuggestionParser::SuggestResult& result = (*list)[i];
     result.set_relevance(
         result.CalculateRelevance(input_, providers_.has_keyword_provider()) +
         (list->size() - i - 1));
     result.set_relevance_from_server(false);
@@ skipping 76 matching lines @@
       keyword_results_.suggest_results.empty() ?
       TemplateURLRef::NO_SUGGESTIONS_AVAILABLE :
       TemplateURLRef::NO_SUGGESTION_CHOSEN;
 
   bool relevance_from_server;
   int verbatim_relevance = GetVerbatimRelevance(&relevance_from_server);
   int did_not_accept_default_suggestion =
       default_results_.suggest_results.empty() ?
       TemplateURLRef::NO_SUGGESTIONS_AVAILABLE :
       TemplateURLRef::NO_SUGGESTION_CHOSEN;
+  const TemplateURL* keyword_url = providers_.GetKeywordProviderURL();
   if (verbatim_relevance > 0) {
     const base::string16& trimmed_verbatim =
         base::CollapseWhitespace(input_.text(), false);
 
     // Verbatim results don't get suggestions and hence, answers.
     // Scan previous matches if the last answer-bearing suggestion matches
     // verbatim, and if so, copy over answer contents.
     base::string16 answer_contents;
     base::string16 answer_type;
     for (ACMatches::iterator it = matches_.begin(); it != matches_.end();
          ++it) {
       if (!it->answer_contents.empty() &&
           it->fill_into_edit == trimmed_verbatim) {
         answer_contents = it->answer_contents;
         answer_type = it->answer_type;
         break;
       }
     }
 
     SearchSuggestionParser::SuggestResult verbatim(
         trimmed_verbatim, AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED,
         trimmed_verbatim, base::string16(), base::string16(), answer_contents,
         answer_type, std::string(), std::string(), false, verbatim_relevance,
         relevance_from_server, false, trimmed_verbatim);
     AddMatchToMap(verbatim, std::string(), did_not_accept_default_suggestion,
-                  false, &map);
+                  false, keyword_url != NULL, &map);
   }
   if (!keyword_input_.text().empty()) {
-    const TemplateURL* keyword_url = providers_.GetKeywordProviderURL();
     // We only create the verbatim search query match for a keyword
     // if it's not an extension keyword.  Extension keywords are handled
     // in KeywordProvider::Start().  (Extensions are complicated...)
     // Note: in this provider, SEARCH_OTHER_ENGINE must correspond
     // to the keyword verbatim search query.  Do not create other matches
     // of type SEARCH_OTHER_ENGINE.
     if (keyword_url &&
         (keyword_url->GetType() != TemplateURL::OMNIBOX_API_EXTENSION)) {
       bool keyword_relevance_from_server;
       const int keyword_verbatim_relevance =
           GetKeywordVerbatimRelevance(&keyword_relevance_from_server);
       if (keyword_verbatim_relevance > 0) {
         const base::string16& trimmed_verbatim =
             base::CollapseWhitespace(keyword_input_.text(), false);
         SearchSuggestionParser::SuggestResult verbatim(
             trimmed_verbatim, AutocompleteMatchType::SEARCH_OTHER_ENGINE,
             trimmed_verbatim, base::string16(), base::string16(),
             base::string16(), base::string16(), std::string(), std::string(),
             true, keyword_verbatim_relevance, keyword_relevance_from_server,
             false, trimmed_verbatim);
         AddMatchToMap(verbatim, std::string(),
-                      did_not_accept_keyword_suggestion, false, &map);
+                      did_not_accept_keyword_suggestion, false, true, &map);
       }
     }
   }
   AddHistoryResultsToMap(keyword_history_results_, true,
                          did_not_accept_keyword_suggestion, &map);
   AddHistoryResultsToMap(default_history_results_, false,
                          did_not_accept_default_suggestion, &map);
 
   AddSuggestResultsToMap(keyword_results_.suggest_results,
                          keyword_results_.metadata, &map);
   AddSuggestResultsToMap(default_results_.suggest_results,
                          default_results_.metadata, &map);
 
   ACMatches matches;
   for (MatchMap::const_iterator i(map.begin()); i != map.end(); ++i)
     matches.push_back(i->second);
 
   AddNavigationResultsToMatches(keyword_results_.navigation_results, &matches);
   AddNavigationResultsToMatches(default_results_.navigation_results, &matches);
 
   // Now add the most relevant matches to |matches_|.  We take up to kMaxMatches
-  // suggest/navsuggest matches, regardless of origin.  If Instant Extended is
-  // enabled and we have server-provided (and thus hopefully more accurate)
-  // scores for some suggestions, we allow more of those, until we reach
+  // suggest/navsuggest matches, regardless of origin.  We always include in
+  // that set a legal default match if possible.  If Instant Extended is enabled
+  // and we have server-provided (and thus hopefully more accurate) scores for
+  // some suggestions, we allow more of those, until we reach
   // AutocompleteResult::kMaxMatches total matches (that is, enough to fill the
   // whole popup).
   //
   // We will always return any verbatim matches, no matter how we obtained their
   // scores, unless we have already accepted AutocompleteResult::kMaxMatches
   // higher-scoring matches under the conditions above.
   std::sort(matches.begin(), matches.end(), &AutocompleteMatch::MoreRelevant);
   matches_.clear();
+  // Guarantee that if there's a legal default match anywhere in the result

[msw, 2014/08/15 21:04:37]

Why is this needed now? I thought the |allowed_to_be_default_match| behavior
made this type of provider-side fixup unnecessary.

[Mark P, 2014/08/15 22:09:21]

This is needed because AutocompleteResult enforces the behavior: find the top
default match and pull it to the top.  However, it can only do that if there is
a default match to be found.  This change makes sure SearchProvider returns one.
 Previously, we trusted SearchProvider to return one by clobbering relevance
scores willy-nilly and then relying on local scoring having the default match
beat the suggestions.  Now we return the top default match regardless of its
relevance score.  We don't need to clobber relevance scores as wildly as before.

[msw, 2014/08/15 23:31:10]

My question is "why do we need to rotate the matches list?". I guess it's
because the constraint enforcement might now leave the "top match"
(highest-scored-inlinable match) scored lowly and ordered lower down the
|matches| list than kMaxMatches, so the step below would otherwise omit the top
result. Then, wouldn't rotating the list potentially use more lowly
ranked/ordered results instead of the prior sorting? If so, how is that not
caught by a unit test? (ie. a test should ensure that we have an inlinable
match, even if it's the lowest-ranked of N>kMaxMatches results). Instead of
rotating, can we just yank the "top match" to the top of the |matches| list?

[Mark P, 2014/08/15 23:50:15]

This call to rotate does a remove from current location and insert at the
beginning.  In other word, it just yanks it (as you request).  Made more
explicit in the comment.

+  // set that it'll get returned by moving the default match to the front
+  // of the list.
+  ACMatches::iterator default_match = FindTopMatch(&matches);
+  if (default_match != matches.end()) {

[msw, 2014/08/15 21:04:37]

nit: remove curly braces.

[Mark P, 2014/08/15 22:09:21]

Done.

+    std::rotate(matches.begin(), default_match, default_match + 1);
+  }
 
   size_t num_suggestions = 0;
   for (ACMatches::const_iterator i(matches.begin());
        (i != matches.end()) &&
            (matches_.size() < AutocompleteResult::kMaxMatches);
        ++i) {
     // SEARCH_OTHER_ENGINE is only used in the SearchProvider for the keyword
     // verbatim result, so this condition basically means "if this match is a
     // suggestion of some sort".
     if ((i->type != AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED) &&
         (i->type != AutocompleteMatchType::SEARCH_OTHER_ENGINE)) {
       // If we've already hit the limit on non-server-scored suggestions, and
       // this isn't a server-scored suggestion we can add, skip it.
       if ((num_suggestions >= kMaxMatches) &&
           (!chrome::IsInstantExtendedAPIEnabled() ||
            (i->GetAdditionalInfo(kRelevanceFromServerKey) != kTrue))) {
         continue;
       }
 
       ++num_suggestions;
     }
 
     matches_.push_back(*i);
   }
   UMA_HISTOGRAM_TIMES("Omnibox.SearchProvider.ConvertResultsTime",
                       base::TimeTicks::Now() - start_time);
 }
 
 ACMatches::const_iterator SearchProvider::FindTopMatch() const {

[msw, 2014/08/15 21:04:38]

Remove this, use FindTopMatch(&matches_) at callsites.

[Mark P, 2014/08/15 22:09:21]

I can't do that because some callsites are const functions and the new
FindTopMatch I created (needs to) return a non-const iterator.  I don't want to
have to remove consts from various functions or do a const-cast.  Instead, I'm
left with the (chosen, in my opinion least bad) option of semi-duplicating (not
exactly duplicates because of the const_iterator) fragment of code.

I'm open to being pushed in a different direction.
(or having this cleaned up later)

[msw, 2014/08/15 23:31:10]

I guess leave the duplication for now, unless you can remove the std::rotate
step, and thus no longer need a non-const iterator.

[Mark P, 2014/08/15 23:50:15]

Acknowledged.
(I can't remove the rotate call.)

   ACMatches::const_iterator it = matches_.begin();
   while ((it != matches_.end()) && !it->allowed_to_be_default_match)
     ++it;
   return it;
 }
 
-bool SearchProvider::HasKeywordDefaultMatchInKeywordMode() const {

[Mark P, 2014/08/15 20:04:38]

Deleted in refactoring changelist.

-  const TemplateURL* keyword_url = providers_.GetKeywordProviderURL();
-  // If the user is not in keyword mode, return true to say that this
-  // constraint is not violated.
-  if (keyword_url == NULL)
-    return true;
-  for (ACMatches::const_iterator it = matches_.begin(); it != matches_.end();
-       ++it) {
-    if ((it->keyword == keyword_url->keyword()) &&
-        it->allowed_to_be_default_match)
-      return true;
-  }
-  return false;
-}
-
 bool SearchProvider::IsTopMatchSearchWithURLInput() const {
   ACMatches::const_iterator first_match = FindTopMatch();
   return (input_.type() == metrics::OmniboxInputType::URL) &&
       (first_match != matches_.end()) &&
       (first_match->relevance > CalculateRelevanceForVerbatim()) &&
       (first_match->type != AutocompleteMatchType::NAVSUGGEST) &&
       (first_match->type != AutocompleteMatchType::NAVSUGGEST_PERSONALIZED);
 }
 
 void SearchProvider::AddNavigationResultsToMatches(
@@ skipping 39 matching lines @@
     if ((scored_results.front().relevance() < 1200) ||
         !HasMultipleWords(scored_results.front().suggestion()))
       scored_results.clear();  // Didn't detect the case above, score normally.
   }
   if (scored_results.empty())
     scored_results = ScoreHistoryResults(results, prevent_inline_autocomplete,
                                          input_multiple_words, input_text,
                                          is_keyword);
   for (SearchSuggestionParser::SuggestResults::const_iterator i(
            scored_results.begin()); i != scored_results.end(); ++i) {
-    AddMatchToMap(*i, std::string(), did_not_accept_suggestion, true, map);
+    AddMatchToMap(*i, std::string(), did_not_accept_suggestion, true,
+                  providers_.GetKeywordProviderURL() != NULL, map);
   }
   UMA_HISTOGRAM_TIMES("Omnibox.SearchProvider.AddHistoryResultsTime",
                       base::TimeTicks::Now() - start_time);
 }
 
 SearchSuggestionParser::SuggestResults SearchProvider::ScoreHistoryResults(
     const HistoryResults& results,
     bool base_prevent_inline_autocomplete,
     bool input_multiple_words,
     const base::string16& input_text,
@@ skipping 22 matching lines @@
         prevent_search_history_inlining);
     // Add the match to |scored_results| by putting the what-you-typed match
     // on the front and appending all other matches.  We want the what-you-
     // typed match to always be first.
     SearchSuggestionParser::SuggestResults::iterator insertion_position =
         scored_results.end();
     if (trimmed_suggestion == trimmed_input) {
       found_what_you_typed_match = true;
       insertion_position = scored_results.begin();
     }
-    scored_results.insert(
-        insertion_position,
-        SearchSuggestionParser::SuggestResult(
-            trimmed_suggestion, AutocompleteMatchType::SEARCH_HISTORY,
-            trimmed_suggestion, base::string16(), base::string16(),
-            base::string16(), base::string16(), std::string(), std::string(),
-            is_keyword, relevance, false, false, trimmed_input));
+    SearchSuggestionParser::SuggestResult history_suggestion(
+        trimmed_suggestion, AutocompleteMatchType::SEARCH_HISTORY,
+        trimmed_suggestion, base::string16(), base::string16(),
+        base::string16(), base::string16(), std::string(), std::string(),
+        is_keyword, relevance, false, false, trimmed_input);
+    // History results are synchronous; they are received at the last keystroke,

[msw, 2014/08/15 21:04:37]

nit: s/upon/with/ remove ", not after it"

[Mark P, 2014/08/15 22:09:21]

Did the latter.

The former fails to apply.   There is no "upon" in the comment.  Left the
comment with "at" because I think it's better than "with".

[msw, 2014/08/15 23:31:10]

I meant to encourage "upon the last keystroke", but what you have is fine.

[Mark P, 2014/08/15 23:50:15]

Switched to "upon".

+    // not after it.
+    history_suggestion.set_received_after_last_keystroke(false);
+    scored_results.insert(insertion_position, history_suggestion);
   }
 
   // History returns results sorted for us.  However, we may have docked some
   // results' scores, so things are no longer in order.  While keeping the
   // what-you-typed match at the front (if it exists), do a stable sort to get
   // things back in order without otherwise disturbing results with equal
   // scores, then force the scores to be unique, so that the order in which
   // they're shown is deterministic.
   std::stable_sort(scored_results.begin() +
                        (found_what_you_typed_match ? 1 : 0),
@@ skipping 52 matching lines @@
     last_relevance = i->relevance();
   }
 
   return scored_results;
 }
 
 void SearchProvider::AddSuggestResultsToMap(
     const SearchSuggestionParser::SuggestResults& results,
     const std::string& metadata,
     MatchMap* map) {
-  for (size_t i = 0; i < results.size(); ++i)
-    AddMatchToMap(results[i], metadata, i, false, map);
+  for (size_t i = 0; i < results.size(); ++i) {
+    AddMatchToMap(results[i], metadata, i, false,
+                  providers_.GetKeywordProviderURL() != NULL, map);
+  }
 }
 
 int SearchProvider::GetVerbatimRelevance(bool* relevance_from_server) const {
   // Use the suggested verbatim relevance score if it is non-negative (valid),
   // if inline autocomplete isn't prevented (always show verbatim on backspace),
   // and if it won't suppress verbatim, leaving no default provider matches.
   // Otherwise, if the default provider returned no matches and was still able
   // to suppress verbatim, the user would have no search/nav matches and may be
   // left unable to search using their default provider from the omnibox.
   // Check for results on each verbatim calculation, as results from older
@@ skipping 138 matching lines @@
     if (inline_autocomplete_offset != base::string16::npos)
       ++inline_autocomplete_offset;
   }
   if (inline_autocomplete_offset != base::string16::npos) {
     DCHECK(inline_autocomplete_offset <= match.fill_into_edit.length());
     match.inline_autocompletion =
         match.fill_into_edit.substr(inline_autocomplete_offset);
   }
   // An inlineable navsuggestion can only be the default match when there
   // is no keyword provider active, lest it appear first and break the user
-  // out of keyword mode.  It can also only be default if either the inline
+  // out of keyword mode.  We also must have received the navsuggestion before
+  // the last keystroke.  (We don't want asynchronous inline autocompletions.)
+  // The navsuggestion can also only be default if either the inline
   // autocompletion is empty or we're not preventing inline autocompletion.
   // Finally, if we have an inlineable navsuggestion with an inline completion
   // that we're not preventing, make sure we didn't trim any whitespace.
   // We don't want to claim http://foo.com/bar is inlineable against the
   // input "foo.com/b ".
-  match.allowed_to_be_default_match = (prefix != NULL) &&
+  match.allowed_to_be_default_match =
+      (prefix != NULL) &&
       (providers_.GetKeywordProviderURL() == NULL) &&
+      !navigation.received_after_last_keystroke() &&
       (match.inline_autocompletion.empty() ||
       (!input_.prevent_inline_autocomplete() && !trimmed_whitespace));
   match.EnsureUWYTIsAllowedToBeDefault(
       input_.canonicalized_url(), providers_.template_url_service());
 
   match.contents = navigation.match_contents();
   match.contents_class = navigation.match_contents_class();
   match.description = navigation.description();
   AutocompleteMatch::ClassifyMatchInString(input, match.description,
       ACMatchClassification::NONE, &match.description_class);
@@ skipping 52 matching lines @@
   last_answer_seen_.query_type = match->answer_type;
 }
 
 void SearchProvider::DoAnswersQuery(const AutocompleteInput& input) {
   // If the query text starts with trimmed input, this is valid prefetch data.
   prefetch_data_ = StartsWith(last_answer_seen_.full_query_text,
                               base::CollapseWhitespace(input.text(), false),
                               false) ?
       last_answer_seen_ : AnswersQueryData();
 }