Omnibox UI Experiments: Add vertical margin variations to about:flags

Omnibox UI Experiments: Add vertical margin variations to about:flags

This CL implements it on Views only. There will be a followup to do
the variation on Mac.

BUG=717777
Review-Url: https://codereview.chromium.org/2854263002
Cr-Commit-Position: refs/heads/master@{#470050}
Committed: https://chromium.googlesource.com/chromium/src/+/2bb95c4284a7126fe819f7ead0cee521cab7ca87

[tommycli, 2017-05-02 23:10:46]

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

[commit-bot: I haz the power, 2017-05-02 23:11:12]

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-02 23:12:46]

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

[tommycli, 2017-05-02 23:12:47]

jdonnelly: PTAL to see if this is what you had in mind. screenshot in bug.

[commit-bot: I haz the power, 2017-05-02 23:14:30]

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

[commit-bot: I haz the power, 2017-05-02 23:14:31]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[tommycli, 2017-05-02 23:21:17]

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

[commit-bot: I haz the power, 2017-05-02 23:21:48]

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-02 23:46:19]

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

[commit-bot: I haz the power, 2017-05-02 23:46:20]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Justin Donnelly, 2017-05-02 23:51:17]

lgtm

I'm excited to try it out.

[tommycli, 2017-05-02 23:55:01]

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

[commit-bot: I haz the power, 2017-05-02 23:55:43]

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-02 23:56:44]

tommycli@chromium.org changed reviewers:
+ isherman@chromium.org, mpearson@chromium.org

[tommycli, 2017-05-02 23:56:45]

mpearson: PTAL components/omnibox
isherman: PTAL histograms

jdonnelly: thanks!

[Ilya Sherman, 2017-05-03 00:10:35]

histograms.xml lgtm

[tommycli, 2017-05-03 00:12:55]

On 2017/05/03 00:10:35, Ilya Sherman wrote:
> histograms.xml lgtm

isherman: thanks for the fast histograms review, as always :)

[commit-bot: I haz the power, 2017-05-03 00:56:53]

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

[commit-bot: I haz the power, 2017-05-03 00:56:54]

Dry run: This issue passed the CQ dry run.

[tommycli, 2017-05-03 15:29:15]

mpearson: ping, thanks!

[Mark P, 2017-05-03 18:38:59]

lgtm once you resolve the questions & comments below

--mark

https://codereview.chromium.org/2854263002/diff/60001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2854263002/diff/60001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:2769: {"omnibox-ui-vertical-margin",
nit: We're supposed to put new entries at the end.
(Frankly, I don't care.)

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

https://codereview.chromium.org/2854263002/diff/60001/components/omnibox/brow...
components/omnibox/browser/omnibox_field_trial.cc:20: #include
"components/omnibox/browser/omnibox_field_trial.h"
nit: this is already included above.

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

https://codereview.chromium.org/2854263002/diff/60001/components/omnibox/brow...
components/omnibox/browser/omnibox_field_trial.h:35: extern const base::Feature
kUIExperiments;
Double-check:
Please make sure that if we make this a feature, then we can independent
about:flags selectors that are both tied to the same feature, just set different
parameters.

[tommycli, 2017-05-03 18:49:46]

mpearson, thanks! Sending it in!

https://codereview.chromium.org/2854263002/diff/60001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2854263002/diff/60001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:2769: {"omnibox-ui-vertical-margin",
On 2017/05/03 18:38:59, Mark P wrote:
> nit: We're supposed to put new entries at the end.
> (Frankly, I don't care.)

Done.

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

https://codereview.chromium.org/2854263002/diff/60001/components/omnibox/brow...
components/omnibox/browser/omnibox_field_trial.cc:20: #include
"components/omnibox/browser/omnibox_field_trial.h"
On 2017/05/03 18:38:59, Mark P wrote:
> nit: this is already included above.

Done. Oops brain fart!

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

https://codereview.chromium.org/2854263002/diff/60001/components/omnibox/brow...
components/omnibox/browser/omnibox_field_trial.h:35: extern const base::Feature
kUIExperiments;
On 2017/05/03 18:38:59, Mark P wrote:
> Double-check:
> Please make sure that if we make this a feature, then we can independent
> about:flags selectors that are both tied to the same feature, just set
different
> parameters.

Done. I just tested this (one Feature, multiple about:flag selectors), and it
seems to work.

Admittedly, it's unprecedented.

I would say -- if indeed it doesn't work later on down the line, it wouldn't be
too disruptive in the code to split it into separate base::Feature instances.

[tommycli, 2017-05-03 18:50:58]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2017-05-03 18:50:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
jdonnelly@chromium.org, mpearson@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2854263002/#ps80001
(title: "address mpearson comments")

[commit-bot: I haz the power, 2017-05-03 18:51:58]

CQ is trying da patch.

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

[tommycli, 2017-05-03 19:02:54]

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

[tommycli, 2017-05-03 19:02:54]

pkasting: PTAL one last OWNER review for:

chrome/browser/ui/views/omnibox/omnibox_result_view.cc

[commit-bot: I haz the power, 2017-05-03 19:03:52]

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

[commit-bot: I haz the power, 2017-05-03 19:03:54]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Peter Kasting, 2017-05-04 23:39:48]

LGTM, but it would have been nice if the bug justified why we want this, and not
just "do X".  (I don't know why we want this.)

[Justin Donnelly, 2017-05-05 14:19:45]

On 2017/05/04 23:39:48, Peter Kasting (catching up) wrote:
> LGTM, but it would have been nice if the bug justified why we want this, and
not
> just "do X".  (I don't know why we want this.)

Added the following comment to the bug:

FYI, this is the first step in a series of experimental suggest UI changes we'll
be making. We'll be sharing a plan soon that describes the changes and our plans
for evaluating them.

[Justin Donnelly, 2017-05-05 14:19:52]

The CQ bit was checked by jdonnelly@chromium.org

[commit-bot: I haz the power, 2017-05-05 14:20:13]

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-05 14:23:02]

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

[commit-bot: I haz the power, 2017-05-05 14:23:04]

Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[tommycli, 2017-05-08 16:02:15]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2017-05-08 16:02:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
mpearson@chromium.org, jdonnelly@chromium.org, isherman@chromium.org,
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2854263002/#ps140001
(title: "update histograms")

[commit-bot: I haz the power, 2017-05-08 16:02:55]

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-08 16:34:18]

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

[commit-bot: I haz the power, 2017-05-08 16:34:19]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[tommycli, 2017-05-08 16:40:58]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2017-05-08 16:41:55]

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-08 17:03:17]

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

[commit-bot: I haz the power, 2017-05-08 17:03:19]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[tommycli, 2017-05-08 17:52:03]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2017-05-08 17:52:43]

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-08 18:07:08]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1494265923114890,
"parent_rev": "62c80a90afa761144d3d54eea40971f16dd9e72a", "commit_rev":
"2bb95c4284a7126fe819f7ead0cee521cab7ca87"}

[commit-bot: I haz the power, 2017-05-08 18:07:18]

Description was changed from

==========
Omnibox UI Experiments: Add vertical margin variations to about:flags

This CL implements it on Views only. There will be a followup to do
the variation on Mac.

BUG=717777
==========

to

==========
Omnibox UI Experiments: Add vertical margin variations to about:flags

This CL implements it on Views only. There will be a followup to do
the variation on Mac.

BUG=717777

Review-Url: https://codereview.chromium.org/2854263002
Cr-Commit-Position: refs/heads/master@{#470050}
Committed:
https://chromium.googlesource.com/chromium/src/+/2bb95c4284a7126fe819f7ead0ce...
==========

[commit-bot: I haz the power, 2017-05-08 18:07:20]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/2bb95c4284a7126fe819f7ead0ce...