Omnibox - Keyword Provider Shouldn't Bold What-You-Typed Queries

Omnibox - Keyword Provider Shouldn't Bold What-You-Typed Queries

Currently we don't bold what-you-typed queries from the default search
provider, not do we bold what-you-typed queries when the keyword is
typed exactly.  We do bold what-you-typed queries for inexact keywords.

The change makes it so we don't.

It also retires a string that is no longer needed after this change.

BUG=593245
Committed: https://crrev.com/2b86465902c5d8e89d75cd6b8c6c2ba21f7b9b2d
Cr-Commit-Position: refs/heads/master@{#385323}

[Mark P, 2016-03-11 18:34:22]

Description was changed from

==========
Omnibox - Keyword Provider Shouldn't Bold What-You-Typed Queries

Currently we don't bold what-you-typed queries from the default search
provider, not do we bold what-you-typed queries when the keyword is
typed exactly.  We do bold what-you-typed queries for inexact keywords.

The change makes it so we don't.

It also retires a string that is no longer needed after this change.

BUG=593245
==========

to

==========
Omnibox - Keyword Provider Shouldn't Bold What-You-Typed Queries

Currently we don't bold what-you-typed queries from the default search
provider, not do we bold what-you-typed queries when the keyword is
typed exactly.  We do bold what-you-typed queries for inexact keywords.

The change makes it so we don't.

It also retires a string that is no longer needed after this change.

BUG=593245
==========

[Mark P, 2016-03-11 18:34:22]

mpearson@chromium.org changed reviewers:
+ pkasting@chromium.org

[Peter Kasting, 2016-03-12 01:34:49]

Should there be a test for this?

LGTM

https://codereview.chromium.org/1784233003/diff/1/components/omnibox/browser/...
File components/omnibox/browser/keyword_provider.cc (left):

https://codereview.chromium.org/1784233003/diff/1/components/omnibox/browser/...
components/omnibox/browser/keyword_provider.cc:453:
IDS_EXTENSION_KEYWORD_COMMAND : IDS_KEYWORD_SEARCH;
Hmm.  Is removing the "run command blah" string for extensions really what we
want? What UI does this give us instead, this?:

  command_name - extension_name

https://codereview.chromium.org/1784233003/diff/1/components/omnibox/browser/...
File components/omnibox/browser/keyword_provider.cc (right):

https://codereview.chromium.org/1784233003/diff/1/components/omnibox/browser/...
components/omnibox/browser/keyword_provider.cc:461:
l10n_util::GetStringFUTF16(IDS_KEYWORD_SEARCH,
Nit: While here: Put IDS_KEYWORD_SEARCH on next line so all lines of args start
at same position

[Mark P, 2016-03-16 22:59:28]

Silly me, I forgot to update the tests.  Now done.

--mark

https://codereview.chromium.org/1784233003/diff/1/components/omnibox/browser/...
File components/omnibox/browser/keyword_provider.cc (left):

https://codereview.chromium.org/1784233003/diff/1/components/omnibox/browser/...
components/omnibox/browser/keyword_provider.cc:453:
IDS_EXTENSION_KEYWORD_COMMAND : IDS_KEYWORD_SEARCH;
On 2016/03/12 01:34:49, Peter Kasting wrote:
> Hmm.  Is removing the "run command blah" string for extensions really what we
> want? What UI does this give us instead, this?:
> 
>   command_name - extension_name

For fully-typed extensions (i.e., user entered extension mode), the entire
omnibox suggestion line is under control of the extension.

For partially-typed extensions, yes, you get command - extension_name.

https://codereview.chromium.org/1784233003/diff/1/components/omnibox/browser/...
File components/omnibox/browser/keyword_provider.cc (right):

https://codereview.chromium.org/1784233003/diff/1/components/omnibox/browser/...
components/omnibox/browser/keyword_provider.cc:461:
l10n_util::GetStringFUTF16(IDS_KEYWORD_SEARCH,
On 2016/03/12 01:34:49, Peter Kasting wrote:
> Nit: While here: Put IDS_KEYWORD_SEARCH on next line so all lines of args
start
> at same position

Done.

[Peter Kasting, 2016-03-17 06:08:11]

LGTM

https://codereview.chromium.org/1784233003/diff/20001/components/omnibox/brow...
File components/omnibox/browser/keyword_provider.cc (right):

https://codereview.chromium.org/1784233003/diff/20001/components/omnibox/brow...
components/omnibox/browser/keyword_provider.cc:464:
l10n_util::GetStringUTF16(IDS_EMPTY_KEYWORD_VALUE)));
Should we change this to just "<enter query> - Amazon search", which would be
parallel to both the "?" placeholder text and the with-query-string text?

If so, we can nuke IDS_KEYWORD_SEARCH entirely,

We can also remove the commentary about "Search ... for" that you added to the
tests, since that string won't appear anywhere in the codebase and thus readers
would have no reason to expect us to use it that we'd need to comment about.

[Mark P, 2016-04-01 21:48:39]

Please take another look, thanks.

--mark

https://codereview.chromium.org/1784233003/diff/20001/components/omnibox/brow...
File components/omnibox/browser/keyword_provider.cc (right):

https://codereview.chromium.org/1784233003/diff/20001/components/omnibox/brow...
components/omnibox/browser/keyword_provider.cc:464:
l10n_util::GetStringUTF16(IDS_EMPTY_KEYWORD_VALUE)));
On 2016/03/17 06:08:11, Peter Kasting wrote:
> Should we change this to just "<enter query> - Amazon search", which would be
> parallel to both the "?" placeholder text and the with-query-string text?

Changed text per the bug. 

> If so, we can nuke IDS_KEYWORD_SEARCH entirely,

And then nuked IDS_KEYWORD_SEARCH, yay.

> We can also remove the commentary about "Search ... for" that you added to the
> tests, since that string won't appear anywhere in the codebase and thus
readers
> would have no reason to expect us to use it that we'd need to comment about.

Revised code and comments in unit test.

[Peter Kasting, 2016-04-02 02:32:29]

LGTM

https://codereview.chromium.org/1784233003/diff/40001/components/omnibox_stri...
File components/omnibox_strings.grdp (right):

https://codereview.chromium.org/1784233003/diff/40001/components/omnibox_stri...
components/omnibox_strings.grdp:8: Type search term
Nit: Do we want "<Type search term>" instead of "Type search term"?  To me that
makes it a little more obvious at a glance that you won't be searching for the
phrase "Type search term".

[Mark P, 2016-04-05 21:38:19]

https://codereview.chromium.org/1784233003/diff/40001/components/omnibox_stri...
File components/omnibox_strings.grdp (right):

https://codereview.chromium.org/1784233003/diff/40001/components/omnibox_stri...
components/omnibox_strings.grdp:8: Type search term
On 2016/04/02 02:32:29, Peter Kasting wrote:
> Nit: Do we want "<Type search term>" instead of "Type search term"?  To me
that
> makes it a little more obvious at a glance that you won't be searching for the
> phrase "Type search term".

Given when this is displayed, I don't think there will be any confusion.  That
said, I'm not opposed to putting the <>, so I went ahead and did it.

[Mark P, 2016-04-05 21:40:17]

mpearson@chromium.org changed reviewers:
+ sdefresne@chromium.org

[Mark P, 2016-04-05 21:40:42]

sdefresne, can you please approve the grdp change?

thanks,
mark

[sdefresne, 2016-04-05 21:54:54]

lgtm

[Mark P, 2016-04-05 22:03:39]

The CQ bit was checked by mpearson@chromium.org

[Mark P, 2016-04-05 22:03:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1784233003/#ps60001
(title: "put <> around text")

[commit-bot: I haz the power, 2016-04-05 22:04:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1784233003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1784233003/60001

[commit-bot: I haz the power, 2016-04-05 23:32:26]

Description was changed from

==========
Omnibox - Keyword Provider Shouldn't Bold What-You-Typed Queries

Currently we don't bold what-you-typed queries from the default search
provider, not do we bold what-you-typed queries when the keyword is
typed exactly.  We do bold what-you-typed queries for inexact keywords.

The change makes it so we don't.

It also retires a string that is no longer needed after this change.

BUG=593245
==========

to

==========
Omnibox - Keyword Provider Shouldn't Bold What-You-Typed Queries

Currently we don't bold what-you-typed queries from the default search
provider, not do we bold what-you-typed queries when the keyword is
typed exactly.  We do bold what-you-typed queries for inexact keywords.

The change makes it so we don't.

It also retires a string that is no longer needed after this change.

BUG=593245
==========

[commit-bot: I haz the power, 2016-04-05 23:32:27]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-05 23:34:06]

Description was changed from

==========
Omnibox - Keyword Provider Shouldn't Bold What-You-Typed Queries

Currently we don't bold what-you-typed queries from the default search
provider, not do we bold what-you-typed queries when the keyword is
typed exactly.  We do bold what-you-typed queries for inexact keywords.

The change makes it so we don't.

It also retires a string that is no longer needed after this change.

BUG=593245
==========

to

==========
Omnibox - Keyword Provider Shouldn't Bold What-You-Typed Queries

Currently we don't bold what-you-typed queries from the default search
provider, not do we bold what-you-typed queries when the keyword is
typed exactly.  We do bold what-you-typed queries for inexact keywords.

The change makes it so we don't.

It also retires a string that is no longer needed after this change.

BUG=593245

Committed: https://crrev.com/2b86465902c5d8e89d75cd6b8c6c2ba21f7b9b2d
Cr-Commit-Position: refs/heads/master@{#385323}
==========

[commit-bot: I haz the power, 2016-04-05 23:34:07]

Patchset 4 (id:??) landed as
https://crrev.com/2b86465902c5d8e89d75cd6b8c6c2ba21f7b9b2d
Cr-Commit-Position: refs/heads/master@{#385323}