Omnibox: Make HistoryURL Highlight Titles like HistoryQuick Provider

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"
 #include "base/command_line.h"
 #include "base/message_loop/message_loop.h"
 #include "base/metrics/histogram.h"
 #include "base/prefs/pref_service.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/autocomplete/autocomplete_match.h"
 #include "chrome/browser/autocomplete/autocomplete_provider_listener.h"
 #include "chrome/browser/history/history_backend.h"
 #include "chrome/browser/history/history_database.h"
 #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"
 #include "chrome/browser/search_engines/template_url_service_factory.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/net/url_fixer_upper.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
 #include "net/base/net_util.h"
 #include "net/base/registry_controlled_domains/registry_controlled_domain.h"
@@ skipping 730 matching lines @@
       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);
-      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
         // 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) ?
             UNVISITED_INTRANET : WHAT_YOU_TYPED;
       }
       break;
@@ skipping 292 matching lines @@
     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);
   }
   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);
+}