Omnibox UI Experiments: Clean up naming for Vertical Margin

Omnibox UI Experiments: Clean up naming for Vertical Margin

Previous naming was a bit convoluted because we thought we would
coalesce all UI experiments into a single base::Feature.

New thinking is that it's cleanest to make a separate base::Feature for
each UI experiment.

BUG=717777
Review-Url: https://codereview.chromium.org/2874593002
Cr-Commit-Position: refs/heads/master@{#470729}
Committed: https://chromium.googlesource.com/chromium/src/+/6c821888667803cb403d104523637008200b5161

[tommycli, 2017-05-10 00:00:42]

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

[tommycli, 2017-05-10 00:00:43]

pkasting: PTAL. Just a cleanup followup.

[tommycli, 2017-05-10 00:25:53]

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

[commit-bot: I haz the power, 2017-05-10 00:27:29]

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-10 02:29:35]

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

[commit-bot: I haz the power, 2017-05-10 02:29:36]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has
not started yet; builder either lacks capacity or does not exist (misspelled?))
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet;
builder either lacks capacity or does not exist (misspelled?))
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build
has not started yet; builder either lacks capacity or does not exist
(misspelled?))
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
build has not started yet; builder either lacks capacity or does not exist
(misspelled?))
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build
has not started yet; builder either lacks capacity or does not exist
(misspelled?))
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
build has not started yet; builder either lacks capacity or does not exist
(misspelled?))

[tommycli, 2017-05-10 19:18:46]

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

[commit-bot: I haz the power, 2017-05-10 19:19:59]

Dry run: CQ is trying da patch.

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

[Peter Kasting, 2017-05-10 20:26:35]

LGTM

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

https://codereview.chromium.org/2874593002/diff/20001/components/omnibox/brow...
components/omnibox/browser/omnibox_field_trial.cc:67:
"OmniboixSearchProviderContextAllowHttpsUrls",
Nit: I just noticed "omnibox" is misspelled here.

https://codereview.chromium.org/2874593002/diff/20001/components/omnibox/brow...
components/omnibox/browser/omnibox_field_trial.cc:83: // Feature used for the
Vertical Margin UI experiment.
Nit: Maybe wouldn't capitalize V, M?

https://codereview.chromium.org/2874593002/diff/20001/components/omnibox/brow...
components/omnibox/browser/omnibox_field_trial.cc:85:
"OmniboxUIExperimentVerticalMargin", base::FEATURE_DISABLED_BY_DEFAULT};
We're not very consistent about whether to put "Omnibox" in front of these
things.  It seems like we should be.  Can we just always remove it?

[tommycli, 2017-05-10 20:30:39]

pkasting: Thanks!

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

https://codereview.chromium.org/2874593002/diff/20001/components/omnibox/brow...
components/omnibox/browser/omnibox_field_trial.cc:67:
"OmniboixSearchProviderContextAllowHttpsUrls",
On 2017/05/10 20:26:34, Peter Kasting wrote:
> Nit: I just noticed "omnibox" is misspelled here.

Done.

https://codereview.chromium.org/2874593002/diff/20001/components/omnibox/brow...
components/omnibox/browser/omnibox_field_trial.cc:83: // Feature used for the
Vertical Margin UI experiment.
On 2017/05/10 20:26:34, Peter Kasting wrote:
> Nit: Maybe wouldn't capitalize V, M?

Done.

https://codereview.chromium.org/2874593002/diff/20001/components/omnibox/brow...
components/omnibox/browser/omnibox_field_trial.cc:85:
"OmniboxUIExperimentVerticalMargin", base::FEATURE_DISABLED_BY_DEFAULT};
On 2017/05/10 20:26:34, Peter Kasting wrote:
> We're not very consistent about whether to put "Omnibox" in front of these
> things.  It seems like we should be.  Can we just always remove it?

I would probably advocate keeping it, since this string also appears in
enums.xml, where there's no context. If it just said "ExperimentVerticalMargin"
it might be confusing.

[Peter Kasting, 2017-05-10 20:31:42]

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

https://codereview.chromium.org/2874593002/diff/20001/components/omnibox/brow...
components/omnibox/browser/omnibox_field_trial.cc:85:
"OmniboxUIExperimentVerticalMargin", base::FEATURE_DISABLED_BY_DEFAULT};
On 2017/05/10 20:30:39, tommycli wrote:
> On 2017/05/10 20:26:34, Peter Kasting wrote:
> > We're not very consistent about whether to put "Omnibox" in front of these
> > things.  It seems like we should be.  Can we just always remove it?
> 
> I would probably advocate keeping it, since this string also appears in
> enums.xml, where there's no context. If it just said
"ExperimentVerticalMargin"
> it might be confusing.

Should we add it to the things that don't have it then?  Or is my desire for
consistency misguided here?

[tommycli, 2017-05-10 20:35:54]

you're not gonna like these answers...

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

https://codereview.chromium.org/2874593002/diff/20001/components/omnibox/brow...
components/omnibox/browser/omnibox_field_trial.cc:67:
"OmniboixSearchProviderContextAllowHttpsUrls",
On 2017/05/10 20:30:39, tommycli wrote:
> On 2017/05/10 20:26:34, Peter Kasting wrote:
> > Nit: I just noticed "omnibox" is misspelled here.
> 
> Done.

Gah, actually I can't change this, since a Finch experiment is already running
with the misspelled name. We can fix in a separate CL....

https://codereview.chromium.org/2874593002/diff/20001/components/omnibox/brow...
components/omnibox/browser/omnibox_field_trial.cc:85:
"OmniboxUIExperimentVerticalMargin", base::FEATURE_DISABLED_BY_DEFAULT};
On 2017/05/10 20:31:42, Peter Kasting wrote:
> On 2017/05/10 20:30:39, tommycli wrote:
> > On 2017/05/10 20:26:34, Peter Kasting wrote:
> > > We're not very consistent about whether to put "Omnibox" in front of these
> > > things.  It seems like we should be.  Can we just always remove it?
> > 
> > I would probably advocate keeping it, since this string also appears in
> > enums.xml, where there's no context. If it just said
> "ExperimentVerticalMargin"
> > it might be confusing.
> 
> Should we add it to the things that don't have it then?  Or is my desire for
> consistency misguided here?

I think that would be a good thing. But would that mess up finch configs? It
would probably also mess up existing histogram data for experiments that have
already been running.

In either case probably a separate CL right?

[tommycli, 2017-05-10 20:36:06]

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

[commit-bot: I haz the power, 2017-05-10 20:37:11]

Dry run: CQ is trying da patch.

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

[Peter Kasting, 2017-05-10 20:43:51]

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

https://codereview.chromium.org/2874593002/diff/20001/components/omnibox/brow...
components/omnibox/browser/omnibox_field_trial.cc:85:
"OmniboxUIExperimentVerticalMargin", base::FEATURE_DISABLED_BY_DEFAULT};
On 2017/05/10 20:35:54, tommycli wrote:
> On 2017/05/10 20:31:42, Peter Kasting wrote:
> > On 2017/05/10 20:30:39, tommycli wrote:
> > > On 2017/05/10 20:26:34, Peter Kasting wrote:
> > > > We're not very consistent about whether to put "Omnibox" in front of
these
> > > > things.  It seems like we should be.  Can we just always remove it?
> > > 
> > > I would probably advocate keeping it, since this string also appears in
> > > enums.xml, where there's no context. If it just said
> > "ExperimentVerticalMargin"
> > > it might be confusing.
> > 
> > Should we add it to the things that don't have it then?  Or is my desire for
> > consistency misguided here?
> 
> I think that would be a good thing. But would that mess up finch configs? It
> would probably also mess up existing histogram data for experiments that have
> already been running.
> 
> In either case probably a separate CL right?

Maybe?  I dunno.  My job as reviewer is to lob the hard questions at you, then
run.  :D

[tommycli, 2017-05-10 20:47:03]

On 2017/05/10 20:43:51, Peter Kasting wrote:
>
https://codereview.chromium.org/2874593002/diff/20001/components/omnibox/brow...
> File components/omnibox/browser/omnibox_field_trial.cc (right):
> 
>
https://codereview.chromium.org/2874593002/diff/20001/components/omnibox/brow...
> components/omnibox/browser/omnibox_field_trial.cc:85:
> "OmniboxUIExperimentVerticalMargin", base::FEATURE_DISABLED_BY_DEFAULT};
> On 2017/05/10 20:35:54, tommycli wrote:
> > On 2017/05/10 20:31:42, Peter Kasting wrote:
> > > On 2017/05/10 20:30:39, tommycli wrote:
> > > > On 2017/05/10 20:26:34, Peter Kasting wrote:
> > > > > We're not very consistent about whether to put "Omnibox" in front of
> these
> > > > > things.  It seems like we should be.  Can we just always remove it?
> > > > 
> > > > I would probably advocate keeping it, since this string also appears in
> > > > enums.xml, where there's no context. If it just said
> > > "ExperimentVerticalMargin"
> > > > it might be confusing.
> > > 
> > > Should we add it to the things that don't have it then?  Or is my desire
for
> > > consistency misguided here?
> > 
> > I think that would be a good thing. But would that mess up finch configs? It
> > would probably also mess up existing histogram data for experiments that
have
> > already been running.
> > 
> > In either case probably a separate CL right?
> 
> Maybe?  I dunno.  My job as reviewer is to lob the hard questions at you, then
> run.  :D

Alrighty. I'll send this one in as-is, and i'll make a note to investigate the
misspelling and inconsistency issue. I'm not sure but my guess is that we're
stuck with the names as-is if there's any significant existing UMA data, and
we'll strive for consistency and correctness going forward.

[tommycli, 2017-05-10 20:48:22]

The CQ bit was unchecked by tommycli@chromium.org

[tommycli, 2017-05-10 20:48:22]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2017-05-10 20:48:23]

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/2874593002/#ps60001
(title: "change back misspelling")

[commit-bot: I haz the power, 2017-05-10 20:48:59]

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-10 22:20:54]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494449302778440,
"parent_rev": "eadedef7305278a22fca35db34b2831135ff5fb2", "commit_rev":
"6c821888667803cb403d104523637008200b5161"}

[commit-bot: I haz the power, 2017-05-10 22:21:12]

Description was changed from

==========
Omnibox UI Experiments: Clean up naming for Vertical Margin

Previous naming was a bit convoluted because we thought we would
coalesce all UI experiments into a single base::Feature.

New thinking is that it's cleanest to make a separate base::Feature for
each UI experiment.

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

to

==========
Omnibox UI Experiments: Clean up naming for Vertical Margin

Previous naming was a bit convoluted because we thought we would
coalesce all UI experiments into a single base::Feature.

New thinking is that it's cleanest to make a separate base::Feature for
each UI experiment.

BUG=717777

Review-Url: https://codereview.chromium.org/2874593002
Cr-Commit-Position: refs/heads/master@{#470729}
Committed:
https://chromium.googlesource.com/chromium/src/+/6c821888667803cb403d10452363...
==========

[commit-bot: I haz the power, 2017-05-10 22:21:14]

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