Cleanup AutocompleteInput and AutocompleteProvider Functions

* Make FixupUserInput() just return the fixed-up input text, since almost every
  caller wanted that (and the one that didn't, the HUP, will change in the
  future)
* Make some functions which took an AutocompleteInput object and then only cared
  about the text() just take a string16
* Eliminated HistoryURLProvider::RunAutocompletePasses(), since it had only one
  caller, and just inline the code into Start().  Also got rid of the
  |fixup_input_and_run_pass_1| variable that was never false.

BUG=none
TEST=none

Review URL: https://codereview.chromium.org/312423003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@275540 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/autocomplete/autocomplete_provider.cc

@@ -134,15 +134,18 @@ void AutocompleteProvider::UpdateStarredStateOfMatches() {
 }
 
 // static
-bool AutocompleteProvider::FixupUserInput(AutocompleteInput* input) {
-  const base::string16& input_text = input->text();
+AutocompleteProvider::FixupReturn AutocompleteProvider::FixupUserInput(
+    const AutocompleteInput& input) {
+  const base::string16& input_text = input.text();
+  const FixupReturn failed(false, input_text);
+
   // Fixup and canonicalize user input.
   const GURL canonical_gurl(URLFixerUpper::FixupURL(
       base::UTF16ToUTF8(input_text), std::string()));
   std::string canonical_gurl_str(canonical_gurl.possibly_invalid_spec());
   if (canonical_gurl_str.empty()) {
     // This probably won't happen, but there are no guarantees.
-    return false;
+    return failed;
   }
 
   // If the user types a number, GURL will convert it to a dotted quad.
@@ -151,11 +154,11 @@ bool AutocompleteProvider::FixupUserInput(AutocompleteInput* input) {
   // for hostname beginning with numbers (e.g. input of "17173" will be matched
   // against "0.0.67.21" instead of the original "17173", failing to find
   // "17173.com"), swap the original hostname in for the fixed-up one.
-  if ((input->type() != AutocompleteInput::URL) &&
+  if ((input.type() != AutocompleteInput::URL) &&
       canonical_gurl.HostIsIPAddress()) {
     std::string original_hostname =
-        base::UTF16ToUTF8(input_text.substr(input->parts().host.begin,
-                                            input->parts().host.len));
+        base::UTF16ToUTF8(input_text.substr(input.parts().host.begin,
+                                            input.parts().host.len));
     const url::Parsed& parts =
         canonical_gurl.parsed_for_possibly_invalid_spec();
     // parts.host must not be empty when HostIsIPAddress() is true.
@@ -163,7 +166,7 @@ bool AutocompleteProvider::FixupUserInput(AutocompleteInput* input) {
     canonical_gurl_str.replace(parts.host.begin, parts.host.len,
                                original_hostname);
   }
-  base::string16 output = base::UTF8ToUTF16(canonical_gurl_str);
+  base::string16 output(base::UTF8ToUTF16(canonical_gurl_str));
   // Don't prepend a scheme when the user didn't have one.  Since the fixer
   // upper only prepends the "http" scheme, that's all we need to check for.
   if (!AutocompleteInput::HasHTTPScheme(input_text))
@@ -197,11 +200,10 @@ bool AutocompleteProvider::FixupUserInput(AutocompleteInput* input) {
     output.append(num_input_slashes - num_output_slashes, '/');
   else if (num_output_slashes > num_input_slashes)
     output.erase(output.length() - num_output_slashes + num_input_slashes);
+  if (output.empty())
+    return failed;
 
-  url::Parsed parts;
-  URLFixerUpper::SegmentURL(output, &parts);
-  input->UpdateText(output, base::string16::npos, parts);
-  return !output.empty();
+  return FixupReturn(true, output);
 }
 
 // static

chrome/browser/autocomplete/autocomplete_provider.h

@@ -241,6 +241,8 @@ class AutocompleteProvider
   friend class base::RefCountedThreadSafe<AutocompleteProvider>;
   FRIEND_TEST_ALL_PREFIXES(BookmarkProviderTest, InlineAutocompletion);
 
+  typedef std::pair<bool, base::string16> FixupReturn;
+
   virtual ~AutocompleteProvider();
 
   // Updates the starred state of each of the matches in matches_ from the
@@ -257,9 +259,11 @@ class AutocompleteProvider
   // Note that we don't do this in AutocompleteInput's constructor, because if
   // e.g. we convert a Unicode hostname to punycode, other providers will show
   // output that surprises the user ("Search Google for xn--6ca.com").
-  // Returns false if the fixup attempt resulted in an empty string (which
-  // providers generally can't do anything with).
-  static bool FixupUserInput(AutocompleteInput* input);
+  // Returns a bool indicating whether fixup succeeded, as well as the fixed-up
+  // input text.  The returned string will be the same as the input string if
+  // fixup failed; this lets callers who don't care about failure simply use the
+  // string unconditionally.
+  static FixupReturn FixupUserInput(const AutocompleteInput& input);
 
   // Trims "http:" and up to two subsequent slashes from |url|.  Returns the
   // number of characters that were trimmed.

chrome/browser/autocomplete/bookmark_provider.cc

@@ -86,8 +86,7 @@ void BookmarkProvider::DoAutocomplete(const AutocompleteInput& input) {
                                         &matches);
   if (matches.empty())
     return;  // There were no matches.
-  AutocompleteInput fixed_up_input(input);
-  FixupUserInput(&fixed_up_input);
+  const base::string16 fixed_up_input(FixupUserInput(input).second);
   for (BookmarkMatches::const_iterator i = matches.begin(); i != matches.end();
        ++i) {
     // Create and score the AutocompleteMatch. If its score is 0 then the
@@ -138,7 +137,7 @@ class ScoringFunctor {
 
 AutocompleteMatch BookmarkProvider::BookmarkMatchToACMatch(
     const AutocompleteInput& input,
-    const AutocompleteInput& fixed_up_input,
+    const base::string16& fixed_up_input_text,
     const BookmarkMatch& bookmark_match) {
   // The AutocompleteMatch we construct is non-deletable because the only
   // way to support this would be to delete the underlying bookmark, which is
@@ -149,7 +148,7 @@ AutocompleteMatch BookmarkProvider::BookmarkMatchToACMatch(
   const GURL& url(bookmark_match.node->url());
   const base::string16& url_utf16 = base::UTF8ToUTF16(url.spec());
   size_t inline_autocomplete_offset = URLPrefix::GetInlineAutocompleteOffset(
-      input, fixed_up_input, false, url_utf16);
+      input.text(), fixed_up_input_text, false, url_utf16);
   match.destination_url = url;
   const size_t match_start = bookmark_match.url_match_positions.empty() ?
       0 : bookmark_match.url_match_positions[0].first;

chrome/browser/autocomplete/bookmark_provider.h

@@ -51,11 +51,11 @@ class BookmarkProvider : public AutocompleteProvider {
   // Compose an AutocompleteMatch based on |match| that has 1) the URL of
   // |match|'s bookmark, and 2) the bookmark's title, not the URL's page
   // title, as the description.  |input| is used to compute the match's
-  // inline_autocompletion.  |fixed_up_input| is used in that way as well;
+  // inline_autocompletion.  |fixed_up_input_text| is used in that way as well;
   // it's passed separately so this function doesn't have to compute it.
   AutocompleteMatch BookmarkMatchToACMatch(
       const AutocompleteInput& input,
-      const AutocompleteInput& fixed_up_input,
+      const base::string16& fixed_up_input_text,
       const BookmarkMatch& match);
 
   // Converts |positions| into ACMatchClassifications and returns the

chrome/browser/autocomplete/bookmark_provider_unittest.cc

@@ -394,8 +394,8 @@ TEST_F(BookmarkProviderTest, InlineAutocompletion) {
                             base::string16::npos, base::string16(), GURL(),
                             AutocompleteInput::INVALID_SPEC, false, false,
                             false, true);
-    AutocompleteInput fixed_up_input(input);
-    provider_->FixupUserInput(&fixed_up_input);
+    const base::string16 fixed_up_input(
+        provider_->FixupUserInput(input).second);
     BookmarkNode node(GURL(query_data[i].url));
     node.SetTitle(base::ASCIIToUTF16(query_data[i].url));
     BookmarkMatch bookmark_match;

chrome/browser/autocomplete/history_url_provider.cc

@@ -408,7 +408,91 @@ void HistoryURLProvider::Start(const AutocompleteInput& input,
   // Cancel any in-progress query.
   Stop(false);
 
-  RunAutocompletePasses(input, true);
+  matches_.clear();
+
+  if ((input.type() == AutocompleteInput::INVALID) ||
+      (input.type() == AutocompleteInput::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.
+  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() != AutocompleteInput::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);
+  }
+
+  // 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;
+  URLFixerUpper::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,
+          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();
+  }
+
+  // Pass 2: Ask the history service to call us back on the history thread,
+  // where we can read the full on-disk DB.
+  if (search_url_database_ && 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) {
@@ -688,93 +772,6 @@ int HistoryURLProvider::CalculateRelevance(MatchType match_type,
   }
 }
 
-void HistoryURLProvider::RunAutocompletePasses(
-    const AutocompleteInput& input,
-    bool fixup_input_and_run_pass_1) {
-  matches_.clear();
-
-  if ((input.type() == AutocompleteInput::INVALID) ||
-      (input.type() == AutocompleteInput::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.
-  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() != AutocompleteInput::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);
-  }
-
-  // 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_);
-
-  // 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(
-          input, trim_http,
-          profile_->GetPrefs()->GetString(prefs::kAcceptLanguages),
-          default_search_provider, data));
-
-  params->prevent_inline_autocomplete =
-      PreventInlineAutocomplete(input);
-
-  if (fixup_input_and_run_pass_1) {
-    // Do some fixup on the user input before matching against it, so we provide
-    // good results for local file paths, input with spaces, etc.
-    if (!FixupUserInput(&params->input))
-      return;
-
-    // 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();
-    }
-  }
-
-  // Pass 2: Ask the history service to call us back on the history thread,
-  // where we can read the full on-disk DB.
-  if (search_url_database_ && 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_);
-  }
-}
-
 bool HistoryURLProvider::FixupExactSuggestion(
     history::URLDatabase* db,
     const AutocompleteInput& input,

chrome/browser/autocomplete/history_url_provider.h

@@ -219,10 +219,6 @@ class HistoryURLProvider : public HistoryProvider {
   // 1415, 1414, ...).
   int CalculateRelevance(MatchType match_type, size_t match_number) const;
 
-  // Helper function that actually launches the two autocomplete passes.
-  void RunAutocompletePasses(const AutocompleteInput& input,
-                             bool fixup_input_and_run_pass_1);
-
   // Given a |match| containing the "what you typed" suggestion created by
   // SuggestExactInput(), looks up its info in the DB.  If found, fills in the
   // title from the DB, promotes the match's priority to that of an inline

chrome/browser/autocomplete/shortcuts_provider.cc

@@ -139,10 +139,9 @@ void ShortcutsProvider::GetMatches(const AutocompleteInput& input) {
   base::string16 term_string(base::i18n::ToLower(input.text()));
   DCHECK(!term_string.empty());
 
-  AutocompleteInput fixed_up_input(input);
-  FixupUserInput(&fixed_up_input);
   const GURL& input_as_gurl = URLFixerUpper::FixupURL(
       base::UTF16ToUTF8(input.text()), std::string());
+  const base::string16 fixed_up_input(FixupUserInput(input).second);
 
   int max_relevance;
   if (!OmniboxFieldTrial::ShortcutsScoringMaxRelevance(
@@ -156,8 +155,8 @@ void ShortcutsProvider::GetMatches(const AutocompleteInput& input) {
     // Don't return shortcuts with zero relevance.
     int relevance = CalculateScore(term_string, it->second, max_relevance);
     if (relevance) {
-      matches_.push_back(ShortcutToACMatch(
-          it->second, relevance, input, fixed_up_input, input_as_gurl));
+      matches_.push_back(ShortcutToACMatch(it->second, relevance, input,
+                                           fixed_up_input, input_as_gurl));
       matches_.back().ComputeStrippedDestinationURL(profile_);
     }
   }
@@ -189,7 +188,7 @@ AutocompleteMatch ShortcutsProvider::ShortcutToACMatch(
     const history::ShortcutsDatabase::Shortcut& shortcut,
     int relevance,
     const AutocompleteInput& input,
-    const AutocompleteInput& fixed_up_input,
+    const base::string16& fixed_up_input_text,
     const GURL& input_as_gurl) {
   DCHECK(!input.text().empty());
   AutocompleteMatch match;
@@ -234,7 +233,7 @@ AutocompleteMatch ShortcutsProvider::ShortcutToACMatch(
   } else {
     const size_t inline_autocomplete_offset =
         URLPrefix::GetInlineAutocompleteOffset(
-            input, fixed_up_input, true, match.fill_into_edit);
+            input.text(), fixed_up_input_text, true, match.fill_into_edit);
     if (inline_autocomplete_offset != base::string16::npos) {
       match.inline_autocompletion =
           match.fill_into_edit.substr(inline_autocomplete_offset);

chrome/browser/autocomplete/shortcuts_provider.h

@@ -52,13 +52,13 @@ class ShortcutsProvider
   // Returns an AutocompleteMatch corresponding to |shortcut|. Assigns it
   // |relevance| score in the process, and highlights the description and
   // contents against |input|, which should be the lower-cased version
-  // of the user's input. |input|, |fixed_up_input|, and
+  // of the user's input. |input|, |fixed_up_input_text|, and
   // |input_as_gurl| are used to decide what can be inlined.
   AutocompleteMatch ShortcutToACMatch(
       const history::ShortcutsDatabase::Shortcut& shortcut,
       int relevance,
       const AutocompleteInput& input,
-      const AutocompleteInput& fixed_up_input,
+      const base::string16& fixed_up_input_text,
       const GURL& input_as_gurl);
 
   // Returns a map mapping characters to groups of words from |text| that start

chrome/browser/autocomplete/url_prefix.cc

@@ -7,7 +7,6 @@
 #include "base/basictypes.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
-#include "chrome/browser/autocomplete/autocomplete_input.h"
 
 namespace {
 
@@ -78,24 +77,22 @@ bool URLPrefix::PrefixMatch(const URLPrefix& prefix,
 
 // static
 size_t URLPrefix::GetInlineAutocompleteOffset(
-    const AutocompleteInput& input,
-    const AutocompleteInput& fixed_up_input,
+    const base::string16& input,
+    const base::string16& fixed_up_input,
     const bool allow_www_prefix_without_scheme,
     const base::string16& text) {
   const URLPrefix* best_prefix = allow_www_prefix_without_scheme ?
-      BestURLPrefixWithWWWCase(text, input.text()) :
-      BestURLPrefix(text, input.text());
-  const base::string16* matching_string = &input.text();
+      BestURLPrefixWithWWWCase(text, input) : BestURLPrefix(text, input);
+  const base::string16* matching_string = &input;
   // If we failed to find a best_prefix initially, try again using a fixed-up
   // version of the user input.  This is especially useful to get about: URLs
   // to inline against chrome:// shortcuts.  (about: URLs are fixed up to the
   // chrome:// scheme.)
-  if ((best_prefix == NULL) && !fixed_up_input.text().empty() &&
-      (fixed_up_input.text() != input.text())) {
+  if (!best_prefix && !fixed_up_input.empty() && (fixed_up_input != input)) {
     best_prefix = allow_www_prefix_without_scheme ?
-        BestURLPrefixWithWWWCase(text, fixed_up_input.text()) :
-        BestURLPrefix(text, fixed_up_input.text());
-    matching_string = &fixed_up_input.text();
+        BestURLPrefixWithWWWCase(text, fixed_up_input) :
+        BestURLPrefix(text, fixed_up_input);
+    matching_string = &fixed_up_input;
   }
   return (best_prefix != NULL) ?
       (best_prefix->prefix.length() + matching_string->length()) :

chrome/browser/autocomplete/url_prefix.h

@@ -8,7 +8,6 @@
 #include <vector>
 
 #include "base/strings/string16.h"
-#include "chrome/browser/autocomplete/autocomplete_input.h"
 
 struct URLPrefix;
 typedef std::vector<URLPrefix> URLPrefixes;
@@ -48,8 +47,8 @@ struct URLPrefix {
   // a protocol at the beginning, and we want to allow these matches to be
   // inlineable.  ("www." is not otherwise on the default prefix list.)
   static size_t GetInlineAutocompleteOffset(
-      const AutocompleteInput& input,
-      const AutocompleteInput& fixed_up_input,
+      const base::string16& input,
+      const base::string16& fixed_up_input,
       const bool allow_www_prefix_without_scheme,
       const base::string16& text);