Use LayoutUnit for positioning ellipses

Use LayoutUnit for positioning ellipses

Just remove a bunch of toInt()'s from the code that positions
ellipses.

BUG=693905, 693935
Review-Url: https://codereview.chromium.org/2702043002
Cr-Commit-Position: refs/heads/master@{#451951}
Committed: https://chromium.googlesource.com/chromium/src/+/0ba2c64035b4ff978e61e2af1943b29f5a48fca3

[rhogan, 2017-02-18 18:43:02]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-18 18:43:12]

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-02-18 19:46:43]

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

[commit-bot: I haz the power, 2017-02-18 19:46:44]

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_...)

[rhogan, 2017-02-18 20:16:58]

robhogan@gmail.com changed reviewers:
+ eae@chromium.org

[rhogan, 2017-02-19 11:48:57]

Description was changed from

==========
Use LayoutUnit for positioning ellipses

BUG=693905
==========

to

==========
Use LayoutUnit for positioning ellipses

BUG=693905, 693935
==========

[rhogan, 2017-02-19 18:56:28]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-19 18:56:33]

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-02-19 20:15:34]

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

[commit-bot: I haz the power, 2017-02-19 20:15:35]

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

[rhogan, 2017-02-20 19:26:56]

robhogan@gmail.com changed reviewers:
+ drott@chromium.org, kojii@chromium.org

[rhogan, 2017-02-20 19:27:34]

Description was changed from

==========
Use LayoutUnit for positioning ellipses

BUG=693905, 693935
==========

to

==========
Use LayoutUnit for positioning ellipses

Just remove a bunch of toInt()'s from the code that positions
ellipses.

BUG=693905, 693935
==========

[kojii, 2017-02-21 07:04:18]

Thank you for working on this. The code looks good to me, the test results look
mixed; like fast/css/text-overflow-ellipsis-text-align-center.html on Mac looks
worse. Is it going to be better in the dependent patch?

[rhogan, 2017-02-21 18:35:56]

On 2017/02/21 at 07:04:18, kojii wrote:
> Thank you for working on this. The code looks good to me, the test results
look mixed; like fast/css/text-overflow-ellipsis-text-align-center.html on Mac
looks worse. Is it going to be better in the dependent patch?

The dependent patch (https://codereview.chromium.org/2699393002) doesn't resolve
it. Looks like a separate issue, which I should possibly address with this patch
or separately where we're not aggressive enough in truncating text to ensure
that the ellipsis fits comfortably.

[eae, 2017-02-21 19:06:19]

This is a step in the right direction so LGTM even though some of the tests look
slightly worse (but most look the same or better).

[rhogan, 2017-02-21 19:47:50]

On 2017/02/21 at 07:04:18, kojii wrote:
> Thank you for working on this. The code looks good to me, the test results
look mixed; like fast/css/text-overflow-ellipsis-text-align-center.html on Mac
looks worse. Is it going to be better in the dependent patch?

kojii - there seems to be something platform-specific going on here between
Linux (which hasn't changed its results at all), Mac (which I think shows the
most disimprovement in some cases, especially the RTL case), and Windows (which
I think shows less disimprovement than Mac and shows more improvements in the
RTL case, e.g.
https://storage.googleapis.com/chromium-layout-test-archives/win_chromium_rel...).

It looks like a 'partial glyph' thing potentially - but I don't see that
plumbing through to the platform-specific font stuff. Any thoughts?

(I'm going to take eae's lgtm and run with it, there's clearly room for a
follow-on here).

[rhogan, 2017-02-21 21:22:55]

The CQ bit was checked by robhogan@gmail.com

[rhogan, 2017-02-21 21:22:56]

The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2702043002/#ps40001
(title: "bug 693905-2")

[commit-bot: I haz the power, 2017-02-21 21:24:17]

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-02-21 23:28:33]

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

[commit-bot: I haz the power, 2017-02-21 23:28:36]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[kojii, 2017-02-22 02:49:18]

Following patch is fine, I think this is in good direction, thank you again for
the great work as always.

[rhogan, 2017-02-22 07:24:01]

The CQ bit was checked by robhogan@gmail.com

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

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-02-22 09:16:12]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487748241326600,
"parent_rev": "44b7e0d7c92d708fdc64cb2357bbb2a712551b94", "commit_rev":
"0ba2c64035b4ff978e61e2af1943b29f5a48fca3"}

[commit-bot: I haz the power, 2017-02-22 09:16:52]

Description was changed from

==========
Use LayoutUnit for positioning ellipses

Just remove a bunch of toInt()'s from the code that positions
ellipses.

BUG=693905, 693935
==========

to

==========
Use LayoutUnit for positioning ellipses

Just remove a bunch of toInt()'s from the code that positions
ellipses.

BUG=693905, 693935

Review-Url: https://codereview.chromium.org/2702043002
Cr-Commit-Position: refs/heads/master@{#451951}
Committed:
https://chromium.googlesource.com/chromium/src/+/0ba2c64035b4ff978e61e2af1943...
==========

[commit-bot: I haz the power, 2017-02-22 09:16:53]

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