Don't call AutocompleteInput::Parse() on a background thread, part 2.

chrome/browser/autocomplete/history_url_provider.cc

 // Copyright (c) 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/history_url_provider.h"
 
 #include <algorithm>
 
 #include "base/basictypes.h"
 #include "base/bind.h"
@@ skipping 399 matching lines @@
     const AutocompleteMatch& what_you_typed_match,
     const std::string& languages,
     TemplateURL* default_search_provider,
     const SearchTermsData& search_terms_data)
     : message_loop(base::MessageLoop::current()),
       input(input),
       prevent_inline_autocomplete(input.prevent_inline_autocomplete()),
       trim_http(trim_http),
       what_you_typed_match(what_you_typed_match),
       failed(false),
+      exact_suggestion_is_in_history(false),
+      promote_type(NEITHER),
       languages(languages),
-      dont_suggest_exact_input(false),
       default_search_provider(default_search_provider ?
           new TemplateURL(default_search_provider->data()) : NULL),
       search_terms_data(new SearchTermsDataSnapshot(search_terms_data)) {
 }
 
 HistoryURLProviderParams::~HistoryURLProviderParams() {
 }
 
 HistoryURLProvider::HistoryURLProvider(AutocompleteProviderListener* listener,
                                        Profile* profile)
@@ skipping 88 matching lines @@
   history::URLDatabase* url_db = history_service->InMemoryDatabase();
   // url_db can be NULL if it hasn't finished initializing (or failed to
   // initialize).  In this case all we can do is fall back on the second
   // pass.
   //
   // TODO(pkasting): We should just block here until this loads.  Any time
   // someone unloads the history backend, we'll get inconsistent inline
   // autocomplete behavior here.
   if (url_db) {
     DoAutocomplete(NULL, url_db, params.get());
-    // params->matches now has the matches we should expose to the provider.
-    // Pass 2 expects a "clean slate" set of matches.
     matches_.clear();
-    matches_.swap(params->matches);
+    PromoteMatchIfNecessary(*params);
     UpdateStarredStateOfMatches();
     // Reset the WYT match in |params| so that both passes get the same input
     // state, since DoAutocomplete() may have modified it.
     params->what_you_typed_match = what_you_typed_match;

[Mark P, 2014/06/19 03:53:14]

I think you should explicitly comment something like
// All other variables (but one) in |params| were either not changed in
DoAutocomplete() or will be overridden regardless in DoAutocomplete().  The one
exception is promote_type, which we use to pass information about what the first
pass did to the second.

Consider also adding one or more comments to the data structure in the header. 
Maybe a comment by the declaration of promote_type would be appropriate to
mention its message-passing nature.

[Peter Kasting, 2014/06/19 20:55:22]

I removed this as discussed in person.  I also added a comment here about not
resetting |params|, and why.
Yep, added.

   }
 
   // Pass 2: Ask the history service to call us back on the history thread,
   // where we can read the full on-disk DB.
   if (input.want_asynchronous_matches()) {
     done_ = false;
     params_ = params.release();  // This object will be destroyed in
                                  // QueryComplete() once we're done with it.
     history_service->ScheduleAutocomplete(this, params_);
   }
@@ skipping 134 matching lines @@
           description_matches, offsets, description_word_starts, 0,
           std::string::npos);
   return SpansFromTermMatch(
       description_matches, clean_description.length(), false);
 }
 
 void HistoryURLProvider::DoAutocomplete(history::HistoryBackend* backend,
                                         history::URLDatabase* db,
                                         HistoryURLProviderParams* params) {
   // Get the matching URLs from the DB.
+  params->matches.clear();
   history::URLRows url_matches;
-  history::HistoryMatches history_matches;
-
   const URLPrefixes& prefixes = URLPrefix::GetURLPrefixes();
   for (URLPrefixes::const_iterator i(prefixes.begin()); i != prefixes.end();
        ++i) {
     if (params->cancel_flag.IsSet())
       return;  // Canceled in the middle of a query, give up.
 
     // We only need kMaxMatches results in the end, but before we get there we
     // need to promote lower-quality matches that are prefixes of higher-quality
     // matches, and remove lower-quality redirects.  So we ask for more results
     // than we need, of every prefix type, in hopes this will give us far more
     // than enough to work with.  CullRedirects() will then reduce the list to
     // the best kMaxMatches results.
     db->AutocompleteForPrefix(
         base::UTF16ToUTF8(i->prefix + params->input.text()), kMaxMatches * 2,
         !backend, &url_matches);
     for (history::URLRows::const_iterator j(url_matches.begin());
           j != url_matches.end(); ++j) {
       const URLPrefix* best_prefix = URLPrefix::BestURLPrefix(
           base::UTF8ToUTF16(j->url().spec()), base::string16());
       DCHECK(best_prefix);
-      history_matches.push_back(history::HistoryMatch(
+      params->matches.push_back(history::HistoryMatch(
           *j, i->prefix.length(), !i->num_components,
           i->num_components >= best_prefix->num_components));
     }
   }
 
   // Create sorted list of suggestions.
-  CullPoorMatches(*params, &history_matches);
-  SortAndDedupMatches(&history_matches);
+  CullPoorMatches(params);
+  SortAndDedupMatches(&params->matches);
 
   // Try to create a shorter suggestion from the best match.
   // We allow the what you typed match to be displayed when there's a reasonable
   // chance the user actually cares:
   // * Their input can be opened as a URL, and
   // * We parsed the input as a URL, or it starts with an explicit "http:" or
   //   "https:".
   // Otherwise, this is just low-quality noise.  In the cases where we've parsed
   // as UNKNOWN, we'll still show an accidental search infobar if need be.
   VisitClassifier classifier(this, params->input, db);
   bool have_what_you_typed_match =
       (params->input.type() != metrics::OmniboxInputType::QUERY) &&
       ((params->input.type() != metrics::OmniboxInputType::UNKNOWN) ||
        (classifier.type() == VisitClassifier::UNVISITED_INTRANET) ||
        !params->trim_http ||
        (AutocompleteInput::NumNonHostComponents(params->input.parts()) > 0));
-  PromoteOrCreateShorterSuggestion(db, *params, have_what_you_typed_match,
-                                   &history_matches);
+  PromoteOrCreateShorterSuggestion(db, have_what_you_typed_match, params);
 
-  // Try to promote a match as an exact/inline autocomplete match.  This also
-  // moves it to the front of |history_matches|, so skip over it when
-  // converting the rest of the matches.
-  size_t first_match = 1;
-  size_t exact_suggestion = 0;
-  // Checking params->what_you_typed_match.is_history_what_you_typed_match tells
-  // us whether SuggestExactInput() succeeded in constructing a valid match.
-  if (params->what_you_typed_match.is_history_what_you_typed_match &&
-      (!backend || !params->dont_suggest_exact_input) &&
-      FixupExactSuggestion(db, classifier, params, &history_matches)) {
+  // Try to promote a match as an exact/inline autocomplete match.
+  // Checking what_you_typed_match.is_history_what_you_typed_match tells us

[Mark P, 2014/06/19 03:53:14]

How about inserting a sentence here to give context for this explanation.  The
"Try to promote" is too broad.  It doesn't give enough context, and then you
give into the details.  Perhaps something like
"We need to know whether we should consider the exact suggestion as being in the
user's history and therefore promote it accordingly."

(note also the added emphasis here on "considered to be in history", because the
latter part of the explanation is elaborating on precisely this nuance.

[Peter Kasting, 2014/06/19 20:55:22]

I reorganized the comments here a bit.  I took the unusual step of placing
comments in the midst of the conditional, because I think it's clearer in this
case to do so.  This also addresses your next few comments.

+  // whether SuggestExactInput() succeeded in constructing a valid match.
+  // Additionally, in the case where the user has typed "foo.com" and visited
+  // (but not typed) "foo/", and the input is "foo", the first pass will fall
+  // into the FRONT_HISTORY_MATCH case for "foo.com" but the second pass can

[Mark P, 2014/06/19 03:53:14]

nit:
case
->
case below

to make it obvious this long comment is referring to a particular nearby line of
code.

+  // suggest the exact input as a better URL.  Since we need both passes to
+  // agree, and since during the first pass there's no way to know about "foo/",
+  // ensure that if the promote type was set to FRONT_HISTORY_MATCH during the
+  // first pass, that should prevent this code from suggesting the exact input
+  // as a better match (during the second pass).
+  params->exact_suggestion_is_in_history =
+      params->what_you_typed_match.is_history_what_you_typed_match &&
+      (!backend ||

[Mark P, 2014/06/19 03:53:14]

consider comment:   // on first pass

+       (params->promote_type !=
+           HistoryURLProviderParams::FRONT_HISTORY_MATCH)) &&

[Mark P, 2014/06/19 03:53:14]

consider comment:  // first pass did not inline autocomplete a URL

+      FixupExactSuggestion(db, classifier, params);
+  if (params->exact_suggestion_is_in_history) {
     // Got an exact match for the user's input.  Treat it as the best match
     // regardless of the input type.
-    exact_suggestion = 1;
-    params->matches.push_back(params->what_you_typed_match);
-  } else if (params->prevent_inline_autocomplete ||
-      history_matches.empty() ||
-      !PromoteMatchForInlineAutocomplete(history_matches.front(), params)) {
+    params->promote_type = HistoryURLProviderParams::WHAT_YOU_TYPED_MATCH;
+  } else if (!params->prevent_inline_autocomplete && !params->matches.empty() &&
+      CanPromoteMatchForInlineAutocomplete(params->matches[0])) {
+    params->promote_type = HistoryURLProviderParams::FRONT_HISTORY_MATCH;
+  } else if (have_what_you_typed_match) {
     // Failed to promote any URLs for inline autocompletion.  Use the What You
     // Typed match, if we have it.
-    first_match = 0;
-    if (have_what_you_typed_match)
-      params->matches.push_back(params->what_you_typed_match);
+    params->promote_type = have_what_you_typed_match ?
+        HistoryURLProviderParams::WHAT_YOU_TYPED_MATCH :
+        HistoryURLProviderParams::NEITHER;
   }
 
-  // This is the end of the synchronous pass.
-  if (!backend)
-    return;
-
-  // Determine relevancy of highest scoring match, if any.
-  int relevance = -1;
-  for (ACMatches::const_iterator it = params->matches.begin();
-       it != params->matches.end(); ++it) {
-    relevance = std::max(relevance, it->relevance);
-  }
-
-  if (cull_redirects_) {
+  const size_t max_results =
+      kMaxMatches + (params->exact_suggestion_is_in_history ? 1 : 0);
+  if (backend && cull_redirects_) {
     // Remove redirects and trim list to size.  We want to provide up to
     // kMaxMatches results plus the What You Typed result, if it was added to
-    // |history_matches| above.
-    CullRedirects(backend, &history_matches, kMaxMatches + exact_suggestion);
-  } else {
+    // params->matches above.
+    CullRedirects(backend, &params->matches, max_results);
+  } else if (params->matches.size() > max_results) {
     // Simply trim the list to size.
-    if (history_matches.size() > kMaxMatches + exact_suggestion)
-      history_matches.resize(kMaxMatches + exact_suggestion);
-  }
-
-  // Convert the history matches to autocomplete matches.
-  for (size_t i = first_match; i < history_matches.size(); ++i) {
-    const history::HistoryMatch& match = history_matches[i];
-    DCHECK(!have_what_you_typed_match ||
-           (match.url_info.url() !=
-            GURL(params->matches.front().destination_url)));
-    // If we've assigned a score already, all later matches score one
-    // less than the previous match.
-    relevance = (relevance > 0) ?
-        (relevance - 1) :
-        CalculateRelevance(NORMAL,
-                           static_cast<int>(history_matches.size() - 1 - i));
-    AutocompleteMatch ac_match = HistoryMatchToACMatch(*params, match,
-        NORMAL, relevance);
-    // The experimental scoring must not change the top result's score.
-    if (!params->matches.empty()) {
-      relevance = CalculateRelevanceScoreUsingScoringParams(match, relevance,
-                                                            scoring_params_);
-      ac_match.relevance = relevance;
-    }
-    params->matches.push_back(ac_match);
+    params->matches.resize(max_results);
   }
 }
 
+void HistoryURLProvider::PromoteMatchIfNecessary(
+    const HistoryURLProviderParams& params) {
+  if (params.promote_type == HistoryURLProviderParams::NEITHER)
+    return;
+  matches_.push_back(
+      (params.promote_type == HistoryURLProviderParams::WHAT_YOU_TYPED_MATCH) ?
+          params.what_you_typed_match :
+          HistoryMatchToACMatch(params, 0, INLINE_AUTOCOMPLETE,
+                                CalculateRelevance(INLINE_AUTOCOMPLETE, 0)));
+}
+
 void HistoryURLProvider::QueryComplete(
     HistoryURLProviderParams* params_gets_deleted) {
   // Ensure |params_gets_deleted| gets deleted on exit.
   scoped_ptr<HistoryURLProviderParams> params(params_gets_deleted);
 
   // If the user hasn't already started another query, clear our member pointer
   // so we can't write into deleted memory.
   if (params_ == params_gets_deleted)
     params_ = NULL;
 
   // Don't send responses for queries that have been canceled.
   if (params->cancel_flag.IsSet())
     return;  // Already set done_ when we canceled, no need to set it again.
 
   // Don't modify |matches_| if the query failed, since it might have a default
   // match in it, whereas |params->matches| will be empty.
   if (!params->failed) {
-    matches_.swap(params->matches);
+    matches_.clear();
+    PromoteMatchIfNecessary(*params);
+
+    // If we promoted the first match, skip over it when converting the rest of
+    // the matches.

[Mark P, 2014/06/19 03:53:14]

side comment: I really wish we didn't have the .promoted field on matches, which
doesn't actually mean the same thing as promoted in this sense.  (.promoted only
comes from whether it was promoted by PromoteOrCreateShorterSuggestion().)

[Peter Kasting, 2014/06/19 20:55:22]

I found a way to eliminate this member by adding a return value to
PromoteOrCreateShorterSuggestion().

+    const size_t first_match =
+        (params->exact_suggestion_is_in_history ||
+         (params->promote_type ==
+             HistoryURLProviderParams::FRONT_HISTORY_MATCH)) ? 1 : 0;
+
+    // Determine relevance of highest scoring match, if any.
+    int relevance = matches_.empty() ?
+        CalculateRelevance(
+            NORMAL,
+            static_cast<int>(params->matches.size() - 1 - first_match)) :

[Mark P, 2014/06/19 03:53:14]

I think "- first_match" is unnecessary.  If matches is empty, first_match will
be zero.

[Peter Kasting, 2014/06/19 20:55:22]

You're right, though it took me some examination to be sure.

+        matches_[0].relevance;
+
+    // Convert the history matches to autocomplete matches.
+    for (size_t i = first_match; i < params->matches.size(); ++i) {
+      // All matches score one less than the previous match.
+      --relevance;
+      // The experimental scoring must not change the top result's score.
+      if (!matches_.empty()) {
+        relevance = CalculateRelevanceScoreUsingScoringParams(
+            params->matches[i], relevance, scoring_params_);
+      }
+      matches_.push_back(HistoryMatchToACMatch(*params, i, NORMAL, relevance));
+    }
+
     UpdateStarredStateOfMatches();
   }
 
   done_ = true;
   listener_->OnProviderUpdate(true);
 }
 
 bool HistoryURLProvider::FixupExactSuggestion(
     history::URLDatabase* db,
     const VisitClassifier& classifier,
-    HistoryURLProviderParams* params,
-    history::HistoryMatches* matches) const {
-  DCHECK(matches != NULL);
-
+    HistoryURLProviderParams* params) const {
   MatchType type = INLINE_AUTOCOMPLETE;
   switch (classifier.type()) {
     case VisitClassifier::INVALID:
       return false;
     case VisitClassifier::UNVISITED_INTRANET:
       type = UNVISITED_INTRANET;
       break;
     default:
       DCHECK_EQ(VisitClassifier::VISITED, classifier.type());
       // We have data for this match, use it.
@@ skipping 42 matching lines @@
     return false;
   }
 
   params->what_you_typed_match.relevance = CalculateRelevance(type, 0);
 
   // If there are any other matches, then don't promote this match here, in
   // hopes the caller will be able to inline autocomplete a better suggestion.
   // DoAutocomplete() will fall back on this match if inline autocompletion
   // fails.  This matches how we react to never-visited URL inputs in the non-
   // intranet case.
-  if (type == UNVISITED_INTRANET && !matches->empty())
+  if (type == UNVISITED_INTRANET && !params->matches.empty())
     return false;
 
   // Put it on the front of the HistoryMatches for redirect culling.
   CreateOrPromoteMatch(classifier.url_row(), base::string16::npos, false,
-                       matches, true, true);
+                       &params->matches, true, true);
   return true;
 }
 
 bool HistoryURLProvider::CanFindIntranetURL(
     history::URLDatabase* db,
     const AutocompleteInput& input) const {
   // Normally passing the first two conditions below ought to guarantee the
   // third condition, but because FixupUserInput() can run and modify the
   // input's text and parts between Parse() and here, it seems better to be
   // paranoid and check.
   if ((input.type() != metrics::OmniboxInputType::UNKNOWN) ||
       !LowerCaseEqualsASCII(input.scheme(), url::kHttpScheme) ||
       !input.parts().host.is_nonempty())
     return false;
   const std::string host(base::UTF16ToUTF8(
       input.text().substr(input.parts().host.begin, input.parts().host.len)));
   const size_t registry_length =
       net::registry_controlled_domains::GetRegistryLength(
           host,
           net::registry_controlled_domains::EXCLUDE_UNKNOWN_REGISTRIES,
           net::registry_controlled_domains::EXCLUDE_PRIVATE_REGISTRIES);
   return registry_length == 0 && db->IsTypedHost(host);
 }
 
-bool HistoryURLProvider::PromoteMatchForInlineAutocomplete(
-    const history::HistoryMatch& match,
-    HistoryURLProviderParams* params) {
-  if (!CanPromoteMatchForInlineAutocomplete(match))
-    return false;
-
-  // In the case where the user has typed "foo.com" and visited (but not typed)
-  // "foo/", and the input is "foo", we can reach here for "foo.com" during the
-  // first pass but have the second pass suggest the exact input as a better
-  // URL.  Since we need both passes to agree, and since during the first pass
-  // there's no way to know about "foo/", make reaching this point prevent any
-  // future pass from suggesting the exact input as a better match.
-  params->dont_suggest_exact_input = true;
-  params->matches.push_back(HistoryMatchToACMatch(
-      *params, match, INLINE_AUTOCOMPLETE,
-      CalculateRelevance(INLINE_AUTOCOMPLETE, 0)));
-  return true;
-}
-
 void HistoryURLProvider::PromoteOrCreateShorterSuggestion(
     history::URLDatabase* db,
-    const HistoryURLProviderParams& params,
     bool have_what_you_typed_match,
-    history::HistoryMatches* matches) {
-  if (matches->empty())
+    HistoryURLProviderParams* params) {
+  if (params->matches.empty())
     return;  // No matches, nothing to do.
 
   // Determine the base URL from which to search, and whether that URL could
   // itself be added as a match.  We can add the base iff it's not "effectively
   // the same" as any "what you typed" match.
-  const history::HistoryMatch& match = matches->front();
-  GURL search_base = ConvertToHostOnly(match, params.input.text());
+  const history::HistoryMatch& match = params->matches[0];
+  GURL search_base = ConvertToHostOnly(match, params->input.text());
   bool can_add_search_base_to_matches = !have_what_you_typed_match;
   if (search_base.is_empty()) {
     // Search from what the user typed when we couldn't reduce the best match
     // to a host.  Careful: use a substring of |match| here, rather than the
     // first match in |params|, because they might have different prefixes.  If
     // the user typed "google.com", params->what_you_typed_match will hold
     // "http://google.com/", but |match| might begin with
     // "http://www.google.com/".
     // TODO: this should be cleaned up, and is probably incorrect for IDN.
     std::string new_match = match.url_info.url().possibly_invalid_spec().
-        substr(0, match.input_location + params.input.text().length());
+        substr(0, match.input_location + params->input.text().length());
     search_base = GURL(new_match);
     if (search_base.is_empty())
       return;  // Can't construct a valid URL from which to start a search.
   } else if (!can_add_search_base_to_matches) {
     can_add_search_base_to_matches =
-        (search_base !=  params.what_you_typed_match.destination_url);
+        (search_base != params->what_you_typed_match.destination_url);
   }
   if (search_base == match.url_info.url())
     return;  // Couldn't shorten |match|, so no range of URLs to search over.
 
   // Search the DB for short URLs between our base and |match|.
   history::URLRow info(search_base);
   bool promote = true;
   // A short URL is only worth suggesting if it's been visited at least a third
   // as often as the longer URL.
   const int min_visit_count = ((match.url_info.visit_count() - 1) / 3) + 1;
@@ skipping 13 matching lines @@
     // Try to get info on the search base itself.  Promote it to the top if the
     // original best match isn't good enough to autocomplete.
     db->GetRowForURL(search_base, &info);
     promote = match.url_info.typed_count() <= 1;
   }
 
   // Promote or add the desired URL to the list of matches.
   bool ensure_can_inline =
       promote && CanPromoteMatchForInlineAutocomplete(match);
   ensure_can_inline &= CreateOrPromoteMatch(info, match.input_location,
-      match.match_in_scheme, matches, create_shorter_match_, promote);
+      match.match_in_scheme, &params->matches, create_shorter_match_, promote);
   if (ensure_can_inline)
-    matches->front().promoted = true;
+    params->matches[0].promoted = true;
 }
 
 void HistoryURLProvider::CullPoorMatches(
-    const HistoryURLProviderParams& params,
-    history::HistoryMatches* matches) const {
+    HistoryURLProviderParams* params) const {
   const base::Time& threshold(history::AutocompleteAgeThreshold());
-  for (history::HistoryMatches::iterator i(matches->begin());
-       i != matches->end(); ) {
+  for (history::HistoryMatches::iterator i(params->matches.begin());
+       i != params->matches.end(); ) {
     if (RowQualifiesAsSignificant(i->url_info, threshold) &&
-        !(params.default_search_provider &&
-            params.default_search_provider->IsSearchURL(
-                i->url_info.url(), *params.search_terms_data.get()))) {
+        (!params->default_search_provider ||
+         !params->default_search_provider->IsSearchURL(
+             i->url_info.url(), *params->search_terms_data))) {
       ++i;
     } else {
-      i = matches->erase(i);
+      i = params->matches.erase(i);
     }
   }
 }
 
 void HistoryURLProvider::CullRedirects(history::HistoryBackend* backend,
                                        history::HistoryMatches* matches,
                                        size_t max_results) const {
   for (size_t source = 0;
        (source < matches->size()) && (source < max_results); ) {
     const GURL& url = (*matches)[source].url_info.url();
@@ skipping 46 matching lines @@
     // need to shift it to the right and remember that so we can return it.
     next = matches->erase(next);
     if (static_cast<size_t>(next - matches->begin()) < next_index)
       --next_index;
   }
   return next_index;
 }
 
 AutocompleteMatch HistoryURLProvider::HistoryMatchToACMatch(
     const HistoryURLProviderParams& params,
-    const history::HistoryMatch& history_match,
+    size_t match_number,
     MatchType match_type,
     int relevance) {
+  // The FormattedStringWithEquivalentMeaning() call below requires callers to
+  // be on the UI thread.
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI) ||
+      !content::BrowserThread::IsThreadInitialized(content::BrowserThread::UI));
+
+  const history::HistoryMatch& history_match = params.matches[match_number];
   const history::URLRow& info = history_match.url_info;
   AutocompleteMatch match(this, relevance,
       !!info.visit_count(), AutocompleteMatchType::HISTORY_URL);
   match.typed_count = info.typed_count();
   match.destination_url = info.url();
   DCHECK(match.destination_url.is_valid());
   size_t inline_autocomplete_offset =
       history_match.input_location + params.input.text().length();
   std::string languages = (match_type == WHAT_YOU_TYPED) ?
       std::string() : params.languages;
@@ skipping 31 matching lines @@
     AutocompleteMatch::ClassifyLocationInString(base::string16::npos, 0,
         match.contents.length(), ACMatchClassification::URL,
         &match.contents_class);
   }
   match.description = info.title();
   match.description_class =
       ClassifyDescription(params.input.text(), match.description);
   RecordAdditionalInfoFromUrlRow(info, &match);
   return match;
 }