Omnibox: Make HistoryURL Highlight Titles like HistoryQuick Provider

chrome/browser/autocomplete/history_url_provider.cc

Index: chrome/browser/autocomplete/history_url_provider.cc
diff --git a/chrome/browser/autocomplete/history_url_provider.cc b/chrome/browser/autocomplete/history_url_provider.cc
index d28038a4fa50e49f1dba9ae0a690574e8b3aa2bf..aef336d114c97b8460b0457e28a906facf2f7c55 100644
--- a/chrome/browser/autocomplete/history_url_provider.cc
+++ b/chrome/browser/autocomplete/history_url_provider.cc
@@ -21,6 +21,8 @@
 #include "chrome/browser/history/history_service.h"
 #include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/history/history_types.h"
+#include "chrome/browser/history/in_memory_url_index_types.h"
+#include "chrome/browser/history/scored_history_match.h"
 #include "chrome/browser/omnibox/omnibox_field_trial.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/search_engines/template_url_service.h"
@@ -771,10 +773,8 @@ bool HistoryURLProvider::FixupExactSuggestion(
       match->deletable = true;
       match->description = classifier.url_row().title();
       RecordAdditionalInfoFromUrlRow(classifier.url_row(), match);
-      AutocompleteMatch::ClassifyMatchInString(
-          input.text(),
-          classifier.url_row().title(),
-          ACMatchClassification::NONE, &match->description_class);
+      match->description_class =
+          ClassifyDescription(input.text(), 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
@@ -1087,10 +1087,28 @@ AutocompleteMatch HistoryURLProvider::HistoryMatchToACMatch(
         &match.contents_class);
   }
   match.description = info.title();
-  AutocompleteMatch::ClassifyMatchInString(params.input.text(),
-                                           info.title(),
-                                           ACMatchClassification::NONE,
-                                           &match.description_class);
+  match.description_class =
+      ClassifyDescription(params.input.text(), match.description);
   RecordAdditionalInfoFromUrlRow(info, &match);
   return match;
 }
+
+// static
+ACMatchClassifications HistoryURLProvider::ClassifyDescription(
+    const string16& input_text,
+    const string16& description) {
+  string16 clean_description = history::CleanUpTitleForMatching(description);
+  // Treat the input as a single term.  This probably causes no highlighting
+  // if it's multiple terms such as mail.g.  That seems reasonable.

[Peter Kasting, 2013/11/23 02:15:36]

Wait, does this mean "mail.g" will no longer show any bolding at the beginning
of "mail.google.com"?

[Mark P, 2013/11/25 19:59:16]

If the "title" of the page is "mail.google.com" (which I doubt), then yes.  But
it's important to note that this changelist only affects *titles*.

[Peter Kasting, 2013/11/26 02:15:36]

That does limit the scope of the damage, but it still seems suboptimal. 
Wouldn't we have similar problems with any string with periods in it?  For
example, imagine a page on Pi whose title contains "3.14159...", with us no
longer highlighting a match like "3.141" in it.

Is there a way to do even better here?  I'm not familiar enough with the code to
know.

[Mark P, 2013/11/26 20:08:15]

While looking at the code to see how I can make it do better, I realized the
code looked like it works right in this case (even though I didn't expect it
to).  I tested it (looking at title highlighting after suppressing HistoryQuick
and Shortcuts) and everything looked right.  I checked with about:omnibox that
the matches and highlighting I was seeing came from HistoryURL provider using
this code.

I removed the comment warning that the code might break on multi-term inputs.

+  history::TermMatches description_matches =

[Peter Kasting, 2013/11/23 02:15:36]

Nit: Consider using () instead of = for non-basic types for clarity/efficiency
reasons

[Mark P, 2013/11/25 19:59:16]

Okay.

+      history::MatchTermInString(input_text, clean_description, 0);
+  description_matches = SortAndDeoverlapMatches(description_matches);

[Peter Kasting, 2013/11/23 02:15:36]

Nit: Combine with previous line

[Mark P, 2013/11/25 19:59:16]

Done.

+  history::WordStarts description_word_starts;
+  history::String16VectorFromString16(
+      clean_description, false, &description_word_starts);
+  description_matches = history::ScoredHistoryMatch::

[Peter Kasting, 2013/11/23 02:15:36]

Nit: If we break after '=' can we avoid breaking after "::"?

[Mark P, 2013/11/25 19:59:16]

Yes.  Done.

+      FilterTermMatchesByWordStarts(
+          description_matches, description_word_starts, 0);
+  return SpansFromTermMatch(
+      description_matches, clean_description.length(), false);
+}