Omnibox: Make Bookmarks Set Inline_Autocompletion

Omnibox: Make Bookmarks Set Inline_Autocompletion

Make BookmarksProvider set inline_autocompletion and
allowed_to_be_default_match.

This has no effect unless bookmarks are made to score more highly (and
so they'll empirically end up as the top-scoring match).

Includes unit tests.

In the process, fix whether trailing slashes are omitted, for consistency
with other providers.  This will reduce jank if bookmarks are
ever inlining.  Tested interactively.

Likewise, fix whether http:// is omitted.  Tested interactively.

Also, refactor a common piece of code from ShortcutsProvider
to URLPrefix.

Likewise, make a protected function in HistoryProvider public
for use elsewhere.  Mark it static (because it can be).

Also, fix a PreventInlineAutocomplete bug in ShortcutsProvider.
Adds a test for this.  These tests fail before this change.

Also, fix a PreventInlineAutocomplete bug in BuiltinProvider.
Didn't bother adding a test for this because no tests examine
inline_autocompletion here and I didn't want to bother adding
some just for this minor change.  Tested it interactively.
These tests fail before this change.

BUG=

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

[Mark P, 2014-04-09 00:09:14]

ptal

[Peter Kasting, 2014-04-09 00:53:32]

https://codereview.chromium.org/229733004/diff/20001/chrome/browser/autocompl...
File chrome/browser/autocomplete/bookmark_provider.cc (right):

https://codereview.chromium.org/229733004/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/bookmark_provider.cc:169: // Determine
|inline_autocomplete_offset|.
This whole block looks suspiciously similar to what other providers do (or at
least, need to do).  Is there any opportunity to hoist similar code to a common
location?

https://codereview.chromium.org/229733004/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/bookmark_provider.cc:191: ((best_prefix != NULL) ||
(inline_autocomplete_offset != 0));
I don't think I understand this conditional.

Wouldn't this trim if the user text was "http" (since we'd get a non-null best
prefix of "")?  Also, if best_prefix is NULL, isn't inline_autocomplete_offset
== base::string16::npos, and thus nonzero, so the second line here always
evaluates to true?

[Mark P, 2014-04-09 02:44:42]

I'll take these comments of yours to also mean "go ahead and write unit tests."

--mark

https://codereview.chromium.org/229733004/diff/20001/chrome/browser/autocompl...
File chrome/browser/autocomplete/bookmark_provider.cc (right):

https://codereview.chromium.org/229733004/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/bookmark_provider.cc:169: // Determine
|inline_autocomplete_offset|.
On 2014/04/09 00:53:32, Peter Kasting wrote:
> This whole block looks suspiciously similar to what other providers do (or at
> least, need to do).  Is there any opportunity to hoist similar code to a
common
> location?

I thought about this as I was transferred code here.  The regular providers
don't need to do the fixup case, so their logic is simply one BestURLPrefix call
and the variable declarations around it.  No sharing there.

Shortcuts does the fixup case.  But shortcuts doesn't use BestURLPrefix, it uses
something different BestURLPrefixWithWWWCase because the strings it's trying
match against may be missing the scheme but have a www that we want to drop.  I
guess I could share something with Shortcuts, but that code would be a bit odd
because we'd have to take an extra parameter want_www_case, and condition both
its best-url calls on that.  As those calls are really the only thing happening
here, I don't think it's worthwhile to share.  It'd be ugly.

The trim_http line is in common with history_url provider.  That's one line.  I
can replace it with a call that takes those three variables.  I don't think that
will help readability.  (Those three variables don't naturally go together; I
can't think a nice name.  TrimHttp (input, best_prefix,
inline_autocomplete_offset) doesn't look good to me.)

https://codereview.chromium.org/229733004/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/bookmark_provider.cc:191: ((best_prefix != NULL) ||
(inline_autocomplete_offset != 0));
On 2014/04/09 00:53:32, Peter Kasting wrote:
> I don't think I understand this conditional.
> 
> Wouldn't this trim if the user text was "http" (since we'd get a non-null best
> prefix of "")?  Also, if best_prefix is NULL, isn't inline_autocomplete_offset
> == base::string16::npos, and thus nonzero, so the second line here always
> evaluates to true?

Oops, you're right.  It should be best_prefix == NULL, not best_prefix != NULL.
Done.

[Mark P, 2014-04-09 20:21:41]

https://codereview.chromium.org/229733004/diff/20001/chrome/browser/autocompl...
File chrome/browser/autocomplete/bookmark_provider.cc (right):

https://codereview.chromium.org/229733004/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/bookmark_provider.cc:169: // Determine
|inline_autocomplete_offset|.
On 2014/04/09 02:44:42, Mark P wrote:
> On 2014/04/09 00:53:32, Peter Kasting wrote:
> > This whole block looks suspiciously similar to what other providers do (or
at
> > least, need to do).  Is there any opportunity to hoist similar code to a
> common
> > location?
> 
> I thought about this as I was transferred code here.  The regular providers
> don't need to do the fixup case, so their logic is simply one BestURLPrefix
call
> and the variable declarations around it.  No sharing there.
> 
> Shortcuts does the fixup case.  But shortcuts doesn't use BestURLPrefix, it
uses
> something different BestURLPrefixWithWWWCase because the strings it's trying
> match against may be missing the scheme but have a www that we want to drop. 
I
> guess I could share something with Shortcuts, but that code would be a bit odd
> because we'd have to take an extra parameter want_www_case, and condition both
> its best-url calls on that.  As those calls are really the only thing
happening
> here, I don't think it's worthwhile to share.  It'd be ugly.

I found a bug this code, causing me to change my mind.  I'm going to take your
advice and put it in a common place so I don't have to remember to make the same
fixes in multiple places.

> 
> The trim_http line is in common with history_url provider.  That's one line. 
I
> can replace it with a call that takes those three variables.  I don't think
that
> will help readability.  (Those three variables don't naturally go together; I
> can't think a nice name.  TrimHttp (input, best_prefix,
> inline_autocomplete_offset) doesn't look good to me.)

[Mark P, 2014-04-09 22:03:16]

Large, shiny, new patchset up, with a revised changelist description
that explains everything it does.

I recommended reviewing patchset 4 because it has come
code review comments (shown below) that help explain what I
did.  Patchset 5 only differs from 4 in that I expanded the
test of ShortcutsProvider slightly.

This is ready for a complete review.

--mark

https://codereview.chromium.org/229733004/diff/60001/chrome/browser/autocompl...
File chrome/browser/autocomplete/history_provider.cc (left):

https://codereview.chromium.org/229733004/diff/60001/chrome/browser/autocompl...
chrome/browser/autocomplete/history_provider.cc:21:
HistoryProvider::HistoryProvider(AutocompleteProviderListener* listener,
This moved below for better consistency with declaration order.

https://codereview.chromium.org/229733004/diff/60001/chrome/browser/autocompl...
File chrome/browser/autocomplete/history_provider.cc (right):

https://codereview.chromium.org/229733004/diff/60001/chrome/browser/autocompl...
chrome/browser/autocomplete/history_provider.cc:36: // static
This was moved from below with no changes.

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

https://codereview.chromium.org/229733004/diff/60001/chrome/browser/autocompl...
chrome/browser/autocomplete/shortcuts_provider.cc:141:
FixupUserInput(&fixed_up_input);
I realized we don't need to check the return value; the code handles failures
(which result in fixed_up_input.text() being empty) just fine.

https://codereview.chromium.org/229733004/diff/60001/chrome/browser/autocompl...
File chrome/browser/autocomplete/url_prefix.cc (right):

https://codereview.chromium.org/229733004/diff/60001/chrome/browser/autocompl...
chrome/browser/autocomplete/url_prefix.cc:16: const URLPrefix*
BestURLPrefixWithWWWCase(
This code was moved here from shortcuts_provider.cc with no changes.

https://codereview.chromium.org/229733004/diff/60001/chrome/browser/autocompl...
chrome/browser/autocomplete/url_prefix.cc:80: void
URLPrefix::ComputeMatchStartAndInlineAutocompleteOffset(
This code was moved here from shortcuts provider with a number of revisions
along the way deal with the new parameter types.

[Peter Kasting, 2014-04-10 00:22:34]

LGTM

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

https://codereview.chromium.org/229733004/diff/60001/chrome/browser/autocompl...
chrome/browser/autocomplete/history_provider.h:22: // input.  This method
returns true if |input.prevent_inline_autocomplete()|
Nit: No bars needed

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

https://codereview.chromium.org/229733004/diff/60001/chrome/browser/autocompl...
chrome/browser/autocomplete/shortcuts_provider.cc:249:
match.inline_autocompletion = match.fill_into_edit.substr(
Nit: Wrap after '=' instead

https://codereview.chromium.org/229733004/diff/80001/chrome/browser/autocompl...
File chrome/browser/autocomplete/bookmark_provider.cc (left):

https://codereview.chromium.org/229733004/diff/80001/chrome/browser/autocompl...
chrome/browser/autocomplete/bookmark_provider.cc:48: ((input.matches_requested()
== AutocompleteInput::BEST_MATCH) &&
Man.  Are there legitimate uses of BEST_MATCH left?  It seems like you're
gradually removing all of them.  Maybe we can simplify something.

https://codereview.chromium.org/229733004/diff/80001/chrome/browser/autocompl...
File chrome/browser/autocomplete/bookmark_provider.cc (right):

https://codereview.chromium.org/229733004/diff/80001/chrome/browser/autocompl...
chrome/browser/autocomplete/bookmark_provider.cc:171: false, url_utf16,
&match_start, &inline_autocomplete_offset);
Nit: All lines of args should begin at the same position

[Mark P, 2014-04-10 17:10:01]

Only one interesting comment below.  I'll submit on CQ now.  I will do any
following changes regarding BEST_MATCH / SYNCHRONOUS_MATCHES / ALL_MATCHES in a
follow-up change.

--mark

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

https://codereview.chromium.org/229733004/diff/60001/chrome/browser/autocompl...
chrome/browser/autocomplete/history_provider.h:22: // input.  This method
returns true if |input.prevent_inline_autocomplete()|
On 2014/04/10 00:22:34, Peter Kasting wrote:
> Nit: No bars needed

Done.

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

https://codereview.chromium.org/229733004/diff/60001/chrome/browser/autocompl...
chrome/browser/autocomplete/shortcuts_provider.cc:249:
match.inline_autocompletion = match.fill_into_edit.substr(
On 2014/04/10 00:22:34, Peter Kasting wrote:
> Nit: Wrap after '=' instead

Done.

https://codereview.chromium.org/229733004/diff/80001/chrome/browser/autocompl...
File chrome/browser/autocomplete/bookmark_provider.cc (left):

https://codereview.chromium.org/229733004/diff/80001/chrome/browser/autocompl...
chrome/browser/autocomplete/bookmark_provider.cc:48: ((input.matches_requested()
== AutocompleteInput::BEST_MATCH) &&
On 2014/04/10 00:22:35, Peter Kasting wrote:
> Man.  Are there legitimate uses of BEST_MATCH left?  It seems like you're
> gradually removing all of them.  Maybe we can simplify something.

Interesting question.

The only legitimate uses I see are in the AutocompleteClassifier and here in
keyword provider:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/aut...
, where it's merely for efficiency when dealing with extension providers.

Think we can get rid of BEST_MATCHES (and replace it with SYNCHRONOUS_MATCHES)? 
I think so.  We could even change the MatchesRequested enum and replace it with
a simple bool (want_asychronous_matches).

I think keeping the distinction between SYNCHRONOUS_MATCHES and ALL_MATCHES is
important.  The AutocompleteClassifier gets called all over the place, many
times, and I don't want it to hammer the history system or the suggest server
harder because it keeps sending history DB on-disk requests or suggest server
requests.

https://codereview.chromium.org/229733004/diff/80001/chrome/browser/autocompl...
File chrome/browser/autocomplete/bookmark_provider.cc (right):

https://codereview.chromium.org/229733004/diff/80001/chrome/browser/autocompl...
chrome/browser/autocomplete/bookmark_provider.cc:171: false, url_utf16,
&match_start, &inline_autocomplete_offset);
On 2014/04/10 00:22:35, Peter Kasting wrote:
> Nit: All lines of args should begin at the same position

Done.

[Mark P, 2014-04-10 17:11:34]

The CQ bit was checked by mpearson@chromium.org

[commit-bot: I haz the power, 2014-04-10 17:11:39]

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

[commit-bot: I haz the power, 2014-04-10 21:08:10]

Change committed as 263077

[Peter Kasting, 2014-04-14 22:56:12]

https://codereview.chromium.org/229733004/diff/80001/chrome/browser/autocompl...
File chrome/browser/autocomplete/bookmark_provider.cc (left):

https://codereview.chromium.org/229733004/diff/80001/chrome/browser/autocompl...
chrome/browser/autocomplete/bookmark_provider.cc:48: ((input.matches_requested()
== AutocompleteInput::BEST_MATCH) &&
On 2014/04/10 17:10:02, Mark P wrote:
> On 2014/04/10 00:22:35, Peter Kasting wrote:
> > Man.  Are there legitimate uses of BEST_MATCH left?  It seems like you're
> > gradually removing all of them.  Maybe we can simplify something.
> 
> Interesting question.
> 
> The only legitimate uses I see are in the AutocompleteClassifier and here in
> keyword provider:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/aut...
> , where it's merely for efficiency when dealing with extension providers.
> 
> Think we can get rid of BEST_MATCHES (and replace it with
SYNCHRONOUS_MATCHES)? 
> I think so.  We could even change the MatchesRequested enum and replace it
with
> a simple bool (want_asychronous_matches).
> 
> I think keeping the distinction between SYNCHRONOUS_MATCHES and ALL_MATCHES is
> important.  The AutocompleteClassifier gets called all over the place, many
> times, and I don't want it to hammer the history system or the suggest server
> harder because it keeps sending history DB on-disk requests or suggest server
> requests.

I believe if we tried to remove this distinction entirely, we'd get infinite
recursion -- I think at least one of the autocomplete providers uses the
AutocompleteClassifier, and that call stack would blow up.  So yeah, I'm sure we
still have to keep some distinction around, but maybe we could simplify it a
bit.

[Mark P, 2014-04-14 23:28:14]

https://codereview.chromium.org/229733004/diff/80001/chrome/browser/autocompl...
File chrome/browser/autocomplete/bookmark_provider.cc (left):

https://codereview.chromium.org/229733004/diff/80001/chrome/browser/autocompl...
chrome/browser/autocomplete/bookmark_provider.cc:48: ((input.matches_requested()
== AutocompleteInput::BEST_MATCH) &&
On 2014/04/14 22:56:13, Peter Kasting wrote:
> On 2014/04/10 17:10:02, Mark P wrote:
> > On 2014/04/10 00:22:35, Peter Kasting wrote:
> > > Man.  Are there legitimate uses of BEST_MATCH left?  It seems like you're
> > > gradually removing all of them.  Maybe we can simplify something.
> > 
> > Interesting question.
> > 
> > The only legitimate uses I see are in the AutocompleteClassifier and here in
> > keyword provider:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/aut...
> > , where it's merely for efficiency when dealing with extension providers.
> > 
> > Think we can get rid of BEST_MATCHES (and replace it with
> SYNCHRONOUS_MATCHES)? 
> > I think so.  We could even change the MatchesRequested enum and replace it
> with
> > a simple bool (want_asychronous_matches).
> > 
> > I think keeping the distinction between SYNCHRONOUS_MATCHES and ALL_MATCHES
is
> > important.  The AutocompleteClassifier gets called all over the place, many
> > times, and I don't want it to hammer the history system or the suggest
server
> > harder because it keeps sending history DB on-disk requests or suggest
server
> > requests.
> 
> I believe if we tried to remove this distinction entirely, we'd get infinite
> recursion -- I think at least one of the autocomplete providers uses the
> AutocompleteClassifier, and that call stack would blow up.  So yeah, I'm sure
we
> still have to keep some distinction around, but maybe we could simplify it a
> bit.

I gave you the two possible ways to simplify it.  What do you think of them?  Or
we can leave it as-is.

[Peter Kasting, 2014-04-14 23:33:52]

On 2014/04/14 23:28:14, Mark P wrote:
> I gave you the two possible ways to simplify it.  What do you think of them? 
Or
> we can leave it as-is.

Sorry, I assumed you'd read my message as "yeah, do one of those".  I think
simplifying to just "bool want_asynchronous_matches" is fine.