Omnibox - Allow Matching in Port Number

components/omnibox/browser/scored_history_match.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 "components/omnibox/browser/scored_history_match.h"
 
 #include <math.h>
 
 #include <algorithm>
 #include <vector>
@@ skipping 450 matching lines @@
   // The + 3 skips the // that probably appears in the protocol
   // after the colon.  If the protocol doesn't have two slashes after
   // the colon, that's okay--all this ends up doing is starting our
   // search for the next / a few characters into the hostname.  The
   // only times this can cause problems is if we have a protocol without
   // a // after the colon and the hostname is only one or two characters.
   // This isn't worth worrying about.
   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).
   url_matches = FilterTermMatchesByWordStarts(
       url_matches, terms_to_word_starts_offsets, word_starts.url_word_starts_,
       end_of_hostname_pos, std::string::npos);
   if (colon_pos != std::string::npos) {
     // Also filter matches not at a word boundary and in the scheme.
     url_matches = FilterTermMatchesByWordStarts(
         url_matches, terms_to_word_starts_offsets, word_starts.url_word_starts_,
@@ skipping 13 matching lines @@
     if ((question_mark_pos != std::string::npos) &&
         (url_match.offset > question_mark_pos)) {
       // The match is in a CGI ?... fragment.
       DCHECK(at_word_boundary);
       term_scores[url_match.term_num] += 5;
     } else if ((end_of_hostname_pos != std::string::npos) &&
                (url_match.offset > end_of_hostname_pos)) {
       // 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.
       if ((last_part_of_hostname_pos == std::string::npos) ||
           (url_match.offset < last_part_of_hostname_pos)) {
         // Either there are no dots in the hostname or this match isn't
         // the last dotted component.
         term_scores[url_match.term_num] += at_word_boundary ? 10 : 2;
       } else {
         // The match is in the last part of a dotted hostname (usually this
@@ skipping 212 matching lines @@
           base::StringToDouble(it->first, &bucket.first);
       DCHECK(is_valid_intermediate_score);
       bool is_valid_hqp_score = base::StringToInt(it->second, &bucket.second);
       DCHECK(is_valid_hqp_score);
       hqp_buckets->push_back(bucket);
     }
     return true;
   }
   return false;
 }