Omnibox UI Experiments: Enable trimming of trivial subdomains.

Omnibox UI Experiments: Enable trimming of trivial subdomains.

This trims trivial subdomains whenever the experimental flag is on.

We still need to followup and add a match_in_subdomain flag to prevent
trimming of trivial subdomains that are part of the match.

Depends on https://codereview.chromium.org/2939423003/ landing first.

BUG=732582

[tommycli, 2017-06-23 00:10:53]

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

[tommycli, 2017-06-23 00:10:54]

jdonnelly: PTAL, thanks!

https://codereview.chromium.org/2945173007/diff/1/components/omnibox/browser/...
File components/omnibox/browser/autocomplete_match.cc (right):

https://codereview.chromium.org/2945173007/diff/1/components/omnibox/browser/...
components/omnibox/browser/autocomplete_match.cc:538: }
This pattern now makes me feel that the smart thing to do for scheme trimming is
basically: if (trim_scheme and feature enabled and scheme is HTTPS) replace
scheme with HTTP and allow url_formatter to trim that off.

If you agree I can followup and do that...

[tommycli, 2017-06-23 00:11:05]

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

[commit-bot: I haz the power, 2017-06-23 00:11:20]

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-06-23 02:07:55]

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

[commit-bot: I haz the power, 2017-06-23 02:07:56]

Dry run: This issue passed the CQ dry run.

[Justin Donnelly, 2017-06-23 15:12:25]

Maybe mention in the description that this depends on
https://codereview.chromium.org/2939423003/ since that hasn't landed yet.

https://codereview.chromium.org/2945173007/diff/1/components/omnibox/browser/...
File components/omnibox/browser/autocomplete_match.cc (right):

https://codereview.chromium.org/2945173007/diff/1/components/omnibox/browser/...
components/omnibox/browser/autocomplete_match.cc:538: }
On 2017/06/23 00:10:54, tommycli wrote:
> This pattern now makes me feel that the smart thing to do for scheme trimming
is
> basically: if (trim_scheme and feature enabled and scheme is HTTPS) replace
> scheme with HTTP and allow url_formatter to trim that off.
> 
> If you agree I can followup and do that...

That's both an elegant solution to the code duplication and also feels like a
weird hack. :) I'm inclined to stick with what you have here and just do the
right thing (put it in url_formatter and/or refactor as pkasting suggested) if
and when we decide it's permanent.

[tommycli, 2017-06-23 18:22:28]

Description was changed from

==========
Omnibox UI Experiments: Enable trimming of trivial subdomains.

This trims trivial subdomains whenever the experimental flag is on.

We still need to followup and add a match_in_subdomain flag to prevent
trimming of trivial subdomains that are part of the match.

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

to

==========
Omnibox UI Experiments: Enable trimming of trivial subdomains.

This trims trivial subdomains whenever the experimental flag is on.

We still need to followup and add a match_in_subdomain flag to prevent
trimming of trivial subdomains that are part of the match.

Depends on https://codereview.chromium.org/2939423003/ landing first.

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

[tommycli, 2017-06-23 18:22:41]

On 2017/06/23 15:12:25, Justin Donnelly wrote:
> Maybe mention in the description that this depends on
> https://codereview.chromium.org/2939423003/ since that hasn't landed yet.
> 
>
https://codereview.chromium.org/2945173007/diff/1/components/omnibox/browser/...
> File components/omnibox/browser/autocomplete_match.cc (right):
> 
>
https://codereview.chromium.org/2945173007/diff/1/components/omnibox/browser/...
> components/omnibox/browser/autocomplete_match.cc:538: }
> On 2017/06/23 00:10:54, tommycli wrote:
> > This pattern now makes me feel that the smart thing to do for scheme
trimming
> is
> > basically: if (trim_scheme and feature enabled and scheme is HTTPS) replace
> > scheme with HTTP and allow url_formatter to trim that off.
> > 
> > If you agree I can followup and do that...
> 
> That's both an elegant solution to the code duplication and also feels like a
> weird hack. :) I'm inclined to stick with what you have here and just do the
> right thing (put it in url_formatter and/or refactor as pkasting suggested) if
> and when we decide it's permanent.

Sounds good I updated the CL description.

[tommycli, 2017-06-23 18:23:06]

https://codereview.chromium.org/2945173007/diff/1/components/omnibox/browser/...
File components/omnibox/browser/autocomplete_match.cc (right):

https://codereview.chromium.org/2945173007/diff/1/components/omnibox/browser/...
components/omnibox/browser/autocomplete_match.cc:538: }
On 2017/06/23 15:12:25, Justin Donnelly wrote:
> On 2017/06/23 00:10:54, tommycli wrote:
> > This pattern now makes me feel that the smart thing to do for scheme
trimming
> is
> > basically: if (trim_scheme and feature enabled and scheme is HTTPS) replace
> > scheme with HTTP and allow url_formatter to trim that off.
> > 
> > If you agree I can followup and do that...
> 
> That's both an elegant solution to the code duplication and also feels like a
> weird hack. :) I'm inclined to stick with what you have here and just do the
> right thing (put it in url_formatter and/or refactor as pkasting suggested) if
> and when we decide it's permanent.

Gotcha, alright good to hear my first instinct was the correct one then. :)

[Justin Donnelly, 2017-06-23 19:55:33]

lgtm

Oops! Sorry, I meant to have lgtm'd this in my previous message.