Omnibox: Fix HistoryQuick provider can_inline bug

chrome/browser/history/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 "chrome/browser/history/scored_history_match.h"
 
 #include <algorithm>
 #include <functional>
 #include <iterator>
 #include <numeric>
@@ skipping 79 matching lines @@
   // TODO(mpearson): Investigate whether this has any meaningful
   // effect on scoring.  (It's necessary at some point: removing
   // overlaps and sorting is needed to decide what to highlight in the
   // suggestion string.  But this sort and de-overlap doesn't have to
   // be done before scoring.)
   url_matches = SortAndDeoverlapMatches(url_matches);
   title_matches = SortAndDeoverlapMatches(title_matches);
 
   // We can inline autocomplete a result if:
   //  1) there is only one search term
-  //  2) AND EITHER:
-  //    2a) the first match starts at the beginning of the candidate URL, OR
-  //    2b) the candidate URL starts with one of the standard URL prefixes with
-  //        the URL match immediately following that prefix.
+  //  2) AND a match begins immediately after a droppabled (inlineable)

[Peter Kasting, 2012/09/11 17:52:50]

Nit: droppabled?

[Mark P, 2012/09/11 21:52:17]

Not sure if you wanted me to fix the word (drop typo final "d") or use a
different word.  I changed the word to implicit.

+  //     prefix such as http://www or https://
   //  3) AND the search string does not end in whitespace (making it look to
   //     the IMUI as though there is a single search term when actually there
   //     is a second, empty term).
+  // |best_prefix| stores the inlineable prefix computed in clause (2) or
+  // NULL if no such prefix exists.  (The URL is not inlineable.)
+  const URLPrefix* best_prefix = NULL;

[mrossetti, 2012/09/11 17:36:10]

I'm okay with doing the assignment within the conditional but I don't recall
seeing this pattern elsewhere (though I haven't looked awfully hard). On the
other hand, you could simplify this by determining best_prefix outside the
conditional:

const URLPrefix* best_prefix = !url_matches.empty() && terms.size() == 1 ?
   URLPrefix:: BestURLPrefix(UTF8ToUTF16(gurl.spec()), terms[0]) : NULL;
can_inline = best_prefix && !IsWhitespace(*(lower_string.rbegin()));
match_in_scheme = can_inline && (best_prefix->prefix.length() == 0);

[Peter Kasting, 2012/09/11 17:52:50]

I dislike assignments inside conditionals, so I'm a fan of what Mike proposes,
except for use ".empty()" instead of ".length() == 0".

[Mark P, 2012/09/11 21:52:17]

Took your suggestion (with a bit more explicit logic, e.g. != NULL) with Peter's
.empty() twist.

[Mark P, 2012/09/11 21:52:17]

I don't like them either, but I've seen enough Chrome code that has foo(...) &&
bar(...) && gee(...) where one of those functions (say, bar) has side effects
that change a variable, so I figured I'd go with the flow.
Done.

   can_inline = !url_matches.empty() &&
       terms.size() == 1 &&
-      (url_matches[0].offset == 0 ||
-       URLPrefix::IsURLPrefix(url.substr(0, url_matches[0].offset))) &&
+      ((best_prefix = URLPrefix::
+          BestURLPrefix(UTF8ToUTF16(gurl.spec()), terms[0])) != NULL) &&
       !IsWhitespace(*(lower_string.rbegin()));
-  match_in_scheme = can_inline && url_matches[0].offset == 0;
+  match_in_scheme = can_inline && (best_prefix->prefix.length() == 0);
 
   // Determine if the associated URLs is referenced by any bookmarks.
   float bookmark_boost =
       (bookmark_service && bookmark_service->IsBookmarked(gurl)) ? 10.0 : 0.0;
 
   if (use_new_scoring) {
     const float topicality_score = GetTopicalityScore(
         terms.size(), url, url_matches, title_matches, word_starts);
     const float recency_score = GetRecencyScore(
         (now - row.last_visit()).InDays());
@@ skipping 408 matching lines @@
   // Add a beacon to the logs that'll allow us to identify later what
   // new scoring state a user is in.  Do this by incrementing a bucket in
   // a histogram, where the bucket represents the user's new scoring state.
   UMA_HISTOGRAM_ENUMERATION(
       "Omnibox.HistoryQuickProviderNewScoringFieldTrialBeacon",
       new_scoring_option, NUM_OPTIONS);
 
 }
 
 }  // namespace history