Implemented substring matching within page titles. Fixed bug where URL was be...

chrome/browser/autocomplete/history_quick_provider.cc

Index: chrome/browser/autocomplete/history_quick_provider.cc
===================================================================
--- chrome/browser/autocomplete/history_quick_provider.cc	(revision 77314)
+++ chrome/browser/autocomplete/history_quick_provider.cc	(working copy)
@@ -9,7 +9,6 @@
 #include "base/string_util.h"
 #include "base/logging.h"
 #include "base/utf_string_conversions.h"
-#include "chrome/browser/autocomplete/autocomplete_match.h"
 #include "chrome/browser/history/history.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/profiles/profile.h"
@@ -19,7 +18,7 @@
 #include "chrome/common/notification_type.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
-#include "content/browser/plugin_service.h"
+#include "googleurl/src/url_parse.h"
 #include "googleurl/src/url_util.h"
 #include "net/base/escape.h"
 #include "net/base/net_util.h"
@@ -31,7 +30,6 @@
 HistoryQuickProvider::HistoryQuickProvider(ACProviderListener* listener,
                                            Profile* profile)
     : HistoryProvider(listener, profile, "HistoryQuickProvider"),
-      trim_http_(false),
       languages_(profile_->GetPrefs()->GetString(prefs::kAcceptLanguages)) {}
 
 HistoryQuickProvider::~HistoryQuickProvider() {}
@@ -45,7 +43,6 @@
     return;
 
   autocomplete_input_ = input;
-  trim_http_ = !HasHTTPScheme(input.text());
 
   // Do some fixup on the user input before matching against it, so we provide
   // good results for local file paths, input with spaces, etc.
@@ -80,41 +77,47 @@
   term_string = UnescapeURLComponent(term_string,
       UnescapeRule::SPACES | UnescapeRule::URL_SPECIAL_CHARS);
   history::InMemoryURLIndex::String16Vector terms(
-      HistoryQuickProvider::WordVectorFromString16(term_string));
+      InMemoryURLIndex::WordVectorFromString16(term_string, false));
   ScoredHistoryMatches matches = GetIndex()->HistoryItemsForTerms(terms);
+  if (matches.empty())
+    return;
 
   size_t match_num = matches.size() - 1;
   for (ScoredHistoryMatches::const_iterator match_iter = matches.begin();
        match_iter != matches.end(); ++match_iter, --match_num) {
     const ScoredHistoryMatch& history_match(*match_iter);
-    AutocompleteMatch ac_match =
-        QuickMatchToACMatch(history_match, NORMAL, match_num);
-    matches_.push_back(ac_match);
+    if (history_match.raw_score > 0) {
+      AutocompleteMatch ac_match =
+          QuickMatchToACMatch(history_match, match_num);
+      matches_.push_back(ac_match);
+    }
   }
 }
 
 AutocompleteMatch HistoryQuickProvider::QuickMatchToACMatch(
     const ScoredHistoryMatch& history_match,
-    MatchType match_type,
     size_t match_number) {
   const history::URLRow& info = history_match.url_info;
+  size_t prefix_end;
+  string16 unstripped_url = net::FormatUrl(info.url(), languages_,
+      net::kFormatUrlOmitNothing, UnescapeRule::SPACES, NULL, &prefix_end,
+      NULL);
+  size_t adjust = HasHTTPScheme(unstripped_url) ? prefix_end : 0;
+
   int score = CalculateRelevance(history_match.raw_score,
                                  autocomplete_input_.type(),
-                                 match_type, match_number);
-  AutocompleteMatch match(this, score, !!info.visit_count(),
-                          AutocompleteMatch::HISTORY_URL);
+                                 NORMAL, match_number);
+  AutocompleteMatch::Type match_type = history_match.url_matches.empty() ?

[Peter Kasting, 2011/03/09 02:31:48]

Nit: Just inline this into the next expression.

[mrossetti, 2011/03/10 01:31:14]

Done.

+      AutocompleteMatch::HISTORY_URL : AutocompleteMatch::HISTORY_TITLE;
+  AutocompleteMatch match(this, score, !!info.visit_count(), match_type);
   match.destination_url = info.url();
   DCHECK(match.destination_url.is_valid());
   size_t inline_autocomplete_offset =
       history_match.input_location + autocomplete_input_.text().length();
-  const net::FormatUrlTypes format_types = net::kFormatUrlOmitAll &
-      ~((trim_http_ && !history_match.match_in_scheme) ?
-          0 : net::kFormatUrlOmitHTTP);
-  std::string languages =
-      match_type == WHAT_YOU_TYPED ? std::string() : languages_;
+  const net::FormatUrlTypes format_types = net::kFormatUrlOmitAll;
   match.fill_into_edit =
       AutocompleteInput::FormattedStringWithEquivalentMeaning(info.url(),
-          net::FormatUrl(info.url(), languages, format_types,
+          net::FormatUrl(info.url(), languages_, format_types,
                          UnescapeRule::SPACES, NULL, NULL,
                          &inline_autocomplete_offset));
   if (!autocomplete_input_.prevent_inline_autocomplete())
@@ -122,27 +125,16 @@
   DCHECK((match.inline_autocomplete_offset == string16::npos) ||
          (match.inline_autocomplete_offset <= match.fill_into_edit.length()));
 
-  size_t match_start = history_match.input_location;
-  match.contents = net::FormatUrl(info.url(), languages, format_types,
-                                  UnescapeRule::SPACES, NULL, NULL,
-                                  &match_start);
-  if ((match_start != string16::npos) &&
-      (inline_autocomplete_offset != string16::npos) &&
-      (inline_autocomplete_offset != match_start)) {
-    DCHECK(inline_autocomplete_offset > match_start);
-    AutocompleteMatch::ClassifyLocationInString(match_start,
-        inline_autocomplete_offset - match_start, match.contents.length(),
-        ACMatchClassification::URL, &match.contents_class);
-  } else {
-    AutocompleteMatch::ClassifyLocationInString(string16::npos, 0,
-        match.contents.length(), ACMatchClassification::URL,
-        &match.contents_class);
-  }
+  // Format the URL autocomplete presentation.
+  match.contents = net::FormatUrl(info.url(), languages_, format_types,
+                                  UnescapeRule::SPACES, NULL, NULL, NULL);
+  match.contents_class = SpansFromTermMatch(history_match.url_matches,
+                                            match.contents.size(), adjust);
+
+  // Format the description autocomplete presentation.
   match.description = info.title();
-  AutocompleteMatch::ClassifyMatchInString(autocomplete_input_.text(),
-                                           info.title(),
-                                           ACMatchClassification::NONE,
-                                           &match.description_class);
+  match.description_class = SpansFromTermMatch(history_match.title_matches,
+                                               match.description.size(), 0);
 
   return match;
 }
@@ -165,21 +157,6 @@
   index_for_testing_.reset(index);
 }
 
-// Utility Functions
-
-history::InMemoryURLIndex::String16Vector
-    HistoryQuickProvider::WordVectorFromString16(const string16& uni_string) {
-  history::InMemoryURLIndex::String16Vector words;
-  base::BreakIterator iter(&uni_string, base::BreakIterator::BREAK_WORD);
-  if (iter.Init()) {
-    while (iter.Advance()) {
-      if (iter.IsWord())
-        words.push_back(iter.GetString());
-    }
-  }
-  return words;
-}
-
 // static
 int HistoryQuickProvider::CalculateRelevance(int raw_score,
                                              AutocompleteInput::Type input_type,
@@ -196,3 +173,34 @@
       return 900 + static_cast<int>(match_number);
   }
 }
+
+// static
+ACMatchClassifications HistoryQuickProvider::SpansFromTermMatch(
+    const history::TermMatches& matches, size_t text_length, size_t adjust) {

[Peter Kasting, 2011/03/09 02:31:48]

Nit: One arg per line

[mrossetti, 2011/03/10 01:31:14]

Yep, I reread the style guide and get it now.

+  ACMatchClassifications spans;
+  if (matches.empty()) {
+    if (text_length)
+      spans.push_back(ACMatchClassification(0, ACMatchClassification::NONE));
+    return spans;
+  }
+  if (matches[0].offset > adjust)
+    spans.push_back(ACMatchClassification(0, ACMatchClassification::NONE));
+  size_t match_count = matches.size();
+  for (size_t i = 0; i < match_count; ++i) {
+    size_t offset = matches[i].offset - adjust;

[Peter Kasting, 2011/03/09 02:31:48]

It's not obvious from the surrounding code that |adjust| must not be greater
than matches[i].offset.  e.g. What happens when the user types "h", "t", or "p"?

[mrossetti, 2011/03/10 01:31:14]

Ah! Excellent catch! I've modified the scoring function to eliminate
consideration of a prefix of 'http://'. Thank you.

[Peter Kasting, 2011/03/10 02:13:43]

BTW, something you should know about scoring.

Prefixes like the scheme or an initial "ftp." or "www." matter to scoring.  We
want to score matches that don't match in those sections better than ones that
do.  For example, if I type "w", "washingtonmutual.com" is a better match than
"www.foo.com".

This detail is important because it prevents "w" or "h" from always
inline-autocompleting to the most-commonly visited URL (which is generally
"http://www....").  With Instant on, this is really noticeable.  There's
currently a bug against the HistoryURLProvider where we're not rigorous enough
about taking that into account.

+    spans.push_back(ACMatchClassification(offset,
+                                          ACMatchClassification::MATCH));
+    // Skip all adjacent matches.
+    offset += matches[i].length;

[Peter Kasting, 2011/03/09 02:31:48]

Nit: Shorter:

    do {
      offset += matches[i].length;
      ++i;
    } while ((i < match_count) && (offset == matches[i].offset - adjust));

Doing this means you need to remove the "++i" from the outermost for().

[mrossetti, 2011/03/10 01:31:14]

Sweet!

+    while ((i + 1 < match_count) && (offset == matches[i+1].offset - adjust)) {
+      ++i;
+      offset += matches[i].length;
+    }
+    if (offset < text_length) {
+      spans.push_back(ACMatchClassification(offset,
+                                            ACMatchClassification::NONE));
+    }
+  }
+
+  return spans;
+}