Omnibox UI Experiments: Hook up elide-after-host feature flag.

Omnibox UI Experiments: Hook up elide-after-host feature flag.

This hooks up the (renamed) elide-after-host feature flag to the actual
implementation.

BUG=732582
Review-Url: https://codereview.chromium.org/2961393002
Cr-Commit-Position: refs/heads/master@{#484410}
Committed: https://chromium.googlesource.com/chromium/src/+/10b4b19297da4a3cd28944da3134df517946b50b

[tommycli, 2017-06-29 21:20:48]

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

[commit-bot: I haz the power, 2017-06-29 21:21: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-06-29 21:21:26]

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

[tommycli, 2017-06-29 21:21:27]

mpearson: PTAL. This renames a feature flag and hooks it up to a flag of
url_formatter.

[commit-bot: I haz the power, 2017-06-29 22:32:28]

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

[commit-bot: I haz the power, 2017-06-29 22:32:29]

Dry run: This issue passed the CQ dry run.

[tommycli, 2017-07-05 16:33:32]

On 2017/06/29 21:21:27, tommycli wrote:
> mpearson: PTAL. This renames a feature flag and hooks it up to a flag of
> url_formatter.

mpearson: ping, thanks!

[Mark P, 2017-07-05 18:38:46]

lgtm

Sorry for the delay.  I got caught up in other things I needed to do last week. 
There are some minor concerns / requests below; feel free to submit after you
resolve them.

--mark

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

https://codereview.chromium.org/2961393002/diff/20001/components/omnibox/brow...
components/omnibox/browser/autocomplete_match_unittest.cc:165:
L"google.com/\x2026\x0000"};
Care to test the case when we're not hiding the scheme as well?

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

https://codereview.chromium.org/2961393002/diff/20001/components/omnibox/brow...
components/omnibox/browser/omnibox_field_trial.h:38: extern const base::Feature
kUIExperimentHideSuggestionUrlPath;
As near as I can tell, you checked in this declaration for a feature flag that
wasn't used.  Is that correct?

https://codereview.chromium.org/2961393002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/enums.xml (left):

https://codereview.chromium.org/2961393002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/enums.xml:22478: -     
label="OmniboxUIExperimentHideSuggestionUrlPath:disabled"/>
Do not remove entries from histograms.xml.  It may be relevant for the builds
before this changelist is submitted.

https://codereview.chromium.org/2961393002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2961393002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:65616: +    to this tree, the raster
duration and the time for which the pending tree
I don't think you intended to change this.  A rebase error?

[tommycli, 2017-07-05 19:38:09]

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

[tommycli, 2017-07-05 19:38:27]

mpearson: Thanks! I pushed back on one item below, so it's probably best that
you take another look before I submit.

Tommy

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

https://codereview.chromium.org/2961393002/diff/20001/components/omnibox/brow...
components/omnibox/browser/autocomplete_match_unittest.cc:165:
L"google.com/\x2026\x0000"};
On 2017/07/05 18:38:45, Mark P wrote:
> Care to test the case when we're not hiding the scheme as well?

Done.

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

https://codereview.chromium.org/2961393002/diff/20001/components/omnibox/brow...
components/omnibox/browser/omnibox_field_trial.h:38: extern const base::Feature
kUIExperimentHideSuggestionUrlPath;
On 2017/07/05 18:38:45, Mark P wrote:
> As near as I can tell, you checked in this declaration for a feature flag that
> wasn't used.  Is that correct?

Hey... I wasn't able to grok your question -- do you mind rephrasing it?

The feature flag used to be called "hide suggestion url path", but wasn't
implemented yet... it was just a placeholder feature flag.

Through the course of implementation, Peter suggested that a more accurate name
would be "elide suggestion url after host" -- so I'm updating the feature flag
name to more accurately reflect what's going on.

https://codereview.chromium.org/2961393002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/enums.xml (left):

https://codereview.chromium.org/2961393002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/enums.xml:22478: -     
label="OmniboxUIExperimentHideSuggestionUrlPath:disabled"/>
On 2017/07/05 18:38:46, Mark P wrote:
> Do not remove entries from histograms.xml.  It may be relevant for the builds
> before this changelist is submitted.

This possibility has only survived for about ... 2 weeks. Looking at
LoginCustomFlags, there are only a few hundred instances and on Canary only,
which will fall off pretty quickly.

I guess I'd be more in favor of trimming these dead buckets out than leaving
them in. Thoughts?

https://codereview.chromium.org/2961393002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2961393002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:65616: +    to this tree, the raster
duration and the time for which the pending tree
On 2017/07/05 18:38:46, Mark P wrote:
> I don't think you intended to change this.  A rebase error?

git cl format seems to reformat histograms.xml whenever it re-formats enums.xml.
I'm not sure why -- but otherwise I get presubmit errors.

[commit-bot: I haz the power, 2017-07-05 19:38:35]

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-07-05 19:41:23]

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

[commit-bot: I haz the power, 2017-07-05 19:41:24]

Dry run: 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...)

[Mark P, 2017-07-05 19:43:24]

still mostly lgtm, except I still disagree with your enums.xml comments :-)

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

https://codereview.chromium.org/2961393002/diff/20001/components/omnibox/brow...
components/omnibox/browser/omnibox_field_trial.h:38: extern const base::Feature
kUIExperimentHideSuggestionUrlPath;
On 2017/07/05 19:38:26, tommycli wrote:
> On 2017/07/05 18:38:45, Mark P wrote:
> > As near as I can tell, you checked in this declaration for a feature flag
that
> > wasn't used.  Is that correct?
> 
> Hey... I wasn't able to grok your question -- do you mind rephrasing it?
> 
> The feature flag used to be called "hide suggestion url path", but wasn't
> implemented yet... it was just a placeholder feature flag.
> 
> Through the course of implementation, Peter suggested that a more accurate
name
> would be "elide suggestion url after host" -- so I'm updating the feature flag
> name to more accurately reflect what's going on.

You answered my question. :-)  It was a placeholder declaration.

https://codereview.chromium.org/2961393002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/enums.xml (left):

https://codereview.chromium.org/2961393002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/enums.xml:22478: -     
label="OmniboxUIExperimentHideSuggestionUrlPath:disabled"/>
On 2017/07/05 19:38:26, tommycli wrote:
> On 2017/07/05 18:38:46, Mark P wrote:
> > Do not remove entries from histograms.xml.  It may be relevant for the
builds
> > before this changelist is submitted.
> 
> This possibility has only survived for about ... 2 weeks. Looking at
> LoginCustomFlags, there are only a few hundred instances and on Canary only,
> which will fall off pretty quickly.
> 
> I guess I'd be more in favor of trimming these dead buckets out than leaving
> them in. Thoughts?

This is against best practices.  Part of the key point is that all data remains
interpretable for all time.
https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo...

https://codereview.chromium.org/2961393002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2961393002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:65616: +    to this tree, the raster
duration and the time for which the pending tree
On 2017/07/05 19:38:26, tommycli wrote:
> On 2017/07/05 18:38:46, Mark P wrote:
> > I don't think you intended to change this.  A rebase error?
> 
> git cl format seems to reformat histograms.xml whenever it re-formats
enums.xml.
> I'm not sure why -- but otherwise I get presubmit errors.

Acknowledged.

[tommycli, 2017-07-05 19:49:51]

On 2017/07/05 19:43:24, Mark P wrote:
> still mostly lgtm, except I still disagree with your enums.xml comments :-)
> 
>
https://codereview.chromium.org/2961393002/diff/20001/components/omnibox/brow...
> File components/omnibox/browser/omnibox_field_trial.h (left):
> 
>
https://codereview.chromium.org/2961393002/diff/20001/components/omnibox/brow...
> components/omnibox/browser/omnibox_field_trial.h:38: extern const
base::Feature
> kUIExperimentHideSuggestionUrlPath;
> On 2017/07/05 19:38:26, tommycli wrote:
> > On 2017/07/05 18:38:45, Mark P wrote:
> > > As near as I can tell, you checked in this declaration for a feature flag
> that
> > > wasn't used.  Is that correct?
> > 
> > Hey... I wasn't able to grok your question -- do you mind rephrasing it?
> > 
> > The feature flag used to be called "hide suggestion url path", but wasn't
> > implemented yet... it was just a placeholder feature flag.
> > 
> > Through the course of implementation, Peter suggested that a more accurate
> name
> > would be "elide suggestion url after host" -- so I'm updating the feature
flag
> > name to more accurately reflect what's going on.
> 
> You answered my question. :-)  It was a placeholder declaration.
> 
>
https://codereview.chromium.org/2961393002/diff/20001/tools/metrics/histogram...
> File tools/metrics/histograms/enums.xml (left):
> 
>
https://codereview.chromium.org/2961393002/diff/20001/tools/metrics/histogram...
> tools/metrics/histograms/enums.xml:22478: -     
> label="OmniboxUIExperimentHideSuggestionUrlPath:disabled"/>
> On 2017/07/05 19:38:26, tommycli wrote:
> > On 2017/07/05 18:38:46, Mark P wrote:
> > > Do not remove entries from histograms.xml.  It may be relevant for the
> builds
> > > before this changelist is submitted.
> > 
> > This possibility has only survived for about ... 2 weeks. Looking at
> > LoginCustomFlags, there are only a few hundred instances and on Canary only,
> > which will fall off pretty quickly.
> > 
> > I guess I'd be more in favor of trimming these dead buckets out than leaving
> > them in. Thoughts?
> 
> This is against best practices.  Part of the key point is that all data
remains
> interpretable for all time.
>
https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo...
> 
>
https://codereview.chromium.org/2961393002/diff/20001/tools/metrics/histogram...
> File tools/metrics/histograms/histograms.xml (right):
> 
>
https://codereview.chromium.org/2961393002/diff/20001/tools/metrics/histogram...
> tools/metrics/histograms/histograms.xml:65616: +    to this tree, the raster
> duration and the time for which the pending tree
> On 2017/07/05 19:38:26, tommycli wrote:
> > On 2017/07/05 18:38:46, Mark P wrote:
> > > I don't think you intended to change this.  A rebase error?
> > 
> > git cl format seems to reformat histograms.xml whenever it re-formats
> enums.xml.
> > I'm not sure why -- but otherwise I get presubmit errors.
> 
> Acknowledged.

mpearson: Thanks for the extra context. I'll restore those enum.xml entries I
deleted then I will send it in. Thanks!

[tommycli, 2017-07-05 21:58:28]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2017-07-05 21:58:29]

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/2961393002/#ps80001
(title: "fix")

[commit-bot: I haz the power, 2017-07-05 21:58: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-07-05 22:02:03]

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

[commit-bot: I haz the power, 2017-07-05 22:02:04]

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-07-05 22:32:53]

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

[tommycli, 2017-07-05 22:33:07]

The CQ bit was unchecked by tommycli@chromium.org

[tommycli, 2017-07-05 22:33:08]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2017-07-05 22:33:08]

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/2961393002/#ps100001
(title: "merge")

[commit-bot: I haz the power, 2017-07-05 22:33: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-07-06 00:24:44]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1499293988165380,
"parent_rev": "39b1cbc91fa0f3e57eb4f1c2f3807e24af82657a", "commit_rev":
"10b4b19297da4a3cd28944da3134df517946b50b"}

[commit-bot: I haz the power, 2017-07-06 00:24:59]

Description was changed from

==========
Omnibox UI Experiments: Hook up elide-after-host feature flag.

This hooks up the (renamed) elide-after-host feature flag to the actual
implementation.

BUG=732582
==========

to

==========
Omnibox UI Experiments: Hook up elide-after-host feature flag.

This hooks up the (renamed) elide-after-host feature flag to the actual
implementation.

BUG=732582

Review-Url: https://codereview.chromium.org/2961393002
Cr-Commit-Position: refs/heads/master@{#484410}
Committed:
https://chromium.googlesource.com/chromium/src/+/10b4b19297da4a3cd28944da3134...
==========

[commit-bot: I haz the power, 2017-07-06 00:25:00]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/10b4b19297da4a3cd28944da3134...