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

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 18 matching lines @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/search_engines/template_url_service.h"
 #include "chrome/browser/search_engines/template_url_service_factory.h"
 #include "chrome/browser/search_engines/ui_thread_search_terms_data.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
 #include "components/bookmarks/browser/bookmark_utils.h"
 #include "components/metrics/proto/omnibox_input_type.pb.h"
 #include "components/url_fixer/url_fixer.h"
+#include "content/public/browser/browser_thread.h"
 #include "net/base/net_util.h"
 #include "net/base/registry_controlled_domains/registry_controlled_domain.h"
 #include "url/gurl.h"
 #include "url/url_parse.h"
 #include "url/url_util.h"
 
 namespace {
 
 // Acts like the > operator for URLInfo classes.
 bool CompareHistoryMatch(const history::HistoryMatch& a,
@@ skipping 172 matching lines @@
   // No entry, so create one.
   history::HistoryMatch match(info, input_location, match_in_scheme, true);
   if (promote)
     matches->push_front(match);
   else
     matches->push_back(match);
 
   return true;
 }
 
+// Returns whether |match| is suitable for inline autocompletion.
+bool CanPromoteMatchForInlineAutocomplete(const history::HistoryMatch& match) {
+  // We can promote this match if it's been marked for promotion or typed at
+  // least n times, where n == 1 for "simple" (host-only) URLs and n == 2 for
+  // others.  We set a higher bar for these long URLs because it's less likely
+  // that users will want to visit them again.  Even though we don't increment
+  // the typed_count for pasted-in URLs, if the user manually edits the URL or
+  // types some long thing in by hand, we wouldn't want to immediately start
+  // autocompleting it.
+  return match.promoted ||
+      (match.url_info.typed_count() &&
+       ((match.url_info.typed_count() > 1) || match.IsHostOnly()));
+}
+
 // Given the user's |input| and a |match| created from it, reduce the match's
 // URL to just a host.  If this host still matches the user input, return it.
 // Returns the empty string on failure.
 GURL ConvertToHostOnly(const history::HistoryMatch& match,
                        const base::string16& input) {
   // See if we should try to do host-only suggestions for this URL. Nonstandard
   // schemes means there's no authority section, so suggesting the host name
   // is useless. File URLs are standard, but host suggestion is not useful for
   // them either.
   const GURL& url = match.url_info.url();
@@ skipping 143 matching lines @@
     // The user typed an intranet hostname that they've visited (albeit with a
     // different port and/or path) before.
     url_row_ = history::URLRow(url);
     type_ = UNVISITED_INTRANET;
   }
 }
 
 HistoryURLProviderParams::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)
     : 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),
       languages(languages),
       dont_suggest_exact_input(false),
       default_search_provider(default_search_provider ?
           new TemplateURL(default_search_provider->profile(),
                           default_search_provider->data()) : NULL),
       search_terms_data(new SearchTermsDataSnapshot(search_terms_data)) {
 }
 
 HistoryURLProviderParams::~HistoryURLProviderParams() {
 }
 
 HistoryURLProvider::HistoryURLProvider(AutocompleteProviderListener* listener,
                                        Profile* profile)
     : HistoryProvider(listener, profile,
-          AutocompleteProvider::TYPE_HISTORY_URL),
+                      AutocompleteProvider::TYPE_HISTORY_URL),
       params_(NULL),
       cull_redirects_(
           !OmniboxFieldTrial::InHUPCullRedirectsFieldTrial() ||
           !OmniboxFieldTrial::InHUPCullRedirectsFieldTrialExperimentGroup()),
       create_shorter_match_(
           !OmniboxFieldTrial::InHUPCreateShorterMatchFieldTrial() ||
           !OmniboxFieldTrial::
               InHUPCreateShorterMatchFieldTrialExperimentGroup()) {
   // Initialize HUP scoring params based on the current experiment.
   OmniboxFieldTrial::GetExperimentalHUPScoringParams(&scoring_params_);
@@ skipping 12 matching lines @@
 
   // Cancel any in-progress query.
   Stop(false);
 
   matches_.clear();
 
   if ((input.type() == metrics::OmniboxInputType::INVALID) ||
       (input.type() == metrics::OmniboxInputType::FORCED_QUERY))
     return;
 
-  // Create a match for exactly what the user typed.  This will only be used as
-  // a fallback in case we can't get the history service or URL DB; otherwise,
-  // we'll run this again in DoAutocomplete() and use that result instead.
+  // Do some fixup on the user input before matching against it, so we provide
+  // good results for local file paths, input with spaces, etc.
+  const FixupReturn fixup_return(FixupUserInput(input));
+  if (!fixup_return.first)
+    return;

[Mark P, 2014/06/17 17:43:25]

This makes me nervous.  Previously we'd create a a what-you-typed match on the
original text and use it if there was not DB.  I think the previous order was
conscious (otherwise we would've added the Fixup code earlier in this function).
 Now we only get such a match if the text is successfully fixed up.  A UWYT
match is really an important fallback, esp. if the user is in a screwed up
state.

Worse, these types of bugs are temperamental / hard-to-reproduce, meaning that
if this new approach causes problems, they're not likely to be reported.

[Peter Kasting, 2014/06/17 20:39:47]

It wasn't conscious.  The two pieces were written (both by me) at completely
different times and I wasn't thinking about one when I wrote the other.
If FixupUserInput() fails, the WYT match wouldn't have been navigable anyway. 
We're not losing anything.

I'm not worried about this at all.

[Mark P, 2014/06/17 21:35:35]

Good point.

+  url::Parsed parts;
+  url_fixer::SegmentURL(fixup_return.second, &parts);
+  AutocompleteInput fixed_up_input(input);
+  fixed_up_input.UpdateText(fixup_return.second, base::string16::npos, parts);
+
+  // Create a match for what the user typed.
   const bool trim_http = !AutocompleteInput::HasHTTPScheme(input.text());
-  // Don't do this for queries -- while we can sometimes mark up a match for
-  // this, it's not what the user wants, and just adds noise.
-  if (input.type() != metrics::OmniboxInputType::QUERY) {
-    AutocompleteMatch what_you_typed(SuggestExactInput(
-        input.text(), input.canonicalized_url(), trim_http));
-    what_you_typed.relevance = CalculateRelevance(WHAT_YOU_TYPED, 0);
-    matches_.push_back(what_you_typed);
-  }
+  AutocompleteMatch what_you_typed_match(SuggestExactInput(
+      fixed_up_input.text(), fixed_up_input.canonicalized_url(), trim_http));
+  what_you_typed_match.relevance = CalculateRelevance(WHAT_YOU_TYPED, 0);
+
+  // Add the WYT match as a fallback in case we can't get the history service or
+  // URL DB; otherwise, we'll replace this match lower down.  Don't do this for
+  // queries, though -- while we can sometimes mark up a match for them, it's
+  // not what the user wants, and just adds noise.
+  if (fixed_up_input.type() != metrics::OmniboxInputType::QUERY)
+    matches_.push_back(what_you_typed_match);
 
   // We'll need the history service to run both passes, so try to obtain it.
   if (!profile_)
     return;
   HistoryService* const history_service =
       HistoryServiceFactory::GetForProfile(profile_, Profile::EXPLICIT_ACCESS);
   if (!history_service)
     return;
 
   // Get the default search provider and search terms data now since we have to
   // retrieve these on the UI thread, and the second pass runs on the history
   // thread. |template_url_service| can be NULL when testing.
   TemplateURLService* template_url_service =
       TemplateURLServiceFactory::GetForProfile(profile_);
   TemplateURL* default_search_provider = template_url_service ?
       template_url_service->GetDefaultSearchProvider() : NULL;
   UIThreadSearchTermsData data(profile_);
 
-  // Do some fixup on the user input before matching against it, so we provide
-  // good results for local file paths, input with spaces, etc.
-  const FixupReturn fixup_return(FixupUserInput(input));
-  if (!fixup_return.first)
-    return;
-  url::Parsed parts;
-  url_fixer::SegmentURL(fixup_return.second, &parts);
-  AutocompleteInput fixed_up_input(input);
-  fixed_up_input.UpdateText(fixup_return.second, base::string16::npos, parts);
-
   // Create the data structure for the autocomplete passes.  We'll save this off
   // onto the |params_| member for later deletion below if we need to run pass
   // 2.
   scoped_ptr<HistoryURLProviderParams> params(
       new HistoryURLProviderParams(
-          fixed_up_input, trim_http,
+          fixed_up_input, trim_http, what_you_typed_match,
           profile_->GetPrefs()->GetString(prefs::kAcceptLanguages),
           default_search_provider, data));
   // Note that we use the non-fixed-up input here, since fixup may strip
   // trailing whitespace.
   params->prevent_inline_autocomplete = PreventInlineAutocomplete(input);
 
   // Pass 1: Get the in-memory URL database, and use it to find and promote
   // the inline autocomplete match, if any.
   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);
     UpdateStarredStateOfMatches();
+    params->what_you_typed_match = what_you_typed_match;

[Mark P, 2014/06/17 17:43:25]

I don't understand why this is necessary.  Didn't you set this in the
constructor above?

[Peter Kasting, 2014/06/17 20:39:48]

Running an autocomplete pass changes the What You Typed match.  I want to pass
the same match to both passes to guarantee they both act the same way on it.

[Mark P, 2014/06/17 21:35:35]

This is not an obvious side effect of DoAutocomplete.  Please put this
explanation in the code.

   }
 
   // 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_);
   }
 }
 
 void HistoryURLProvider::Stop(bool clear_cached_results) {
   done_ = true;
 
   if (params_)
     params_->cancel_flag.Set();
 }
 
 AutocompleteMatch HistoryURLProvider::SuggestExactInput(
     const base::string16& text,
     const GURL& destination_url,
     bool trim_http) {
+  // The FormattedStringWithEquivalentMeaning() call below requires callers to
+  // be on the UI thread.
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI) ||
+      !content::BrowserThread::IsMessageLoopValid(content::BrowserThread::UI));

[Mark P, 2014/06/17 17:43:25]

elsewhere we usually use
IsThreadInitialized
rather than
IsMessageLoopValid

[Peter Kasting, 2014/06/17 20:39:47]

Done.

+
   AutocompleteMatch match(this, 0, false,
                           AutocompleteMatchType::URL_WHAT_YOU_TYPED);
 
   if (destination_url.is_valid()) {
     match.destination_url = destination_url;
 
     // Trim off "http://" if the user didn't type it.
     // NOTE: We use TrimHttpPrefix() here rather than StringForURLDisplay() to
     // strip the scheme as we need to know the offset so we can adjust the
     // |match_location| below.  StringForURLDisplay() and TrimHttpPrefix() have
@@ skipping 105 matching lines @@
       history::ScoredHistoryMatch::FilterTermMatchesByWordStarts(
           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) {
-  VisitClassifier classifier(this, params->input, db);
-  // Create a What You Typed match, which we'll need below.
-  //
-  // We display this to the user when there's a reasonable chance they actually
-  // care:
-  // * 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:".
-  //  that is when their input can be opened as a URL.
-  // 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.
-  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));
-  AutocompleteMatch what_you_typed_match(SuggestExactInput(
-      params->input.text(), params->input.canonicalized_url(),
-      params->trim_http));
-  what_you_typed_match.relevance = CalculateRelevance(WHAT_YOU_TYPED, 0);
-
-  // Get the matching URLs from the DB
+  // Get the matching URLs from the DB.
   history::URLRows url_matches;
   history::HistoryMatches history_matches;
 
   const URLPrefixes& prefixes = URLPrefix::GetURLPrefixes();
   for (URLPrefixes::const_iterator i(prefixes.begin()); i != prefixes.end();
-        ++i) {
+       ++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 == NULL),
-        &url_matches);
+        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 != NULL);
-      history_matches.push_back(history::HistoryMatch(*j, i->prefix.length(),
-          i->num_components == 0,
+      const URLPrefix* best_prefix = URLPrefix::BestURLPrefix(
+          base::UTF8ToUTF16(j->url().spec()), base::string16());
+      DCHECK(best_prefix);
+      history_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);
+
+  // 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,
-                                   what_you_typed_match, &history_matches);
+                                   &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 |is_history_what_you_typed_match| tells us whether
-  // SuggestExactInput() succeeded in constructing a valid match.
-  if (what_you_typed_match.is_history_what_you_typed_match &&
+  // 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, params->input, classifier, &what_you_typed_match,
-                           &history_matches)) {
+      FixupExactSuggestion(db, classifier, params, &history_matches)) {
     // 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(what_you_typed_match);
+    params->matches.push_back(params->what_you_typed_match);
   } else if (params->prevent_inline_autocomplete ||
       history_matches.empty() ||
       !PromoteMatchForInlineAutocomplete(history_matches.front(), params)) {
     // 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(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) {
@@ skipping 28 matching lines @@
     // 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);
   }
 }
 
-// Called on the main thread when the query is complete.

[Mark P, 2014/06/17 17:43:25]

Is this incorrect?  Why did you remove it?

[Peter Kasting, 2014/06/17 20:39:47]

This comment is already present in the header.

 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);
     UpdateStarredStateOfMatches();
   }
 
   done_ = true;
   listener_->OnProviderUpdate(true);
 }
 
 bool HistoryURLProvider::FixupExactSuggestion(
     history::URLDatabase* db,
-    const AutocompleteInput& input,
     const VisitClassifier& classifier,
-    AutocompleteMatch* match,
+    HistoryURLProviderParams* params,
     history::HistoryMatches* matches) const {
-  DCHECK(match != NULL);
   DCHECK(matches != NULL);

[Mark P, 2014/06/17 17:43:25]

Perhaps DCHECK(params!=NULL)

[Peter Kasting, 2014/06/17 20:39:48]

I could, but I don't think it adds much.  I intend to remove the |matches|
DCHECK in the next patch, so I didn't bother to add a new one for |params| here.

 
   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.
-      match->deletable = true;
-      match->description = classifier.url_row().title();
-      RecordAdditionalInfoFromUrlRow(classifier.url_row(), match);
-      match->description_class =
-          ClassifyDescription(input.text(), match->description);
+      params->what_you_typed_match.deletable = true;
+      params->what_you_typed_match.description = classifier.url_row().title();
+      RecordAdditionalInfoFromUrlRow(classifier.url_row(),
+                                     &params->what_you_typed_match);
+      params->what_you_typed_match.description_class = ClassifyDescription(
+          params->input.text(), params->what_you_typed_match.description);
       if (!classifier.url_row().typed_count()) {
         // If we reach here, we must be in the second pass, and we must not have
         // this row's data available during the first pass.  That means we
         // either scored it as WHAT_YOU_TYPED or UNVISITED_INTRANET, and to
         // maintain the ordering between passes consistent, we need to score it
         // the same way here.
-        type = CanFindIntranetURL(db, input) ?
+        type = CanFindIntranetURL(db, params->input) ?
             UNVISITED_INTRANET : WHAT_YOU_TYPED;
       }
       break;
   }
 
-  const GURL& url = match->destination_url;
+  const GURL& url = params->what_you_typed_match.destination_url;
   const url::Parsed& parsed = url.parsed_for_possibly_invalid_spec();
   // If the what-you-typed result looks like a single word (which can be
   // interpreted as an intranet address) followed by a pound sign ("#"),
   // leave the score for the url-what-you-typed result as is.  It will be
   // outscored by a search query from the SearchProvider. This test fixes
   // cases such as "c#" and "c# foo" where the user has visited an intranet
   // site "c".  We want the search-what-you-typed score to beat the
   // URL-what-you-typed score in this case.  Most of the below test tries to
   // make sure that this code does not trigger if the user did anything to
   // indicate the desired match is a URL.  For instance, "c/# foo" will not
   // pass the test because that will be classified as input type URL.  The
   // parsed.CountCharactersBefore() in the test looks for the presence of a
   // reference fragment in the URL by checking whether the position differs
   // included the delimiter (pound sign) versus not including the delimiter.
   // (One cannot simply check url.ref() because it will not distinguish
   // between the input "c" and the input "c#", both of which will have empty
   // reference fragments.)
   if ((type == UNVISITED_INTRANET) &&
-      (input.type() != metrics::OmniboxInputType::URL) &&
+      (params->input.type() != metrics::OmniboxInputType::URL) &&
       url.username().empty() && url.password().empty() && url.port().empty() &&
       (url.path() == "/") && url.query().empty() &&
       (parsed.CountCharactersBefore(url::Parsed::REF, true) !=
        parsed.CountCharactersBefore(url::Parsed::REF, false))) {
     return false;
   }
 
-  match->relevance = CalculateRelevance(type, 0);
+  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())
     return false;
 
   // Put it on the front of the HistoryMatches for redirect culling.
@@ skipping 19 matching lines @@
       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) {
-  // Promote the first match if it's been marked for promotion or typed at least
-  // n times, where n == 1 for "simple" (host-only) URLs and n == 2 for others.
-  // We set a higher bar for these long URLs because it's less likely that users
-  // will want to visit them again.  Even though we don't increment the
-  // typed_count for pasted-in URLs, if the user manually edits the URL or types
-  // some long thing in by hand, we wouldn't want to immediately start
-  // autocompleting it.
-  if (!match.promoted &&
-      (!match.url_info.typed_count() ||
-       ((match.url_info.typed_count() == 1) &&
-        !match.IsHostOnly())))
+  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.
-  if (params) {
-    params->dont_suggest_exact_input = true;
-    AutocompleteMatch ac_match = HistoryMatchToACMatch(
-        *params, match, INLINE_AUTOCOMPLETE,
-        CalculateRelevance(INLINE_AUTOCOMPLETE, 0));
-    params->matches.push_back(ac_match);
-  }
+  params->dont_suggest_exact_input = true;
+  params->matches.push_back(HistoryMatchToACMatch(
+      *params, match, INLINE_AUTOCOMPLETE,
+      CalculateRelevance(INLINE_AUTOCOMPLETE, 0)));
   return true;
 }
 
-// See if a shorter version of the best match should be created, and if so place
-// 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.
 void HistoryURLProvider::PromoteOrCreateShorterSuggestion(
     history::URLDatabase* db,
     const HistoryURLProviderParams& params,
     bool have_what_you_typed_match,
-    const AutocompleteMatch& what_you_typed_match,
     history::HistoryMatches* matches) {
   if (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());
   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", |what_you_typed_match| will hold
+    // 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());
     search_base = GURL(new_match);
-    // TODO(mrossetti): There is a degenerate case where the following may
-    // cause a failure: http://www/~someword/fubar.html. Diagnose.
-    // See: http://crbug.com/50101
     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 != 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 11 matching lines @@
       return;  // Couldn't find anything and can't add the search base, bail.
 
     // 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 && PromoteMatchForInlineAutocomplete(match, NULL);
+      promote && CanPromoteMatchForInlineAutocomplete(match);
   ensure_can_inline &= CreateOrPromoteMatch(info, match.input_location,
       match.match_in_scheme, matches, create_shorter_match_, promote);
   if (ensure_can_inline)
     matches->front().promoted = true;
 }
 
 void HistoryURLProvider::CullPoorMatches(
     const HistoryURLProviderParams& params,
     history::HistoryMatches* matches) const {
   const base::Time& threshold(history::AutocompleteAgeThreshold());
@@ skipping 119 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;
 }