Omnibox: Fix HistoryQuick provider can_inline bug

chrome/browser/history/scored_history_match.cc

Index: chrome/browser/history/scored_history_match.cc
diff --git a/chrome/browser/history/scored_history_match.cc b/chrome/browser/history/scored_history_match.cc
index d8ee0b9aa0b76acbcab2cf6e99e2728bdf7d5a7a..fe6a72b8f30b94b9a9a9fcc51abb2a652f2520d6 100644
--- a/chrome/browser/history/scored_history_match.cc
+++ b/chrome/browser/history/scored_history_match.cc
@@ -97,19 +97,20 @@ ScoredHistoryMatch::ScoredHistoryMatch(const URLRow& row,
 
   // 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 =