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

chrome/browser/autocomplete/history_url_provider.h

Index: chrome/browser/autocomplete/history_url_provider.h
diff --git a/chrome/browser/autocomplete/history_url_provider.h b/chrome/browser/autocomplete/history_url_provider.h
index 47d01d9e357e0f3c7bb191ede71e324e2f436be7..63f0608ccd6747a50c6f348ca44f47c03a6eea5d 100644
--- a/chrome/browser/autocomplete/history_url_provider.h
+++ b/chrome/browser/autocomplete/history_url_provider.h
@@ -88,6 +88,12 @@ class URLDatabase;
 // Used to communicate autocomplete parameters between threads via the history
 // service.
 struct HistoryURLProviderParams {
+  enum PromoteType {

[Mark P, 2014/06/18 18:23:45]

nit: comment

[Peter Kasting, 2014/06/18 20:38:30]

I couldn't think of anything to put here that wasn't redundant with the comments
on |promote_type| below.

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

Then perhaps say "See comments by declaration of |promote_type| below."?

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

Done.

+    WHAT_YOU_TYPED_MATCH,
+    FRONT_HISTORY_MATCH,
+    NEITHER,
+  };
+
   HistoryURLProviderParams(const AutocompleteInput& input,
                            bool trim_http,
                            const AutocompleteMatch& what_you_typed_match,
@@ -125,18 +131,21 @@ struct HistoryURLProviderParams {
   // |matches_| at all, so it won't delete the default match Start() creates.
   bool failed;
 
-  // List of matches written by the history thread.  We keep this separate list
-  // to avoid having the main thread read the provider's matches while the
-  // history thread is manipulating them.  The provider copies this list back
-  // to matches_ on the main thread in QueryComplete().
-  ACMatches matches;
+  // List of matches written by DoAutocomplete().  Upon its return the provider
+  // converts this list to ACMatches and places them in |matches_|.
+  history::HistoryMatches matches;
+
+  // 1 if the what you typed match appears as a known history entry, 0
+  // otherwise.
+  size_t exact_suggestion;

[Mark P, 2014/06/18 18:23:45]

nit: (in a later changelist, perhaps)
I've always thought this was a horrible name for this variable.

[Peter Kasting, 2014/06/18 20:38:30]

Me too.  I figured out a way to change this to a bool with a better name.

+
+  // Tells the provider whether to promote the what you typed match, the first
+  // element of |matches|, or neither as the first AutocompleteMatch.

[Mark P, 2014/06/18 18:23:45]

By convention, what is this set to if the first element of matches is the what
you typed match?

[Peter Kasting, 2014/06/18 20:38:30]

Added comments about this.

+  PromoteType promote_type;
 
   // Languages we should pass to gfx::GetCleanStringFromUrl.
   std::string languages;
 
-  // When true, we should avoid calling SuggestExactInput().
-  bool dont_suggest_exact_input;
-
   // The default search provider and search terms data necessary to cull results
   // that correspond to searches (on the default engine).  These can only be
   // obtained on the UI thread, so we have to copy them into here to pass them
@@ -173,8 +182,9 @@ class HistoryURLProvider : public HistoryProvider {
   // input of |text|.  |trim_http| controls whether the match's |fill_into_edit|
   // and |contents| should have any HTTP scheme stripped off, and should not be
   // set to true if |text| contains an http prefix.
-  // NOTE: This does not set the relevance of the returned match, as different
-  //       callers want different behavior. Callers must set this manually.
+  // NOTES: This does not set the relevance of the returned match, as different
+  //        callers want different behavior. Callers must set this manually.
+  //        This function should only be called on the UI thread.
   AutocompleteMatch SuggestExactInput(const base::string16& text,
                                       const GURL& destination_url,
                                       bool trim_http);
@@ -220,6 +230,11 @@ class HistoryURLProvider : public HistoryProvider {
                       history::URLDatabase* db,
                       HistoryURLProviderParams* params);
 
+  // Clears |matches_|, then may promote either the what you typed match or
+  // first history match in |params| to the front of |matches_|, depending on
+  // the value of params->promote_type.

[Mark P, 2014/06/18 18:23:44]

Clearing matches seems like an odd initial/side effect.

Here are better options:
* Make the caller clear matches beforehand; call this function
AddPromotedMatchIfNecessary.
* Call this function something like ResetMatchesToPromotedMatch
* Make this function deal fine with pre-existing matches; i.e., make it
rearrange matches it already has to promote the relevant match if necessary.

[Peter Kasting, 2014/06/18 20:38:30]

It is.  I did it because both callers wanted it so the code was shorter this
way, but that probably shouldn't trump having the function mean something sane,
semantically.
I took this route (but didn't rename the function, the existing name seemed
better).

+  void PromoteMatchIfNecessary(const HistoryURLProviderParams& params);
+
   // Dispatches the results to the autocomplete controller. Called on the
   // main thread by ExecuteWithDB when the results are available.
   // Frees params_gets_deleted on exit.
@@ -228,12 +243,11 @@ class HistoryURLProvider : public HistoryProvider {
   // Looks up the info for params->what_you_typed_match in the DB.  If found,
   // fills in the title, promotes the match's priority to that of an inline
   // autocomplete match (maybe it should be slightly better?), and places it on
-  // the front of |matches| (so we pick the right matches to throw away when
-  // culling redirects to/from it).  Returns whether a match was promoted.
+  // the front of params->matches (so we pick the right matches to throw away
+  // when culling redirects to/from it).  Returns whether a match was promoted.
   bool FixupExactSuggestion(history::URLDatabase* db,
                             const VisitClassifier& classifier,
-                            HistoryURLProviderParams* params,
-                            history::HistoryMatches* matches) const;
+                            HistoryURLProviderParams* params) const;
 
   // Helper function for FixupExactSuggestion, this returns true if the input
   // corresponds to some intranet URL where the user has previously visited the
@@ -241,22 +255,16 @@ class HistoryURLProvider : public HistoryProvider {
   bool CanFindIntranetURL(history::URLDatabase* db,
                           const AutocompleteInput& input) const;
 
-  // Determines if |match| is suitable for inline autocomplete.  If so, promotes
-  // the match.  Returns whether |match| was promoted.
-  bool PromoteMatchForInlineAutocomplete(const history::HistoryMatch& match,
-                                         HistoryURLProviderParams* params);
-
   // Sees if a shorter version of the best match should be created, and if so
-  // places it at the front of |matches|.  This can suggest history URLs that
-  // are prefixes of the best match (if they've been visited enough, compared to
-  // the best match), or create host-only suggestions even when they haven't
-  // been visited before: if the user visited http://example.com/asdf once,
-  // we'll suggest http://example.com/ even if they've never been to it.
+  // places it at the front of params->matches.  This can suggest history URLs
+  // that are prefixes of the best match (if they've been visited enough,
+  // compared to the best match), or create host-only suggestions even when they
+  // haven't been visited before: if the user visited http://example.com/asdf
+  // once, we'll suggest http://example.com/ even if they've never been to it.
   void PromoteOrCreateShorterSuggestion(
       history::URLDatabase* db,
-      const HistoryURLProviderParams& params,
       bool have_what_you_typed_match,
-      history::HistoryMatches* matches);
+      HistoryURLProviderParams* params);
 
   // Removes results that have been rarely typed or visited, and not any time
   // recently.  The exact parameters for this heuristic can be found in the
@@ -264,8 +272,7 @@ class HistoryURLProvider : public HistoryProvider {
   // search engine. These are low-quality, difficult-to-understand matches for
   // users, and the SearchProvider should surface past queries in a better way
   // anyway.
-  void CullPoorMatches(const HistoryURLProviderParams& params,
-                       history::HistoryMatches* matches) const;
+  void CullPoorMatches(HistoryURLProviderParams* params) const;
 
   // Removes results that redirect to each other, leaving at most |max_results|
   // results.
@@ -285,12 +292,13 @@ class HistoryURLProvider : public HistoryProvider {
                                    size_t source_index,
                                    const std::vector<GURL>& remove) const;
 
-  // Converts a line from the database into an autocomplete match for display.
-  // If experimental scoring is enabled, the final relevance score might be
-  // different from the given |relevance|.
+  // Converts a specified |match_number| from params.matches into an
+  // autocomplete match for display.  If experimental scoring is enabled, the
+  // final relevance score might be different from the given |relevance|.
+  // NOTE: This function should only be called on the UI thread.
   AutocompleteMatch HistoryMatchToACMatch(
       const HistoryURLProviderParams& params,
-      const history::HistoryMatch& history_match,
+      size_t match_number,
       MatchType match_type,
       int relevance);