Omnibox - Allow Matching in Port Number

components/omnibox/browser/scored_history_match.cc

Index: components/omnibox/browser/scored_history_match.cc
diff --git a/components/omnibox/browser/scored_history_match.cc b/components/omnibox/browser/scored_history_match.cc
index df76edb89d92bbd4915f7862035f25db6b97faed..f7f7c3c55c792393ff36df897e1f41d4ce351808 100644
--- a/components/omnibox/browser/scored_history_match.cc
+++ b/components/omnibox/browser/scored_history_match.cc
@@ -468,9 +468,21 @@ float ScoredHistoryMatch::GetTopicalityScore(
   const size_t end_of_hostname_pos = (colon_pos != std::string::npos)
                                          ? url.find('/', colon_pos + 3)
                                          : url.find('/');
-  size_t last_part_of_hostname_pos = (end_of_hostname_pos != std::string::npos)
-                                         ? url.rfind('.', end_of_hostname_pos)
-                                         : url.rfind('.');
+  const size_t last_part_of_hostname_pos =
+      (end_of_hostname_pos != std::string::npos)
+      ? url.rfind('.', end_of_hostname_pos)
+      : url.rfind('.');
+  // Find the port in the last part of the hostname if we've identified such.
+  // Otherwise, find it starting from later in the URL and looking backwards.
+  size_t port_pos = (last_part_of_hostname_pos != std::string::npos)
+      ? url.find(':', last_part_of_hostname_pos)
+      : url.rfind(':',
+          (end_of_hostname_pos != std::string::npos)
+          ? end_of_hostname_pos
+          : url.length());
+  // If we've found the colon in the scheme, that's not the port!
+  if (port_pos <= colon_pos)
+    port_pos = std::string::npos;

[Peter Kasting, 2016/09/20 20:44:55]

I'm really uncomfortable with all this code.

A claim (on a different bug) was that we cannot use the eTLD service because of
perf concerns.  But the code in this function (not just in this changed block)
is already really inefficient because it does a bunch of different find() calls
that each can scan the whole string.  And the chance for errors is worrisome. 
There are already multiple known errors in this that "aren't worth worrying
about" and the aforementioned open bug that you punted.  What else is
problematic, now or later?

It seems like rewriting to use the standard machinery for parsing URLs can
locate things like the hostname boundaries, port, CGI params, etc. with complete
accuracy, in noticeably less code, and I'm not convinced will lose performance. 
I think it is worth taking the time to do.

   // Loop through all URL matches and score them appropriately.
   // First, filter all matches not at a word boundary and in the path (or
   // later).
@@ -504,6 +516,12 @@ float ScoredHistoryMatch::GetTopicalityScore(
       // The match is in the path.
       DCHECK(at_word_boundary);
       term_scores[url_match.term_num] += 8;
+    } else if ((port_pos != std::string::npos) &&
+               (url_match.offset > port_pos)) {
+      // The match is in the port.
+      // (This'll also trigger for the last component of raw IPv6 addresses.
+      // This situation isn't worth worrying about.)
+      term_scores[url_match.term_num] += at_word_boundary ? 5 : 0;
     } else if ((colon_pos == std::string::npos) ||
                (url_match.offset > colon_pos)) {
       // The match is in the hostname.