Invert description and URL for experimental zero suggest suggestions.

Invert description and URL for experimental zero suggest suggestions.

BUG=682420, 693649
Review-Url: https://codereview.chromium.org/2716273004
Cr-Commit-Position: refs/heads/master@{#454084}
Committed: https://chromium.googlesource.com/chromium/src/+/ba64a6af8eb5ce7ff17219925fa9c0de5a324554

[gcomanici, 2017-02-27 17:22:02]

Description was changed from

==========
Invert description and URL for experimental zero suggest suggestions.

BUG=682420, 693649
==========

to

==========
Invert description and URL for experimental zero suggest suggestions.

BUG=682420, 693649
==========

[gcomanici, 2017-02-27 17:22:02]

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

[Mark P, 2017-02-27 18:11:36]

Please make this independently configurable and not tied in with the redirect
field trial.

thanks,
mark

[gcomanici, 2017-02-27 19:41:05]

Done. PTAL, Mark. Thanks!

[Mark P, 2017-02-28 22:24:28]

Sorry for the delay.

Have you tested this?  I'm curious how it feels.

cheers,
mark

https://codereview.chromium.org/2716273004/diff/20001/components/omnibox/brow...
File components/omnibox/browser/features.h (right):

https://codereview.chromium.org/2716273004/diff/20001/components/omnibox/brow...
components/omnibox/browser/features.h:12: // Feature used for the Zero Suggest
Redirect to Chrome Field Trial.
optional nit: I'm not sure if either of these comments add anything useful. 
Remove?

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

https://codereview.chromium.org/2716273004/diff/20001/components/omnibox/brow...
components/omnibox/browser/omnibox_field_trial.cc:527: // Look up the parameter
named kEmphasizeTitlesRule + ":" + input_type,
nit: This comment no longer applies to the code.  Move it to the code it's near
and add a new comment.

https://codereview.chromium.org/2716273004/diff/20001/components/omnibox/brow...
components/omnibox/browser/omnibox_field_trial.cc:530:
metrics::OmniboxInputType::Type input_type = input.type();
nit: it's alias isn't significantly shorter or more clear than what it's
aliasing.  Please remove.

https://codereview.chromium.org/2716273004/diff/20001/components/omnibox/brow...
components/omnibox/browser/omnibox_field_trial.cc:532:
OmniboxFieldTrial::InZeroSuggestSwapTitleAndUrlFieldTrial()) {
nit: just test directly to see if the feature is enabled.

(The function is necessary.)

https://codereview.chromium.org/2716273004/diff/20001/components/omnibox/brow...
File components/omnibox/browser/omnibox_field_trial.h (right):

https://codereview.chromium.org/2716273004/diff/20001/components/omnibox/brow...
components/omnibox/browser/omnibox_field_trial.h:377: // of a URL more
prominently than the URL for an input of type |input_type|.
Revise this.

[gcomanici, 2017-03-01 02:56:28]

Thanks, Mark! Here's a snapshot of zero suggest suggestions under this patch:

https://drive.google.com/a/google.com/file/d/0BxodERAvSxROcmw1cWtUbHdsZWc/vie...

I tested it and what it does is it displays the title followed by the URL in
zero-suggest, but switches back to the normal behavior as soon as the I start
typing.

https://codereview.chromium.org/2716273004/diff/20001/components/omnibox/brow...
File components/omnibox/browser/features.h (right):

https://codereview.chromium.org/2716273004/diff/20001/components/omnibox/brow...
components/omnibox/browser/features.h:12: // Feature used for the Zero Suggest
Redirect to Chrome Field Trial.
On 2017/02/28 22:24:28, Mark P wrote:
> optional nit: I'm not sure if either of these comments add anything useful. 
> Remove?

Done.

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

https://codereview.chromium.org/2716273004/diff/20001/components/omnibox/brow...
components/omnibox/browser/omnibox_field_trial.cc:527: // Look up the parameter
named kEmphasizeTitlesRule + ":" + input_type,
On 2017/02/28 22:24:28, Mark P wrote:
> nit: This comment no longer applies to the code.  Move it to the code it's
near
> and add a new comment.

I added a comment for the condition I added, and kept the comment for the rest.

https://codereview.chromium.org/2716273004/diff/20001/components/omnibox/brow...
components/omnibox/browser/omnibox_field_trial.cc:530:
metrics::OmniboxInputType::Type input_type = input.type();
On 2017/02/28 22:24:28, Mark P wrote:
> nit: it's alias isn't significantly shorter or more clear than what it's
> aliasing.  Please remove.

Done.

https://codereview.chromium.org/2716273004/diff/20001/components/omnibox/brow...
components/omnibox/browser/omnibox_field_trial.cc:532:
OmniboxFieldTrial::InZeroSuggestSwapTitleAndUrlFieldTrial()) {
On 2017/02/28 22:24:28, Mark P wrote:
> nit: just test directly to see if the feature is enabled.
> 
> (The function is necessary.)

Done.

https://codereview.chromium.org/2716273004/diff/20001/components/omnibox/brow...
File components/omnibox/browser/omnibox_field_trial.h (right):

https://codereview.chromium.org/2716273004/diff/20001/components/omnibox/brow...
components/omnibox/browser/omnibox_field_trial.h:377: // of a URL more
prominently than the URL for an input of type |input_type|.
On 2017/02/28 22:24:28, Mark P wrote:
> Revise this.

Done.

[Mark P, 2017-03-01 20:09:26]

lgtm

On 2017/03/01 02:56:28, gcomanici wrote:
> Thanks, Mark! Here's a snapshot of zero suggest suggestions under this patch:
> 
>
https://drive.google.com/a/google.com/file/d/0BxodERAvSxROcmw1cWtUbHdsZWc/vie...
> 
> I tested it and what it does is it displays the title followed by the URL in
> zero-suggest, but switches back to the normal behavior as soon as the I start
> typing.

For what it's worth, it looks a little odd to have the current page's URL on the
left here next to the other pages' titles.

Perhaps we should display the current page's URL with the title on zero suggest,
then thus allow swapping.

If you don't mind, can you post the screenshot on the relevant bug, and also
mention my idea (and perhaps what you think of it)?

thanks,
mark

https://codereview.chromium.org/2716273004/diff/40001/components/omnibox/brow...
File components/omnibox/browser/omnibox_field_trial.h (right):

https://codereview.chromium.org/2716273004/diff/40001/components/omnibox/brow...
components/omnibox/browser/omnibox_field_trial.h:425: static bool
InZeroSuggestSwapTitleAndUrlFieldTrial();
Please remove this unused function and its implementation.

[gcomanici, 2017-03-01 20:47:12]

The CQ bit was checked by gcomanici@chromium.org

[gcomanici, 2017-03-01 20:47:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
mpearson@chromium.org
Link to the patchset: https://codereview.chromium.org/2716273004/#ps60001
(title: "Sync.")

[commit-bot: I haz the power, 2017-03-01 20:47:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mark P, 2017-03-01 21:14:15]

The CQ bit was unchecked by mpearson@chromium.org

[Mark P, 2017-03-01 21:15:01]

Please resolve my comment on patchset #3 before submitting.

thanks,
mark

[gcomanici, 2017-03-01 22:02:15]

On 2017/03/01 21:15:01, Mark P wrote:
> Please resolve my comment on patchset #3 before submitting.
> 
> thanks,
> mark

Done. Sorry for missing that. I am still in the process of getting used with
codereview.chromium.org.

[gcomanici, 2017-03-01 22:07:51]

The CQ bit was checked by gcomanici@chromium.org

[gcomanici, 2017-03-01 22:07:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
mpearson@chromium.org
Link to the patchset: https://codereview.chromium.org/2716273004/#ps80001
(title: "Remove unnecessary function.")

[commit-bot: I haz the power, 2017-03-01 22:08:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-01 23:06:10]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488406071156050,
"parent_rev": "ac3839d51a3735566cab023413f2978e09c77edd", "commit_rev":
"ba64a6af8eb5ce7ff17219925fa9c0de5a324554"}

[commit-bot: I haz the power, 2017-03-01 23:06:51]

Description was changed from

==========
Invert description and URL for experimental zero suggest suggestions.

BUG=682420, 693649
==========

to

==========
Invert description and URL for experimental zero suggest suggestions.

BUG=682420, 693649

Review-Url: https://codereview.chromium.org/2716273004
Cr-Commit-Position: refs/heads/master@{#454084}
Committed:
https://chromium.googlesource.com/chromium/src/+/ba64a6af8eb5ce7ff17219925fa9...
==========

[commit-bot: I haz the power, 2017-03-01 23:06:52]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/ba64a6af8eb5ce7ff17219925fa9...