Omnibox: Make Exact Match Tab-to-Search Work

Omnibox: Make Exact Match Tab-to-Search Work

Tested interactively and adds unit test.  Seems to behave as expected.

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

[Mark P, 2014-08-04 21:21:10]

Peter,

please take a look.

thanks,
mark

[Peter Kasting, 2014-08-04 21:54:23]

LGTM

https://codereview.chromium.org/440753003/diff/1/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/autocomplete_controller.cc (left):

https://codereview.chromium.org/440753003/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/autocomplete_controller.cc:525: if
(!match->associated_keyword.get())
What does removing this conditional do?

https://codereview.chromium.org/440753003/diff/1/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/autocomplete_controller.cc (right):

https://codereview.chromium.org/440753003/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/autocomplete_controller.cc:524: // work for this
exact keyword.
Nit: This comment is a bit confusing.  How about this:

When the user has typed an exact keyword, we want tab-to-search on the default
match to select that keyword, even if the match inline-autocompletes to a
different keyword.  (This prevents inline autocompletions from blocking a user's
attempts to use an explicitly-set keyword of their own creation.)  So use
|exact_keyword| if it's available.

[Mark P, 2014-08-04 22:05:38]

Please take another look, thanks.

https://codereview.chromium.org/440753003/diff/1/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/autocomplete_controller.cc (left):

https://codereview.chromium.org/440753003/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/autocomplete_controller.cc:525: if
(!match->associated_keyword.get())
On 2014/08/04 21:54:22, Peter Kasting wrote:
> What does removing this conditional do?

I was worried that the copy-old-match code will keep an old, inappropriate
associated_keyword.

For instance, suppose we have keywords "f" and "foo.com"
Suppose the user types "f" and get a match "foo.com".  Though naturally
associated with the latter keyword, we decide to associate it with the former
under the exact-match-trumps policy.
Now the user type o, yielding "fo".  This match may be preserved due to the
copy-old-matches logic.  I need to be sure to clobber the associated_keyword so
it no longer points to "f".  That's why I don't keep the value from before.

Actually, this explanation made me realize I'm doing this sligtly wrong. 
Revised code slightly.

BTW, I looked at the GetKeywordForText, new AutocompleteMatch, and
CreateVerbatimMatch functions.  They don't seem heavy-weight; I feel no qualms
about making them run a bit more often.

https://codereview.chromium.org/440753003/diff/1/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/autocomplete_controller.cc (right):

https://codereview.chromium.org/440753003/diff/1/chrome/browser/autocomplete/...
chrome/browser/autocomplete/autocomplete_controller.cc:524: // work for this
exact keyword.
On 2014/08/04 21:54:22, Peter Kasting wrote:
> Nit: This comment is a bit confusing.  How about this:
> 
> When the user has typed an exact keyword, we want tab-to-search on the default
> match to select that keyword, even if the match inline-autocompletes to a
> different keyword.  (This prevents inline autocompletions from blocking a
user's
> attempts to use an explicitly-set keyword of their own creation.)  So use
> |exact_keyword| if it's available.

Done.

[Peter Kasting, 2014-08-04 22:53:11]

Is it possible to test that scenario you're worried about with preserving the
keywords of old matches incorrectly?

LGTM otherwise.

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

https://codereview.chromium.org/440753003/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_controller.cc:535: // Otherwise use the
particular match's associated keyword.
Nit: This comment is now somewhat misleading as you don't actually pay attention
to any associated keyword.  How about:

Otherwise, set a match's associated keyword based on the match's fill_into_edit,
which should take inline autocompletions into account.

[Mark P, 2014-08-04 22:57:03]

On Mon, Aug 4, 2014 at 3:53 PM, <pkasting@chromium.org> wrote:

> Is it possible to test that scenario you're worried about with preserving
> the
> keywords of old matches incorrectly?
>

Not easily.

Side comment: I think that code should go.  There's only two asynchronous
providers nowadays (Search and HUP).  The former does its own caching.  The
latter has many fewer (no?) inconsistency issues as before.

--mark


>
> LGTM otherwise.
>
>
> https://codereview.chromium.org/440753003/diff/20001/
> chrome/browser/autocomplete/autocomplete_controller.cc
> File chrome/browser/autocomplete/autocomplete_controller.cc (right):
>
> https://codereview.chromium.org/440753003/diff/20001/
> chrome/browser/autocomplete/autocomplete_controller.cc#newcode535
> chrome/browser/autocomplete/autocomplete_controller.cc:535: // Otherwise
> use the particular match's associated keyword.
> Nit: This comment is now somewhat misleading as you don't actually pay
> attention to any associated keyword.  How about:
>
> Otherwise, set a match's associated keyword based on the match's
> fill_into_edit, which should take inline autocompletions into account.
>
> https://codereview.chromium.org/440753003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mark P, 2014-08-04 22:58:24]

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

https://codereview.chromium.org/440753003/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_controller.cc:535: // Otherwise use the
particular match's associated keyword.
On 2014/08/04 22:53:11, Peter Kasting wrote:
> Nit: This comment is now somewhat misleading as you don't actually pay
attention
> to any associated keyword.  How about:
> 
> Otherwise, set a match's associated keyword based on the match's
fill_into_edit,
> which should take inline autocompletions into account.

Done.  I see why it's better.  Thank you for rewriting comments for me. :-)

[Mark P, 2014-08-04 22:58:28]

The CQ bit was checked by mpearson@chromium.org

[commit-bot: I haz the power, 2014-08-04 23:00:47]

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

[Peter Kasting, 2014-08-04 23:01:12]

On 2014/08/04 22:57:03, Mark P wrote:
> Side comment: I think that code should go.  There's only two asynchronous
> providers nowadays (Search and HUP).  The former does its own caching.  The
> latter has many fewer (no?) inconsistency issues as before.

We always did the caching thing primarily for the search provider (not for other
providers, so the absence of other async providers doesn't affect the issue),
and it was aimed at different problems than the search provider's own caching. 
This is a long-winded way of saying that I don't actually think this caching
code is less-relevant than it used to be or could be removed without harming the
user experience.

[commit-bot: I haz the power, 2014-08-05 03:06:15]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...)
  mac_chromium_rel on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-05 03:15:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-05 03:15:20]

Try jobs failed on following builders:
  android_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...)

[Mark P, 2014-08-05 20:28:19]

Peter,

I had to fix a browser test and a minor android compilation error.  You may want
to review what's changed since your last review (which was patchset 3).  If I
don't hear anything from you, I'll submit this tomorrow.

--mark

[Peter Kasting, 2014-08-05 21:03:47]

LGTM

https://codereview.chromium.org/440753003/diff/80001/chrome/browser/ui/omnibo...
File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right):

https://codereview.chromium.org/440753003/diff/80001/chrome/browser/ui/omnibo...
chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:61: const wchar_t
kSearchKeywordCompletionKeys[] = { ui::VKEY_O, 0 };
Nit: Since these are only used by SendKeySequence(), and that's defined within
this file, can we fix up the function prototype while we're here to take a
ui::KeyboardCode* instead of a wchar_t*?  It looks really confusing to have
these arrays with VKEY members but the type of the array is "wchar_t[]".

[Mark P, 2014-08-05 22:20:06]

https://codereview.chromium.org/440753003/diff/80001/chrome/browser/ui/omnibo...
File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right):

https://codereview.chromium.org/440753003/diff/80001/chrome/browser/ui/omnibo...
chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:61: const wchar_t
kSearchKeywordCompletionKeys[] = { ui::VKEY_O, 0 };
On 2014/08/05 21:03:47, Peter Kasting wrote:
> Nit: Since these are only used by SendKeySequence(), and that's defined within
> this file, can we fix up the function prototype while we're here to take a
> ui::KeyboardCode* instead of a wchar_t*?  It looks really confusing to have
> these arrays with VKEY members but the type of the array is "wchar_t[]".

Done.

[Mark P, 2014-08-05 22:20:09]

The CQ bit was checked by mpearson@chromium.org

[commit-bot: I haz the power, 2014-08-05 22:21:58]

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

[Peter Kasting, 2014-08-05 23:13:36]

My goal here is to give you the most nit cycles possible for such a small
change.

https://codereview.chromium.org/440753003/diff/100001/chrome/browser/ui/omnib...
File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right):

https://codereview.chromium.org/440753003/diff/100001/chrome/browser/ui/omnib...
chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:181: for (; *keys; ++keys)
Tiny nit: For maximum clarity, we should probably either change "*keys" here to
"*keys != VKEY_UNKNOWN", or else continue to use "0" in the array constants.

[Mark P, 2014-08-05 23:17:12]

The CQ bit was unchecked by mpearson@chromium.org

[Mark P, 2014-08-05 23:19:38]

https://codereview.chromium.org/440753003/diff/100001/chrome/browser/ui/omnib...
File chrome/browser/ui/omnibox/omnibox_view_browsertest.cc (right):

https://codereview.chromium.org/440753003/diff/100001/chrome/browser/ui/omnib...
chrome/browser/ui/omnibox/omnibox_view_browsertest.cc:181: for (; *keys; ++keys)
On 2014/08/05 23:13:36, Peter Kasting wrote:
> Tiny nit: For maximum clarity, we should probably either change "*keys" here
to
> "*keys != VKEY_UNKNOWN", or else continue to use "0" in the array constants.

I can't use 0 because it's the wrong type.  (I'd need an explicit cast.)

Switched the loop termination condition.

[Mark P, 2014-08-05 23:19:41]

The CQ bit was checked by mpearson@chromium.org

[commit-bot: I haz the power, 2014-08-05 23:22:22]

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

[commit-bot: I haz the power, 2014-08-06 02:54:44]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2014-08-06 04:50:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-06 04:50:13]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mark P, 2014-08-06 17:35:26]

The CQ bit was checked by mpearson@chromium.org

[commit-bot: I haz the power, 2014-08-06 17:35:54]

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

[commit-bot: I haz the power, 2014-08-06 18:10:57]

Change committed as 287807