Omnibox UI Experiments: Implement elide-after-host experiment.

Omnibox UI Experiments: Implement elide-after-host experiment.

This implements the logic to do the elide-after-host experiment. This
isn't hooked up to the feature flag yet (which will be in a followup).

This CL elides everything after the host, including the path, query, and
ref.

BUG=732582
Review-Url: https://codereview.chromium.org/2961093002
Cr-Commit-Position: refs/heads/master@{#483179}
Committed: https://chromium.googlesource.com/chromium/src/+/94d50d2defeafa336cd6574f43dccbbd69306480

[tommycli, 2017-06-28 18:31:50]

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

[commit-bot: I haz the power, 2017-06-28 18:32:14]

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-28 18:34:41]

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

[tommycli, 2017-06-28 18:34:41]

pkasting: PTAL. This is the path elision technique that we discussed a few days
ago.

[commit-bot: I haz the power, 2017-06-28 18:37:05]

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

[commit-bot: I haz the power, 2017-06-28 18:37:06]

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

[tommycli, 2017-06-28 18:42:34]

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

[commit-bot: I haz the power, 2017-06-28 18:43:08]

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-28 19:43:48]

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

[commit-bot: I haz the power, 2017-06-28 19:43:49]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Peter Kasting, 2017-06-28 20:54:35]

LGTM, only real significant comment is on naming.

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

https://codereview.chromium.org/2961093002/diff/1/components/url_formatter/ur...
components/url_formatter/url_formatter.cc:513: // Case when it's just a single
trailing slash we can strip.
Nit: "Omit the path, which is a single trailing slash.  There is no query or
ref."

https://codereview.chromium.org/2961093002/diff/1/components/url_formatter/ur...
components/url_formatter/url_formatter.cc:521: // Case when we replace the path
with an ellipsis.
Nit: "Replace everything after the host with a slash + ellipsis."

https://codereview.chromium.org/2961093002/diff/1/components/url_formatter/ur...
components/url_formatter/url_formatter.cc:527: // New length is one to account
for the new ellipsis character.
Nit: These comments actually refer, respectively, to line 534, 528, and 534.  I
had a really hard time understanding this reading them.  How about spreading
them out?  I also tried to explain why I thought you added "+1"s on the
query/ref, correct me if I'm wrong.

    // Compute the length of everything we're replacing.  For the path, we're
    // removing all but the first slash.
    size_t old_length = parsed.path.len - 1;

    // We're also removing any query, plus the delimiting '?'.
    if (parsed.query.is_valid())
      old_length += parsed.query.len + 1;

    // And any ref, plus delimiting '#'.
    if (parsed.ref.is_valid())
      old_length += parsed.ref.len + 1;

    // We're replacing all these with a single character (an ellipsis).
    adjustments->push_back(
        base::OffsetAdjuster::Adjustment(parsed.path.begin + 1, old_length, 1));

https://codereview.chromium.org/2961093002/diff/1/components/url_formatter/ur...
components/url_formatter/url_formatter.cc:536: // Default case.
Nit: "Append formatted path, query, and ref."

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

https://codereview.chromium.org/2961093002/diff/1/components/url_formatter/ur...
components/url_formatter/url_formatter.h:58: // Replaces paths with an ellipsis.
Experimental and not in kFormatUrlOmitAll.
As implemented, this actually replaces the path, query, and ref.

I might call it ElideAfterHost instead of EllipsizePath.

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

https://codereview.chromium.org/2961093002/diff/1/components/url_formatter/ur...
components/url_formatter/url_formatter_unittest.cc:1259: 5, 6,     7,     8,    
kNpos, kNpos, kNpos, kNpos, kNpos, kNpos, 9};
Nit: Is this spacing git cl format's fault?  It's really strange.

[tommycli, 2017-06-28 21:13:56]

Description was changed from

==========
Omnibox UI Experiments: Implement path elision into ellipsis logic.

This implements the logic to do the path elision and tests. This
isn't hooked up to the feature flag yet (which will be in a followup).

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

to

==========
Omnibox UI Experiments: Implement elide-after-host experiment.

This implements the logic to do the elide-after-host experiment. This
isn't hooked up to the feature flag yet (which will be in a followup).

This CL elides everything after the host, including the path, query, and
ref.

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

[tommycli, 2017-06-28 21:21:40]

Thanks!

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

https://codereview.chromium.org/2961093002/diff/1/components/url_formatter/ur...
components/url_formatter/url_formatter.cc:513: // Case when it's just a single
trailing slash we can strip.
On 2017/06/28 20:54:34, Peter Kasting wrote:
> Nit: "Omit the path, which is a single trailing slash.  There is no query or
> ref."

Done.

https://codereview.chromium.org/2961093002/diff/1/components/url_formatter/ur...
components/url_formatter/url_formatter.cc:521: // Case when we replace the path
with an ellipsis.
On 2017/06/28 20:54:34, Peter Kasting wrote:
> Nit: "Replace everything after the host with a slash + ellipsis."

Done.

https://codereview.chromium.org/2961093002/diff/1/components/url_formatter/ur...
components/url_formatter/url_formatter.cc:527: // New length is one to account
for the new ellipsis character.
On 2017/06/28 20:54:34, Peter Kasting wrote:
> Nit: These comments actually refer, respectively, to line 534, 528, and 534. 
I
> had a really hard time understanding this reading them.  How about spreading
> them out?  I also tried to explain why I thought you added "+1"s on the
> query/ref, correct me if I'm wrong.
> 
>     // Compute the length of everything we're replacing.  For the path, we're
>     // removing all but the first slash.
>     size_t old_length = parsed.path.len - 1;
> 
>     // We're also removing any query, plus the delimiting '?'.
>     if (parsed.query.is_valid())
>       old_length += parsed.query.len + 1;
> 
>     // And any ref, plus delimiting '#'.
>     if (parsed.ref.is_valid())
>       old_length += parsed.ref.len + 1;
> 
>     // We're replacing all these with a single character (an ellipsis).
>     adjustments->push_back(
>         base::OffsetAdjuster::Adjustment(parsed.path.begin + 1, old_length,
1));

Done. This makes it make much more sense!

https://codereview.chromium.org/2961093002/diff/1/components/url_formatter/ur...
components/url_formatter/url_formatter.cc:536: // Default case.
On 2017/06/28 20:54:34, Peter Kasting wrote:
> Nit: "Append formatted path, query, and ref."

Done.

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

https://codereview.chromium.org/2961093002/diff/1/components/url_formatter/ur...
components/url_formatter/url_formatter.h:58: // Replaces paths with an ellipsis.
Experimental and not in kFormatUrlOmitAll.
On 2017/06/28 20:54:35, Peter Kasting wrote:
> As implemented, this actually replaces the path, query, and ref.
> 
> I might call it ElideAfterHost instead of EllipsizePath.

Done.

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

https://codereview.chromium.org/2961093002/diff/1/components/url_formatter/ur...
components/url_formatter/url_formatter_unittest.cc:1259: 5, 6,     7,     8,    
kNpos, kNpos, kNpos, kNpos, kNpos, kNpos, 9};
On 2017/06/28 20:54:35, Peter Kasting wrote:
> Nit: Is this spacing git cl format's fault?  It's really strange.

Yes, it's from git cl format. I didn't question it. :)

https://codereview.chromium.org/2961093002/diff/20001/components/url_formatte...
File components/url_formatter/url_formatter.cc (right):

https://codereview.chromium.org/2961093002/diff/20001/components/url_formatte...
components/url_formatter/url_formatter.cc:523:
url_string.append(kEllipsisUTF16);
As a change from PS1, I also added a copy of kEllipsisUTF16 in here locally as a
constexpr instead of importing from gfx/ (which is disallowed by DEPs and seems
architecturally unsound).

[tommycli, 2017-06-28 21:22:27]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2017-06-28 21:22:28]

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/2961093002/#ps40001
(title: "address pkasting comments")

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

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1498684947692520,
"parent_rev": "1ac7b378fc716a389d97d5cc10d42bbfd47cd1dd", "commit_rev":
"94d50d2defeafa336cd6574f43dccbbd69306480"}

[commit-bot: I haz the power, 2017-06-28 22:47:34]

Description was changed from

==========
Omnibox UI Experiments: Implement elide-after-host experiment.

This implements the logic to do the elide-after-host experiment. This
isn't hooked up to the feature flag yet (which will be in a followup).

This CL elides everything after the host, including the path, query, and
ref.

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

to

==========
Omnibox UI Experiments: Implement elide-after-host experiment.

This implements the logic to do the elide-after-host experiment. This
isn't hooked up to the feature flag yet (which will be in a followup).

This CL elides everything after the host, including the path, query, and
ref.

BUG=732582

Review-Url: https://codereview.chromium.org/2961093002
Cr-Commit-Position: refs/heads/master@{#483179}
Committed:
https://chromium.googlesource.com/chromium/src/+/94d50d2defeafa336cd6574f43dc...
==========

[commit-bot: I haz the power, 2017-06-28 22:47:35]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/94d50d2defeafa336cd6574f43dc...