Omnibox: Prevent Asynchronous Suggestions from Changing Default Match

chrome/browser/autocomplete/base_search_provider.cc

 // Copyright 2014 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/base_search_provider.h"
 
 #include "base/i18n/case_conversion.h"
 #include "base/prefs/pref_registry_simple.h"
 #include "base/prefs/pref_service.h"
 #include "base/strings/string_util.h"
@@ skipping 237 matching lines @@
       (base::CollapseWhitespace(input.text(), false) ==
        suggestion.match_contents());
 
   // When the user forced a query, we need to make sure all the fill_into_edit
   // values preserve that property.  Otherwise, if the user starts editing a
   // suggestion, non-Search results will suddenly appear.
   if (input.type() == metrics::OmniboxInputType::FORCED_QUERY)
     match.fill_into_edit.assign(base::ASCIIToUTF16("?"));
   if (suggestion.from_keyword_provider())
     match.fill_into_edit.append(match.keyword + base::char16(' '));
+  // We only allow inlinable navsuggestions that were received before the

[msw, 2014/08/15 21:27:59]

Do we actually want to check received_after_last_keystroke() here, not assign
the inline_autocompletion for potential later use, and set
allowed_to_be_default_match in a way that might hinder subsequent passes? It
seems like we should only enforce !received_after_last_keystroke with the other
constraints enforcement, (or with the higher-level default match selection?).

If this *is* the right place to do this, why isn't it also done above with the
match.allowed_to_be_default_match assignment on line 246?

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

Line 246 is fine.  Verbatim matches are always allow to be default.  It makes
little sense to ask when they were returned.  (I think you asked about this
earlier.)
I'd rather not do late enforcement.  It's kind of a pain.  I think if we know
we're not going to allow something to be the default match, then we shouldn't
fill in information like inline_autocomplete because it'll be misleading.  Just
because we (you) think in the future that we may want to renege on this
allow-to-be-default-match decision and want to correct it later in the code
flow, doesn't mean we should build it late correction when it's not currently
necessary.

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

Ah, I get it now, I mistakenly thought that marking this *match* as
|allowed_to_be_default_match == false| would prevent the re-used suggest
*result* from being inlined after the next keystroke.

+  // last keystroke because we don't want asynchronous inline autocompletions.
   if (!input.prevent_inline_autocomplete() &&
+      !suggestion.received_after_last_keystroke() &&
       (!in_keyword_mode || suggestion.from_keyword_provider()) &&
       StartsWith(suggestion.suggestion(), input.text(), false)) {
     match.inline_autocompletion =
         suggestion.suggestion().substr(input.text().length());
     match.allowed_to_be_default_match = true;
   }
   match.fill_into_edit.append(suggestion.suggestion());
 
   const TemplateURLRef& search_url = template_url->url_ref();
   DCHECK(search_url.SupportsReplacement(search_terms_data));
@@ skipping 225 matching lines @@
 }
 
 void BaseSearchProvider::OnDeletionComplete(
     bool success, SuggestionDeletionHandler* handler) {
   RecordDeletionResult(success);
   SuggestionDeletionHandlers::iterator it = std::find(
       deletion_handlers_.begin(), deletion_handlers_.end(), handler);
   DCHECK(it != deletion_handlers_.end());
   deletion_handlers_.erase(it);
 }