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

chrome/browser/autocomplete/history_url_provider.h

 // 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.
 
 #ifndef CHROME_BROWSER_AUTOCOMPLETE_HISTORY_URL_PROVIDER_H_
 #define CHROME_BROWSER_AUTOCOMPLETE_HISTORY_URL_PROVIDER_H_
 
 #include <string>
 #include <vector>
 
@@ skipping 70 matching lines @@
 // request.  This can happen frequently when the user is typing quickly.  In
 // this case, the main thread sets params_->cancel, which the background thread
 // checks periodically.  If it finds the flag set, it stops what it's doing
 // immediately and calls back to the main thread.  (We don't delete the params
 // on the history thread, because we should only do that when we can safely
 // NULL out params_, and that must be done on the main thread.)
 
 // 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,
                            const std::string& languages,
                            TemplateURL* default_search_provider,
                            const SearchTermsData& search_terms_data);
   ~HistoryURLProviderParams();
 
   base::MessageLoop* message_loop;
 
@@ skipping 17 matching lines @@
   // running queries that are no longer needed.  Since we don't care if we run
   // the extra queries, the lack of signaling is not a problem.
   base::CancellationFlag cancel_flag;
 
   // Set by ExecuteWithDB() on the history thread when the query could not be
   // performed because the history system failed to properly init the database.
   // If this is set when the main thread is called back, it avoids changing
   // |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
   // to the history thread.  We use a scoped_ptr<TemplateURL> for the DSP since
   // TemplateURLs can't be copied by value. We use a scoped_ptr<SearchTermsData>
   // so that we can store a snapshot of the SearchTermsData accessible from the
   // history thread.
   scoped_ptr<TemplateURL> default_search_provider;
   scoped_ptr<SearchTermsData> search_terms_data;
 
@@ skipping 16 matching lines @@
 
   // HistoryProvider:
   virtual void Start(const AutocompleteInput& input,
                      bool minimal_changes) OVERRIDE;
   virtual void Stop(bool clear_cached_results) OVERRIDE;
 
   // Returns a match representing a navigation to |destination_url| given user
   // 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);
 
   // Runs the history query on the history thread, called by the history
   // system. The history database MAY BE NULL in which case it is not
   // available and we should return no data. Also schedules returning the
   // results to the main thread
   void ExecuteWithDB(history::HistoryBackend* backend,
                      history::URLDatabase* db,
@@ skipping 25 matching lines @@
       const base::string16& input_text,
       const base::string16& description);
 
   // Actually runs the autocomplete job on the given database, which is
   // guaranteed not to be NULL.  Used by both autocomplete passes, and therefore
   // called on multiple different threads (though not simultaneously).
   void DoAutocomplete(history::HistoryBackend* backend,
                       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.
   void QueryComplete(HistoryURLProviderParams* params_gets_deleted);
 
   // 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
   // host in question.  In this case the input should be treated as a URL.
   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
   // function body. Also culls results corresponding to queries from the default
   // 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.
   void CullRedirects(history::HistoryBackend* backend,
                      history::HistoryMatches* matches,
                      size_t max_results) const;
 
   // Helper function for CullRedirects, this removes all but the first
   // occurance of [any of the set of strings in |remove|] from the |matches|
   // list.
   //
   // The return value is the index of the item that is after the item in the
   // input identified by |source_index|. If |source_index| or an item before
   // is removed, the next item will be shifted, and this allows the caller to
   // pick up on the next one when this happens.
   size_t RemoveSubsequentMatchesOf(history::HistoryMatches* matches,
                                    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);
 
   // Params for the current query.  The provider should not free this directly;
   // instead, it is passed as a parameter through the history backend, and the
   // parameter itself is freed once it's no longer needed.  The only reason we
   // keep this member is so we can set the cancel bit on it.
   HistoryURLProviderParams* params_;
 
   // Params controlling experimental behavior of this provider.
   HUPScoringParams scoring_params_;
 
   // If true, HistoryURL provider should lookup and cull redirects.  If
   // false, it returns matches that may be redirects to each other and
   // simply hopes the default AutoCompleteController behavior to remove
   // URLs that are likely duplicates (http://google.com <->
   // https://www.google.com/, etc.) will do a good enough job.
   bool cull_redirects_;
 
   // Used in PromoteOrCreateShorterSuggestion().  If true, we may create
   // shorter 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.
   bool create_shorter_match_;
 
   DISALLOW_COPY_AND_ASSIGN(HistoryURLProvider);
 };
 
 #endif  // CHROME_BROWSER_AUTOCOMPLETE_HISTORY_URL_PROVIDER_H_