Omnibox UI Experiments: Add Vertical Layout experiment (title-on-top).

Omnibox UI Experiments: Add Vertical Layout experiment (title-on-top).

This is a minimal Views-only patch that renders the title above the
origin. It doesn't change any styling yet, nor does it touch Cocoa.

Those will be in followup CLs.

BUG=725599
Review-Url: https://codereview.chromium.org/2901953002
Cr-Commit-Position: refs/heads/master@{#475119}
Committed: https://chromium.googlesource.com/chromium/src/+/26317da136aba1d8f3c46cd8347d396894416c7d

[tommycli, 2017-05-23 19:03:51]

tommycli@chromium.org changed reviewers:
+ jdonnelly@chromium.org

[tommycli, 2017-05-23 19:03:52]

jdonnelly: Sending this CL out to for informational purposes. Screenshots and
discussion in bug. Looks a little funny to me since some results don't have a
Title.

[tommycli, 2017-05-25 18:18:05]

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

[tommycli, 2017-05-25 18:18:06]

pkasting: PTAL. See bug for discussion and screenshots. Thanks!

[Peter Kasting, 2017-05-25 20:24:39]

LGTM

https://codereview.chromium.org/2901953002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right):

https://codereview.chromium.org/2901953002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:392: contents_max_width);
Nit: I'd probably rework this to avoid the duplicated call:

  if (description_max_width == 0)
    y += ...;
  DrawRenderText(..., CONTENTS, ...);
  if (description_max_width != 0) {
    ...

This in turn makes this much more parallel to the non-vertical-layout path, so
that you could probably combine with that.

However, see also comments on bug regarding this behavior.

https://codereview.chromium.org/2901953002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:621: bool swap_match_text
=
Don't we have an experiment somewhere that sometimes displays titles before URLs
for certain things (like zerosuggestions)?

I would look for that code and try to combine this with it.  I think that maybe
happens in AutocompleteResult?  It seems like that's more where you want to do
something like this, and the OmniboxResultView should be a "dumb client".  Even
if you left stuff here, I'm not real keen on sticking the contents in
|description_rendertext_| and the description in |contents_rendertext_| -- those
are just misleading.

Maybe the members here should be |first_rendertext_| and |second_rendertext_| or
something, so we isolate "physical" descriptions like first/second from semantic
ones like content/description. 

(We might want to consider renaming the autocomplete match fields to not be
"contents" and "description" but rather things that actually reflect what they
are, like URLs and titles and search queries and such?  Not only would this feel
less random, it would be more robust -- the current code assumes that non-search
matches always put URLs in the contents and titles in the description, but maybe
that wouldn't always be true.)

https://codereview.chromium.org/2901953002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:647: if
(base::FeatureList::IsEnabled(omnibox::kUIExperimentVerticalLayout)) {
Nit: {} not necessary

Another way to write all this might be:

  int row_height = GetTextHeight();
  if (vertical layout)
    row_height += kVerticalPadding + GetTextHeight();
  const int icon_y = GetVerticalMargin() + (row_height - icon.height()) / 2;

This is shorter and IMO a little less error-prone.

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

https://codereview.chromium.org/2901953002/diff/20001/components/omnibox/brow...
components/omnibox/browser/omnibox_popup_model.cc:60: if (!description_width ||
description_on_separate_line ||
I feel like the caller should have passed true for
|description_on_separate_line|, rather than checking it internally here.

This would also make for better testability, since you can specify the behavior
you want directly in the test.

https://codereview.chromium.org/2901953002/diff/20001/components/omnibox/brow...
components/omnibox/browser/omnibox_popup_model.cc:61:
base::FeatureList::IsEnabled(omnibox::kUIExperimentVerticalLayout)) {
Nit: Omnibox code doesn't usually use braces for cases like this

[tommycli, 2017-05-25 21:59:10]

pkasting: thanks!

https://codereview.chromium.org/2901953002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right):

https://codereview.chromium.org/2901953002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:392: contents_max_width);
On 2017/05/25 20:24:39, Peter Kasting wrote:
> Nit: I'd probably rework this to avoid the duplicated call:
> 
>   if (description_max_width == 0)
>     y += ...;
>   DrawRenderText(..., CONTENTS, ...);
>   if (description_max_width != 0) {
>     ...
> 
> This in turn makes this much more parallel to the non-vertical-layout path, so
> that you could probably combine with that.
> 
> However, see also comments on bug regarding this behavior.

Done.

I was not able to combine it with the old behavior as the logic is fairly
different.

https://codereview.chromium.org/2901953002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:621: bool swap_match_text
=
On 2017/05/25 20:24:39, Peter Kasting wrote:
> Don't we have an experiment somewhere that sometimes displays titles before
URLs
> for certain things (like zerosuggestions)?
> 
> I would look for that code and try to combine this with it.  I think that
maybe
> happens in AutocompleteResult?  It seems like that's more where you want to do
> something like this, and the OmniboxResultView should be a "dumb client". 
Even
> if you left stuff here, I'm not real keen on sticking the contents in
> |description_rendertext_| and the description in |contents_rendertext_| --
those
> are just misleading.
> 
> Maybe the members here should be |first_rendertext_| and |second_rendertext_|
or
> something, so we isolate "physical" descriptions like first/second from
semantic
> ones like content/description. 
> 
> (We might want to consider renaming the autocomplete match fields to not be
> "contents" and "description" but rather things that actually reflect what they
> are, like URLs and titles and search queries and such?  Not only would this
feel
> less random, it would be more robust -- the current code assumes that
non-search
> matches always put URLs in the contents and titles in the description, but
maybe
> that wouldn't always be true.)

Hey Peter -- Looking at what ZeroSuggest does, they display the title in place
of the URL:
https://cs.chromium.org/chromium/src/components/omnibox/browser/zero_suggest_...

It wouldn't be quite appropriate here...

In fact this swapping logic is cribbed from the iOS implementation:
https://cs.chromium.org/chromium/src/ios/chrome/browser/ui/omnibox/omnibox_po...

In the future, if we are going to always have vertical matches, we will just
swap the title and description in the actual provider, so we don't have to do it
here.

But for now, so long as we support both vertical and side-by-side, I think we're
going to have to do the swapping at the UI level.

Thanks!

Tommy

https://codereview.chromium.org/2901953002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:647: if
(base::FeatureList::IsEnabled(omnibox::kUIExperimentVerticalLayout)) {
On 2017/05/25 20:24:39, Peter Kasting wrote:
> Nit: {} not necessary
> 
> Another way to write all this might be:
> 
>   int row_height = GetTextHeight();
>   if (vertical layout)
>     row_height += kVerticalPadding + GetTextHeight();
>   const int icon_y = GetVerticalMargin() + (row_height - icon.height()) / 2;
> 
> This is shorter and IMO a little less error-prone.

Done. Thanks for the concrete suggestions!

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

https://codereview.chromium.org/2901953002/diff/20001/components/omnibox/brow...
components/omnibox/browser/omnibox_popup_model.cc:60: if (!description_width ||
description_on_separate_line ||
On 2017/05/25 20:24:39, Peter Kasting wrote:
> I feel like the caller should have passed true for
> |description_on_separate_line|, rather than checking it internally here.
> 
> This would also make for better testability, since you can specify the
behavior
> you want directly in the test.

Done.

https://codereview.chromium.org/2901953002/diff/20001/components/omnibox/brow...
components/omnibox/browser/omnibox_popup_model.cc:61:
base::FeatureList::IsEnabled(omnibox::kUIExperimentVerticalLayout)) {
On 2017/05/25 20:24:39, Peter Kasting wrote:
> Nit: Omnibox code doesn't usually use braces for cases like this

Done.

[tommycli, 2017-05-25 21:59:14]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-25 22:00:30]

Dry run: CQ is trying da patch.

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

[tommycli, 2017-05-25 22:00:59]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-25 22:01:54]

Dry run: CQ is trying da patch.

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

[tommycli, 2017-05-25 22:02:59]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-25 22:03:53]

Dry run: 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-05-26 00:20:50]

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

[commit-bot: I haz the power, 2017-05-26 00:20:50]

Dry run: This issue passed the CQ dry run.

[tommycli, 2017-05-26 20:43:49]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2017-05-26 20:43:49]

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/2901953002/#ps60001
(title: "remove stray includes")

[commit-bot: I haz the power, 2017-05-26 20:44:37]

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-05-26 20:50:36]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1495831429352520,
"parent_rev": "fa46590a75919b4892df4d26074cf074bd79b048", "commit_rev":
"26317da136aba1d8f3c46cd8347d396894416c7d"}

[commit-bot: I haz the power, 2017-05-26 20:50:47]

Description was changed from

==========
Omnibox UI Experiments: Add Vertical Layout experiment (title-on-top).

This is a minimal Views-only patch that renders the title above the
origin. It doesn't change any styling yet, nor does it touch Cocoa.

Those will be in followup CLs.

BUG=725599
==========

to

==========
Omnibox UI Experiments: Add Vertical Layout experiment (title-on-top).

This is a minimal Views-only patch that renders the title above the
origin. It doesn't change any styling yet, nor does it touch Cocoa.

Those will be in followup CLs.

BUG=725599

Review-Url: https://codereview.chromium.org/2901953002
Cr-Commit-Position: refs/heads/master@{#475119}
Committed:
https://chromium.googlesource.com/chromium/src/+/26317da136aba1d8f3c46cd8347d...
==========

[commit-bot: I haz the power, 2017-05-26 20:50:54]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/26317da136aba1d8f3c46cd8347d...