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

chrome/browser/autocomplete/history_url_provider.cc

Index: chrome/browser/autocomplete/history_url_provider.cc
diff --git a/chrome/browser/autocomplete/history_url_provider.cc b/chrome/browser/autocomplete/history_url_provider.cc
index d60910b65d3dc7de62aa27baaade8676421c65b8..39238bb04e93541de41c8306299aee6f31888095 100644
--- a/chrome/browser/autocomplete/history_url_provider.cc
+++ b/chrome/browser/autocomplete/history_url_provider.cc
@@ -417,8 +417,9 @@ HistoryURLProviderParams::HistoryURLProviderParams(
       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)) {
@@ -527,10 +528,8 @@ void HistoryURLProvider::Start(const AutocompleteInput& input,
   // 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.
@@ -689,9 +688,8 @@ 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) {
@@ -712,15 +710,15 @@ void HistoryURLProvider::DoAutocomplete(history::HistoryBackend* backend,
       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
@@ -737,77 +735,62 @@ void HistoryURLProvider::DoAutocomplete(history::HistoryBackend* backend,
        (classifier.type() == VisitClassifier::UNVISITED_INTRANET) ||
        !params->trim_http ||
        (AutocompleteInput::NumNonHostComponents(params->input.parts()) > 0));
-  PromoteOrCreateShorterSuggestion(db, *params, have_what_you_typed_match,
-                                   &history_matches);
-
-  // 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)) {
+  PromoteOrCreateShorterSuggestion(db, have_what_you_typed_match, params);
+
+  // 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);
-  }
-
-  // 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);
+    params->promote_type = have_what_you_typed_match ?
+        HistoryURLProviderParams::WHAT_YOU_TYPED_MATCH :
+        HistoryURLProviderParams::NEITHER;
   }
 
-  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);
+    params->matches.resize(max_results);
   }
+}
 
-  // 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);
-  }
+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(
@@ -827,7 +810,35 @@ void HistoryURLProvider::QueryComplete(
   // 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();
   }
 
@@ -838,10 +849,7 @@ void HistoryURLProvider::QueryComplete(
 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:
@@ -904,12 +912,12 @@ bool HistoryURLProvider::FixupExactSuggestion(
   // 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;
 }
 
@@ -934,38 +942,18 @@ bool HistoryURLProvider::CanFindIntranetURL(
   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
@@ -976,13 +964,13 @@ void HistoryURLProvider::PromoteOrCreateShorterSuggestion(
     // "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.
@@ -1016,24 +1004,23 @@ void HistoryURLProvider::PromoteOrCreateShorterSuggestion(
   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);
     }
   }
 }
@@ -1100,9 +1087,15 @@ size_t HistoryURLProvider::RemoveSubsequentMatchesOf(
 
 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);