Omnibox: Fix HistoryQuick provider can_inline bug

Omnibox: Fix HistoryQuick provider can_inline bug

The HistoryQuick provider erroneously sets can_inline to false when the user's input matches mid-word in the scheme, even if the scheme is droppable.  For instance, for the omnibox input "t", the URL http://teams.foo.com/ would be considered not inlineable.  This is wrong.

This changelist revises the can_inline code to make it correct in these cases.

I added tests.  Before this change, the new tests fail.  Specifically, lines 158 and 174 fail.

BUG=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=156204

[Mark P, 2012-09-11 15:22:27]

Mike,

I found the problem I saw yesterday.

Once you review this, I'll add sky for OWNERS rubberstamp.

CCed to Peter as an FYI

thanks,
mark

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

LGTM with one minor suggestion.

https://chromiumcodereview.appspot.com/10907176/diff/2001/chrome/browser/hist...
File chrome/browser/history/scored_history_match.cc (right):

https://chromiumcodereview.appspot.com/10907176/diff/2001/chrome/browser/hist...
chrome/browser/history/scored_history_match.cc:107: const URLPrefix* best_prefix
= NULL;
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:49]

https://chromiumcodereview.appspot.com/10907176/diff/2001/chrome/browser/hist...
File chrome/browser/history/scored_history_match.cc (right):

https://chromiumcodereview.appspot.com/10907176/diff/2001/chrome/browser/hist...
chrome/browser/history/scored_history_match.cc:100: //  2) AND a match begins
immediately after a droppabled (inlineable)
Nit: droppabled?

https://chromiumcodereview.appspot.com/10907176/diff/2001/chrome/browser/hist...
chrome/browser/history/scored_history_match.cc:107: const URLPrefix* best_prefix
= NULL;
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]

ptal

https://chromiumcodereview.appspot.com/10907176/diff/2001/chrome/browser/hist...
File chrome/browser/history/scored_history_match.cc (right):

https://chromiumcodereview.appspot.com/10907176/diff/2001/chrome/browser/hist...
chrome/browser/history/scored_history_match.cc:100: //  2) AND a match begins
immediately after a droppabled (inlineable)
On 2012/09/11 17:52:50, Peter Kasting wrote:
> Nit: droppabled?

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.

https://chromiumcodereview.appspot.com/10907176/diff/2001/chrome/browser/hist...
chrome/browser/history/scored_history_match.cc:107: const URLPrefix* best_prefix
= NULL;
On 2012/09/11 17:36:10, mrossetti wrote:
> 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);

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

https://chromiumcodereview.appspot.com/10907176/diff/2001/chrome/browser/hist...
chrome/browser/history/scored_history_match.cc:107: const URLPrefix* best_prefix
= NULL;
On 2012/09/11 17:52:50, Peter Kasting wrote:
> I dislike assignments inside conditionals,

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.

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

Done.

[Peter Kasting, 2012-09-11 21:57:48]

LGTM

https://chromiumcodereview.appspot.com/10907176/diff/7001/chrome/browser/hist...
File chrome/browser/history/scored_history_match.cc (right):

https://chromiumcodereview.appspot.com/10907176/diff/7001/chrome/browser/hist...
chrome/browser/history/scored_history_match.cc:100: //  2) AND a match begins
immediately after a implicit / inlineable
Nit: a -> an, but maybe this would be clearer as "AND the match begins
immediately after one of the prefixes in URLPrefix (note that one of these is
the empty prefix, for cases where the user has typed the scheme)"?

https://chromiumcodereview.appspot.com/10907176/diff/7001/chrome/browser/hist...
chrome/browser/history/scored_history_match.cc:107: const URLPrefix* best_prefix
= (!url_matches.empty()) && (terms.size() == 1) ?
Nit: No need for parens around unary expression -- I'd be happy to have them
around the binary expression before '?' though

[Mark P, 2012-09-11 22:05:00]

https://chromiumcodereview.appspot.com/10907176/diff/7001/chrome/browser/hist...
File chrome/browser/history/scored_history_match.cc (right):

https://chromiumcodereview.appspot.com/10907176/diff/7001/chrome/browser/hist...
chrome/browser/history/scored_history_match.cc:100: //  2) AND a match begins
immediately after a implicit / inlineable
On 2012/09/11 21:57:48, Peter Kasting wrote:
> Nit: a -> an, but maybe this would be clearer as "AND the match begins
> immediately after one of the prefixes in URLPrefix (note that one of these is
> the empty prefix, for cases where the user has typed the scheme)"?

I like your suggestion a lot.  I took it entirely and added an example.

https://chromiumcodereview.appspot.com/10907176/diff/7001/chrome/browser/hist...
chrome/browser/history/scored_history_match.cc:107: const URLPrefix* best_prefix
= (!url_matches.empty()) && (terms.size() == 1) ?
On 2012/09/11 21:57:48, Peter Kasting wrote:
> Nit: No need for parens around unary expression -- I'd be happy to have them
> around the binary expression before '?' though

Rearranged parens.

[Mark P, 2012-09-11 23:02:40]

Adding sky for rubberstamp OWNERS approval.

[sky, 2012-09-11 23:27:22]

LGTM

[commit-bot: I haz the power, 2012-09-11 23:35:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mpearson@chromium.org/10907176/3004

[commit-bot: I haz the power, 2012-09-12 01:36:41]

Change committed as 156204