Omnibox: Bug Fixes for Shortcuts Inlining

Omnibox: Bug Fixes for Shortcuts Inlining

Precisely, fixes up URL-like inputs before comparing to the shortcuts
fill_into_edit in order to better determine inlineability.

This is especially useful for inputs such as about: which need to have
their scheme fixed up to be recognized as comparable to chrome://
shortcuts.

BUG=328220
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=242029

R=pkasting@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=242119

[Mark P, 2013-12-17 19:29:01]

Hi Peter,

Hope you had a good break!

What do you think about this approach to fix the ("CENTRAL ISSUE") in the linked
bug?

--mark

https://codereview.chromium.org/108643003/diff/40001/chrome/browser/autocompl...
File chrome/browser/autocomplete/shortcuts_provider.cc (right):

https://codereview.chromium.org/108643003/diff/40001/chrome/browser/autocompl...
chrome/browser/autocomplete/shortcuts_provider.cc:239: if (url.is_valid()) {
Alternatively, do I want to skip is_valid() and simply use
possibly_invalid_spec()?  I'm not sure.

https://codereview.chromium.org/108643003/diff/40001/chrome/browser/autocompl...
chrome/browser/autocomplete/shortcuts_provider.cc:257: }
I think this strategy works but I'm not positive.  I think the simplest case
where it might be able fail is this: I'm trying to come up with an example where
we need this FixupURL() strategy but want to match against a fill_into_edit that
lacks a scheme.  Can you come up with a failure example like this?

[Peter Kasting, 2013-12-17 19:56:48]

I'm worried about this handcoded addition.  I feel like using
HistoryProvider::FixupUserInput() might be safer if doing so is possible.  After
all, that is trying to do something similar (prepare some user input for
checking against the prefixes of other known things) and has a whole bunch of
special-casing logic to deal with problems that can occur when doing that --
which I worry may be applicable here too.

It may be that this is only usable if we also fix up the original fill_into_edit
similarly (see comment below).

It may be that we want to do this all the time, not just if BestPrefix fails.

https://codereview.chromium.org/108643003/diff/40001/chrome/browser/autocompl...
File chrome/browser/autocomplete/shortcuts_provider.cc (right):

https://codereview.chromium.org/108643003/diff/40001/chrome/browser/autocompl...
chrome/browser/autocomplete/shortcuts_provider.cc:234: // inline against URL
shortcuts.  This is especially userful to get
Nit: userful -> useful

https://codereview.chromium.org/108643003/diff/40001/chrome/browser/autocompl...
chrome/browser/autocomplete/shortcuts_provider.cc:236: // are fixed up to the
chrome:// scheme.)
What about if the fill_into_edit is about:foo and the term_string is chrome://f?
 Should this be able to match?  If so we might need to fix up both
fill_into_edit and term_string?

https://codereview.chromium.org/108643003/diff/40001/chrome/browser/autocompl...
chrome/browser/autocomplete/shortcuts_provider.cc:239: if (url.is_valid()) {
On 2013/12/17 19:29:02, Mark P wrote:
> Alternatively, do I want to skip is_valid() and simply use
> possibly_invalid_spec()?  I'm not sure.

I think so.  "about:" might get fixed to "chrome://" which is probably invalid
but would be useful to get here.

https://codereview.chromium.org/108643003/diff/40001/chrome/browser/autocompl...
chrome/browser/autocomplete/shortcuts_provider.cc:246: // the default match. 
(Fill_into_edit may be missing the trailing
Nit: Fill -> fill

https://codereview.chromium.org/108643003/diff/40001/chrome/browser/autocompl...
chrome/browser/autocomplete/shortcuts_provider.cc:257: }
On 2013/12/17 19:29:02, Mark P wrote:
> I think this strategy works but I'm not positive.  I think the simplest case
> where it might be able fail is this: I'm trying to come up with an example
where
> we need this FixupURL() strategy but want to match against a fill_into_edit
that
> lacks a scheme.  Can you come up with a failure example like this?

Check i18n domain names and escaped versus unescaped paths.

[Mark P, 2013-12-17 20:36:17]

>>>
I'm worried about this handcoded addition.  I feel like using
HistoryProvider::FixupUserInput() might be safer if doing so is possible.  After
all, that is trying to do something similar (prepare some user input for
checking against the prefixes of other known things) and has a whole bunch of
special-casing logic to deal with problems that can occur when doing that --
which I worry may be applicable here too.

It may be that this is only usable if we also fix up the original fill_into_edit
similarly (see comment below).

It may be that we want to do this all the time, not just if BestPrefix fails.
>>>

I made the changes you suggested, comments attached.  Then I read this message
in more detail.  I think using FixupUserInput() will be good, if not better than
the current situation.  Looking at exactly what it does, I think if I use it I
can do the Fixup(), then pass the fixed up version to BestURL prefix and not
need this second case at all.  It's also remove the need for special-casing
trailing slashes.  If it works (fixup before besturl), would that be okay with
you?

Or maybe I should try the original with besturlprefix and, if that fails, try
the fixed-up version through besturlprefix.  That seems more resilient.  Even
this would be shorter than what I currently have.

--mark

https://codereview.chromium.org/108643003/diff/40001/chrome/browser/autocompl...
File chrome/browser/autocomplete/shortcuts_provider.cc (right):

https://codereview.chromium.org/108643003/diff/40001/chrome/browser/autocompl...
chrome/browser/autocomplete/shortcuts_provider.cc:234: // inline against URL
shortcuts.  This is especially userful to get
On 2013/12/17 19:56:49, Peter Kasting wrote:
> Nit: userful -> useful

Done.

https://codereview.chromium.org/108643003/diff/40001/chrome/browser/autocompl...
chrome/browser/autocomplete/shortcuts_provider.cc:236: // are fixed up to the
chrome:// scheme.)
On 2013/12/17 19:56:49, Peter Kasting wrote:
> What about if the fill_into_edit is about:foo and the term_string is
chrome://f?
>  Should this be able to match?  If so we might need to fix up both
> fill_into_edit and term_string?

This will not happen.  There's no way to select a match from the omnibox
dropdown that has a fill_into_edit of about:foo.  All fill_into_edits (whether
from HistoryURL or BuiltinProvider) are already fixed up.

https://codereview.chromium.org/108643003/diff/40001/chrome/browser/autocompl...
chrome/browser/autocomplete/shortcuts_provider.cc:239: if (url.is_valid()) {
On 2013/12/17 19:56:49, Peter Kasting wrote:
> On 2013/12/17 19:29:02, Mark P wrote:
> > Alternatively, do I want to skip is_valid() and simply use
> > possibly_invalid_spec()?  I'm not sure.
> 
> I think so.  "about:" might get fixed to "chrome://" which is probably invalid
> but would be useful to get here.

Done.  I thought I tested this case but apparently I didn't...  I thought I
tested it before I saw at least one shortcut could be inlined against about:. 
Now I look closer and I find another problem:
about:
gets fixed up to
chrome://version

This allows chrome://version to inline (and hence fixes the bug by making sure a
non-search can inline) but it's misleading in that all the other shortcuts get
marked as non-inlineable.  In other words, no matter how good a chrome://omnibox
shortcut is for me, it'll never be returned for about:.  I'm not sure how much I
should be concerned about this new discovery.  I believe it should only affect
"about:", "about:/", and "about://".

Note that "about", "abou", "abo", will never match against BuiltinProvider
shortcuts (which are always stored in the form chrome://).  We would need more
special code to deal handle this.

https://codereview.chromium.org/108643003/diff/40001/chrome/browser/autocompl...
chrome/browser/autocomplete/shortcuts_provider.cc:246: // the default match. 
(Fill_into_edit may be missing the trailing
On 2013/12/17 19:56:49, Peter Kasting wrote:
> Nit: Fill -> fill

Done.

https://codereview.chromium.org/108643003/diff/40001/chrome/browser/autocompl...
chrome/browser/autocomplete/shortcuts_provider.cc:257: }
On 2013/12/17 19:56:49, Peter Kasting wrote:
> On 2013/12/17 19:29:02, Mark P wrote:
> > I think this strategy works but I'm not positive.  I think the simplest case
> > where it might be able fail is this: I'm trying to come up with an example
> where
> > we need this FixupURL() strategy but want to match against a fill_into_edit
> that
> > lacks a scheme.  Can you come up with a failure example like this?
> 
> Check i18n domain names and escaped versus unescaped paths.

Seems to work.  I can only manage to get shortcut's database to have properly
escaped fill_into_edit (because other providers set the fill_into_edit
correctly), so when I fixup the user input, it matches correctly.

[Peter Kasting, 2013-12-17 21:21:35]

On 2013/12/17 20:36:17, Mark P wrote:
> I think using FixupUserInput() will be good, if not better than
> the current situation.  Looking at exactly what it does, I think if I use it I
> can do the Fixup(), then pass the fixed up version to BestURL prefix and not
> need this second case at all.  It's also remove the need for special-casing
> trailing slashes.  If it works (fixup before besturl), would that be okay with
> you?

Yeah, if that works, that feels safest and best to me.  I wonder if that would
fix the "about: -> chrome://version" issue too, though probably not, since the
core of FixupUserInput() is still a FixupURL() call.  (Maybe we could
special-case detect that.)

I think this is probably better than trying BestURLPrefix() first and then
calling FixupUserInput(), unless there's some case that obviously doesn't work
well.

[Mark P, 2013-12-17 22:52:40]

New approach, using the two-attempt version (regular input, plus fixed up
input).  This feels safer and there's no harm that can come from it.

--mark

https://codereview.chromium.org/108643003/diff/80001/chrome/browser/autocompl...
File chrome/browser/autocomplete/history_provider.h (right):

https://codereview.chromium.org/108643003/diff/80001/chrome/browser/autocompl...
chrome/browser/autocomplete/history_provider.h:33: static bool
FixupUserInput(AutocompleteInput* input);
This is a simple cut-and-paste of FixupUserInput from the protected section to
the public section.  The diff highlighting is icky.

[Peter Kasting, 2013-12-17 23:04:22]

Obviously, this needs some tests.

It would be nice to come up with a test that requires the two-pass algorithm,
and would fail if we only attempted to match against the fixed-up input or
something.

https://codereview.chromium.org/108643003/diff/100001/chrome/browser/autocomp...
File chrome/browser/autocomplete/history_provider.h (right):

https://codereview.chromium.org/108643003/diff/100001/chrome/browser/autocomp...
chrome/browser/autocomplete/history_provider.h:33: static bool
FixupUserInput(AutocompleteInput* input);
Perhaps we should actually move this to AutocompleteProvider's protected section
instead of HistoryProvider's public section?

[Mark P, 2013-12-19 23:18:56]

I implemented the approach we discussed, plus added tests.  lease take a look.

--mark

https://codereview.chromium.org/108643003/diff/100001/chrome/browser/autocomp...
File chrome/browser/autocomplete/history_provider.h (right):

https://codereview.chromium.org/108643003/diff/100001/chrome/browser/autocomp...
chrome/browser/autocomplete/history_provider.h:33: static bool
FixupUserInput(AutocompleteInput* input);
On 2013/12/17 23:04:22, Peter Kasting wrote:
> Perhaps we should actually move this to AutocompleteProvider's protected
section
> instead of HistoryProvider's public section?

I'm not sure.  It sounds like you're not either.  I'm slightly inclined to leave
it where it is because I think it's best deserves to be in a URL-focused class. 
After all, it kind of assuming the input is URL-like in doing its fixups; I'm
worried that if we put it in AutocompleteProvider (at least without changing the
name) then it'll be inviting misuse.

[Peter Kasting, 2013-12-19 23:32:27]

LGTM.  I don't remember actually discussing moving BestURLPrefixWithWWWCase()
into the class -- I would probably keep it out, and use CR_DEFINE_STATIC_LOCAL
if constructing a URLPrefix object is truly expensive (which I doubt) -- but if
you think it's really a win, it's fine.

https://codereview.chromium.org/108643003/diff/100001/chrome/browser/autocomp...
File chrome/browser/autocomplete/history_provider.h (right):

https://codereview.chromium.org/108643003/diff/100001/chrome/browser/autocomp...
chrome/browser/autocomplete/history_provider.h:33: static bool
FixupUserInput(AutocompleteInput* input);
On 2013/12/19 23:18:58, Mark P wrote:
> On 2013/12/17 23:04:22, Peter Kasting wrote:
> > Perhaps we should actually move this to AutocompleteProvider's protected
> section
> > instead of HistoryProvider's public section?
> 
> I'm not sure.  It sounds like you're not either.  I'm slightly inclined to
leave
> it where it is because I think it's best deserves to be in a URL-focused
class. 
> After all, it kind of assuming the input is URL-like in doing its fixups; I'm
> worried that if we put it in AutocompleteProvider (at least without changing
the
> name) then it'll be inviting misuse.

Hmm.  I guess I don't have that sense.  It seems like if this is useful for
non-history providers, making it available to non-history providers is probably
better.  Keeping it in HistoryProvider doesn't just make it seem "URL-focused",
it makes it seem like this is only useful when the history provider is actually
involved somehow.

I don't feel strongly enough to mandate a change, though, so do what you think
best.

[Mark P, 2013-12-20 00:39:19]

On 2013/12/19 23:32:27, Peter Kasting wrote:
> LGTM.  I don't remember actually discussing moving BestURLPrefixWithWWWCase()
> into the class -- I would probably keep it out, and use CR_DEFINE_STATIC_LOCAL
> if constructing a URLPrefix object is truly expensive (which I doubt) -- but
if
> you think it's really a win, it's fine.
> 

Switched to CR_DEFINE_STATIC_LOCAL (which I forgot exists), and so moved the
class function back into the anonymous namespace.

--mark

https://codereview.chromium.org/108643003/diff/100001/chrome/browser/autocomp...
File chrome/browser/autocomplete/history_provider.h (right):

https://codereview.chromium.org/108643003/diff/100001/chrome/browser/autocomp...
chrome/browser/autocomplete/history_provider.h:33: static bool
FixupUserInput(AutocompleteInput* input);
On 2013/12/19 23:32:27, Peter Kasting wrote:
> On 2013/12/19 23:18:58, Mark P wrote:
> > On 2013/12/17 23:04:22, Peter Kasting wrote:
> > > Perhaps we should actually move this to AutocompleteProvider's protected
> > section
> > > instead of HistoryProvider's public section?
> > 
> > I'm not sure.  It sounds like you're not either.  I'm slightly inclined to
> leave
> > it where it is because I think it's best deserves to be in a URL-focused
> class. 
> > After all, it kind of assuming the input is URL-like in doing its fixups;
I'm
> > worried that if we put it in AutocompleteProvider (at least without changing
> the
> > name) then it'll be inviting misuse.
> 
> Hmm.  I guess I don't have that sense.  It seems like if this is useful for
> non-history providers, making it available to non-history providers is
probably
> better.  Keeping it in HistoryProvider doesn't just make it seem
"URL-focused",
> it makes it seem like this is only useful when the history provider is
actually
> involved somehow.
> 
> I don't feel strongly enough to mandate a change, though, so do what you think
> best.

Done.  Had to move TrimHttpPrefix() too.

[commit-bot: I haz the power, 2013-12-20 00:40:43]

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

[commit-bot: I haz the power, 2013-12-20 07:27:56]

Change committed as 242029

[Mark P, 2013-12-20 17:42:07]

Committed patchset #10 manually as r242119 (presubmit successful).