Omnibox UI Experiments: Refactor HTTPS trimming into UrlFormatter.

Omnibox UI Experiments: Refactor HTTPS trimming into UrlFormatter.

Given that we must implement elide-after-host logic within UrlFormatter,
consolidate the other experimental URL trimming logic into the same
method.

BUG=732582
Review-Url: https://codereview.chromium.org/2963883002
Cr-Commit-Position: refs/heads/master@{#483497}
Committed: https://chromium.googlesource.com/chromium/src/+/72014f6a8ab12897583185fc6c5b4752fcbaa278

[tommycli, 2017-06-28 22:29:02]

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

[tommycli, 2017-06-28 22:29:02]

pkasting: PTAL. followup from earlier today

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

https://codereview.chromium.org/2963883002/diff/1/components/omnibox/browser/...
components/omnibox/browser/autocomplete_match_unittest.cc:114:
TEST(AutocompleteMatchTest, FormatUrlForSuggestionDisplay) {
Modify this test to solely do the sanity check for the feature flags. We let the
URL Formatter test cover the formatting and offset adjustment correctness.

[Peter Kasting, 2017-06-28 23:09:59]

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

https://codereview.chromium.org/2963883002/diff/1/components/omnibox/browser/...
components/omnibox/browser/autocomplete_match.cc:492: base::string16
AutocompleteMatch::FormatUrlForSuggestionDisplay(
I think we should nuke all these functions now.  Instead, just provide a simple
accessor somewhere to get the right format types; then all the places calling
these can call the real underlying FormatURL functions and not duplicate the
"compute the format types" logic.

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

https://codereview.chromium.org/2963883002/diff/1/components/omnibox/browser/...
components/omnibox/browser/autocomplete_match_unittest.cc:114:
TEST(AutocompleteMatchTest, FormatUrlForSuggestionDisplay) {
On 2017/06/28 22:29:02, tommycli wrote:
> Modify this test to solely do the sanity check for the feature flags. We let
the
> URL Formatter test cover the formatting and offset adjustment correctness.

In that case it seems like you don't need |normal_cases| at all.

https://codereview.chromium.org/2963883002/diff/1/components/url_formatter/ur...
File components/url_formatter/url_formatter.cc (right):

https://codereview.chromium.org/2963883002/diff/1/components/url_formatter/ur...
components/url_formatter/url_formatter.cc:551: // means that if we trim
"http://" off a URL whose host starts with "ftp." and
Nit: "http://" -> the scheme

https://codereview.chromium.org/2963883002/diff/1/components/url_formatter/ur...
components/url_formatter/url_formatter.cc:568: url_string =
url_string.substr(scheme_size);
I noticed we need to compute the scheme size above already.  It seems like we
can do something like this:

  const size_t scheme_size = static_cast<size_t>(
      parsed.CountCharactersBefore(url::Parsed::USERNAME, true));
  ... use this temp above ...

    ... and use it here:
    url_string.erase(0, scheme_size);

https://codereview.chromium.org/2963883002/diff/1/components/url_formatter/ur...
File components/url_formatter/url_formatter_unittest.cc (right):

https://codereview.chromium.org/2963883002/diff/1/components/url_formatter/ur...
components/url_formatter/url_formatter_unittest.cc:841: // -------- omit https
--------
I kinda feel like this block should go after your block for your other
experimental flag, to keep the test order matching the bitfield order.  Same
comment for the block you add below.

https://codereview.chromium.org/2963883002/diff/1/components/url_formatter/ur...
components/url_formatter/url_formatter_unittest.cc:842: {"omit https",
"http://www.google.com/",
This URL uses HTTP and not HTTPS, so it doesn't test your new flag.

https://codereview.chromium.org/2963883002/diff/1/components/url_formatter/ur...
components/url_formatter/url_formatter_unittest.cc:843: kFormatUrlOmitHTTP |
kFormatUrlExperimentalOmitHTTPS,
Seems like we don't need (or, honestly, want) to pass OmitHTTP here or below? 
And I'm not really sure why we have a couple cases just below testing
username/password eliding.  Maybe _not_ eliding those would be a better test, if
we're going to test them?

But you are missing a check that "omit HTTPS but not omit HTTP" does not strip
http://.

[tommycli, 2017-06-29 00:19:44]

pkasting: Thanks!

For your reference, here was the original patch that moved the providers away
from generating their own format_types:

https://codereview.chromium.org/2940973002/

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

https://codereview.chromium.org/2963883002/diff/1/components/omnibox/browser/...
components/omnibox/browser/autocomplete_match.cc:492: base::string16
AutocompleteMatch::FormatUrlForSuggestionDisplay(
On 2017/06/28 23:09:59, Peter Kasting wrote:
> I think we should nuke all these functions now.  Instead, just provide a
simple
> accessor somewhere to get the right format types; then all the places calling
> these can call the real underlying FormatURL functions and not duplicate the
> "compute the format types" logic.

Done.

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

https://codereview.chromium.org/2963883002/diff/1/components/omnibox/browser/...
components/omnibox/browser/autocomplete_match_unittest.cc:114:
TEST(AutocompleteMatchTest, FormatUrlForSuggestionDisplay) {
On 2017/06/28 23:09:59, Peter Kasting wrote:
> On 2017/06/28 22:29:02, tommycli wrote:
> > Modify this test to solely do the sanity check for the feature flags. We let
> the
> > URL Formatter test cover the formatting and offset adjustment correctness.
> 
> In that case it seems like you don't need |normal_cases| at all.

Done. I revamped this to only sanity check the format flags.

https://codereview.chromium.org/2963883002/diff/1/components/url_formatter/ur...
File components/url_formatter/url_formatter.cc (right):

https://codereview.chromium.org/2963883002/diff/1/components/url_formatter/ur...
components/url_formatter/url_formatter.cc:551: // means that if we trim
"http://" off a URL whose host starts with "ftp." and
On 2017/06/28 23:09:59, Peter Kasting wrote:
> Nit: "http://" -> the scheme

Done.

https://codereview.chromium.org/2963883002/diff/1/components/url_formatter/ur...
components/url_formatter/url_formatter.cc:568: url_string =
url_string.substr(scheme_size);
On 2017/06/28 23:09:59, Peter Kasting wrote:
> I noticed we need to compute the scheme size above already.  It seems like we
> can do something like this:
> 
>   const size_t scheme_size = static_cast<size_t>(
>       parsed.CountCharactersBefore(url::Parsed::USERNAME, true));
>   ... use this temp above ...
> 
>     ... and use it here:
>     url_string.erase(0, scheme_size);

Done.

https://codereview.chromium.org/2963883002/diff/1/components/url_formatter/ur...
File components/url_formatter/url_formatter_unittest.cc (right):

https://codereview.chromium.org/2963883002/diff/1/components/url_formatter/ur...
components/url_formatter/url_formatter_unittest.cc:841: // -------- omit https
--------
On 2017/06/28 23:09:59, Peter Kasting wrote:
> I kinda feel like this block should go after your block for your other
> experimental flag, to keep the test order matching the bitfield order.  Same
> comment for the block you add below.

Done.

https://codereview.chromium.org/2963883002/diff/1/components/url_formatter/ur...
components/url_formatter/url_formatter_unittest.cc:842: {"omit https",
"http://www.google.com/",
On 2017/06/28 23:09:59, Peter Kasting wrote:
> This URL uses HTTP and not HTTPS, so it doesn't test your new flag.

Done.

https://codereview.chromium.org/2963883002/diff/1/components/url_formatter/ur...
components/url_formatter/url_formatter_unittest.cc:843: kFormatUrlOmitHTTP |
kFormatUrlExperimentalOmitHTTPS,
On 2017/06/28 23:09:59, Peter Kasting wrote:
> Seems like we don't need (or, honestly, want) to pass OmitHTTP here or below? 
> And I'm not really sure why we have a couple cases just below testing
> username/password eliding.  Maybe _not_ eliding those would be a better test,
if
> we're going to test them?
> 
> But you are missing a check that "omit HTTPS but not omit HTTP" does not strip
> http://.

Done.

Also testing username/password eliding to verify interaction (like HTTP tests
do). I added another test that makes sure we can still preserve username /
password even if we elide scheme.

[tommycli, 2017-06-29 00:19:49]

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

[commit-bot: I haz the power, 2017-06-29 00:20:10]

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-06-29 01:05:44]

LGTM

Man, I sure don't like /* name_of_parameter */ comments in function calls.  It's
so noisy and distracting :(

https://codereview.chromium.org/2963883002/diff/40001/components/omnibox/brow...
File components/omnibox/browser/autocomplete_match.h (right):

https://codereview.chromium.org/2963883002/diff/40001/components/omnibox/brow...
components/omnibox/browser/autocomplete_match.h:215: static
url_formatter::FormatUrlTypes GetFormatForSuggestionDisplay(
Nit: GetFormatTypesForSuggestionDisplay()?

Or maybe this should be GetFormatTypesForDisplay() or even just
GetFormatTypes()?  "Suggestion" isn't used much in the omnibox code and I don't
really know what it means, but this function does seem relevant to anywhere
we're formatting an AutocompleteMatch for display.

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

https://codereview.chromium.org/2963883002/diff/40001/components/omnibox/brow...
components/omnibox/browser/autocomplete_match_unittest.cc:114:
TEST(AutocompleteMatchTest, GetFormatForSuggestionDisplay) {
It's OK I guess, but this test does seem a bit change-detector-y.  If we inline
this function, for example, the test disappears.  I would think it might be
better to just look at how the match strings are formatted for display; we have
to check that anyway, and once we do, checking this underlying bit seems
unnecessary.

https://codereview.chromium.org/2963883002/diff/40001/components/omnibox/brow...
File components/omnibox/browser/history_url_provider.cc (right):

https://codereview.chromium.org/2963883002/diff/40001/components/omnibox/brow...
components/omnibox/browser/history_url_provider.cc:1146: const
url_formatter::FormatUrlTypes fill_into_edit_format_types =
I suspect what we want here might be:

  const auto format_types = AutocompleteMatch::GetFormatForSuggestionDisplay(
      params.trim_http && !history_match.match_in_scheme);
  match.fill_into_edit = ...format_types &
~url_formatter::kFormatUrlExperimentalOmitHTTPS...

i.e. we want to use the same flags here and below, except it's important never
to trim HTTPS in the fill_into_edit lest it be misinterpreted.

[commit-bot: I haz the power, 2017-06-29 01:24:59]

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

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

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tommycli, 2017-06-29 16:46:46]

Thanks! I also removed the /* name_of_parameter */. It's not really necessary
for a single-param function anyways.

https://codereview.chromium.org/2963883002/diff/40001/components/omnibox/brow...
File components/omnibox/browser/autocomplete_match.h (right):

https://codereview.chromium.org/2963883002/diff/40001/components/omnibox/brow...
components/omnibox/browser/autocomplete_match.h:215: static
url_formatter::FormatUrlTypes GetFormatForSuggestionDisplay(
On 2017/06/29 01:05:44, Peter Kasting wrote:
> Nit: GetFormatTypesForSuggestionDisplay()?
> 
> Or maybe this should be GetFormatTypesForDisplay() or even just
> GetFormatTypes()?  "Suggestion" isn't used much in the omnibox code and I
don't
> really know what it means, but this function does seem relevant to anywhere
> we're formatting an AutocompleteMatch for display.

Done. GetFormatTypes -- love the brevity. Indeed it being a static function of
AutocompleteMatch should already explain that it's for autocomplete matches.

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

https://codereview.chromium.org/2963883002/diff/40001/components/omnibox/brow...
components/omnibox/browser/autocomplete_match_unittest.cc:114:
TEST(AutocompleteMatchTest, GetFormatForSuggestionDisplay) {
On 2017/06/29 01:05:44, Peter Kasting wrote:
> It's OK I guess, but this test does seem a bit change-detector-y.  If we
inline
> this function, for example, the test disappears.  I would think it might be
> better to just look at how the match strings are formatted for display; we
have
> to check that anyway, and once we do, checking this underlying bit seems
> unnecessary.

Done. No problem. I changed it back to just sanity-check that the feature flags
and the trim_strings parameter work

https://codereview.chromium.org/2963883002/diff/40001/components/omnibox/brow...
File components/omnibox/browser/history_url_provider.cc (right):

https://codereview.chromium.org/2963883002/diff/40001/components/omnibox/brow...
components/omnibox/browser/history_url_provider.cc:1146: const
url_formatter::FormatUrlTypes fill_into_edit_format_types =
On 2017/06/29 01:05:44, Peter Kasting wrote:
> I suspect what we want here might be:
> 
>   const auto format_types = AutocompleteMatch::GetFormatForSuggestionDisplay(
>       params.trim_http && !history_match.match_in_scheme);
>   match.fill_into_edit = ...format_types &
> ~url_formatter::kFormatUrlExperimentalOmitHTTPS...
> 
> i.e. we want to use the same flags here and below, except it's important never
> to trim HTTPS in the fill_into_edit lest it be misinterpreted.

Done. Yes, that makes sense. I missed the "formatted string with equivalent
meaning" catch below...

[tommycli, 2017-06-29 16:46:49]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2017-06-29 16:46:50]

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/2963883002/#ps60001
(title: "address comments")

[commit-bot: I haz the power, 2017-06-29 16:47:04]

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-29 17:45:55]

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

[commit-bot: I haz the power, 2017-06-29 17:45:56]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tommycli, 2017-06-29 17:50:04]

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

[commit-bot: I haz the power, 2017-06-29 17:50:21]

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-29 18:50:19]

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

[commit-bot: I haz the power, 2017-06-29 18:50:20]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tommycli, 2017-06-29 19:08:12]

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

[commit-bot: I haz the power, 2017-06-29 19:08:40]

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 19:09:47]

pkasting: Previously, "http://" was formatted to "http:" on OmitHTTP due to a
quirk of how the scheme trimming was implemented.

During the refactor, it's a bit more consistent now and "http://" is formatted
to "" with OmitHTTP. I updated the Mac test to reflect that, as well as added a
test on the url_formatter side.

This seems like the correct thing to do, but please object if I'm missing some
context here.

Thanks,

Tommy

[commit-bot: I haz the power, 2017-06-29 19:35:56]

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

[commit-bot: I haz the power, 2017-06-29 19:35:57]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Peter Kasting, 2017-06-29 19:54:21]

On 2017/06/29 19:09:47, tommycli wrote:
> pkasting: Previously, "http://" was formatted to "http:" on OmitHTTP due to a
> quirk of how the scheme trimming was implemented.
> 
> During the refactor, it's a bit more consistent now and "http://" is formatted
> to "" with OmitHTTP. I updated the Mac test to reflect that, as well as added
a
> test on the url_formatter side.
> 
> This seems like the correct thing to do, but please object if I'm missing some
> context here.

"http://" is clearly an invalid URL, so I suspect it doesn't much matter what we
do.  The old behavior is definitely nonsensical and yours is better.  The
alternate route would be to not elide anything in this case.  I can't really see
that it matters which one we pick, so this still LGTM.

[Peter Kasting, 2017-06-29 19:54:47]

Apparently forgot to publish this

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

https://codereview.chromium.org/2963883002/diff/80001/components/omnibox/brow...
components/omnibox/browser/autocomplete_match_unittest.cc:139: for
(FormatUrlTestData& test_case : normal_cases) {
Nit: {} unnecessary (2 places)

[tommycli, 2017-06-29 20:27:50]

thanks!

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

https://codereview.chromium.org/2963883002/diff/80001/components/omnibox/brow...
components/omnibox/browser/autocomplete_match_unittest.cc:139: for
(FormatUrlTestData& test_case : normal_cases) {
On 2017/06/29 19:54:47, Peter Kasting wrote:
> Nit: {} unnecessary (2 places)

Done.

[tommycli, 2017-06-29 20:28:19]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2017-06-29 20:28:19]

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

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

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1498768099280490,
"parent_rev": "7cd2d63b3831dc53449c8585f1e84b946c02e61a", "commit_rev":
"72014f6a8ab12897583185fc6c5b4752fcbaa278"}

[commit-bot: I haz the power, 2017-06-29 21:42:43]

Description was changed from

==========
Omnibox UI Experiments: Refactor HTTPS trimming into UrlFormatter.

Given that we must implement elide-after-host logic within UrlFormatter,
consolidate the other experimental URL trimming logic into the same
method.

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

to

==========
Omnibox UI Experiments: Refactor HTTPS trimming into UrlFormatter.

Given that we must implement elide-after-host logic within UrlFormatter,
consolidate the other experimental URL trimming logic into the same
method.

BUG=732582

Review-Url: https://codereview.chromium.org/2963883002
Cr-Commit-Position: refs/heads/master@{#483497}
Committed:
https://chromium.googlesource.com/chromium/src/+/72014f6a8ab12897583185fc6c5b...
==========

[commit-bot: I haz the power, 2017-06-29 21:42:45]

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