Use characters (not code units) when computing value list positions

Use characters (not code units) when computing value list positions

The value list position is updated by one for each character, and not
at all when spaces are skipped (collapsed). When assigning value list
positions, we are currently counting surrogates as two (on for each
code unit.)
Use the text metrics data to count the number of (non-collapsed)
characters instead.

BUG=597312, 594058
Committed: https://crrev.com/9331211c22aee7b499131aa1c58ce515deceeb45
Cr-Commit-Position: refs/heads/master@{#386305}

[fs, 2016-04-06 18:00:49]

fs@opera.com changed reviewers:
+ pdr@chromium.org

[fs, 2016-04-06 18:00:50]

After some shaving of large furry animal, the time has come to revisit this
issue.

[pdr., 2016-04-06 18:10:57]

LGTM with some optional grumbles about our font loading.

https://codereview.chromium.org/1866703002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/svg/text/surrogate-pair-attribute-positions.html
(right):

https://codereview.chromium.org/1866703002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/svg/text/surrogate-pair-attribute-positions.html:11:
document.body.offsetLeft; // Kick off loading @font-face
Is the test really flaky without this? I know we had this issue with Ahem, but
there have been some loading changes lately and I wonder if it's still needed.

Is there any way we can generalize this in a helper js file?

[fs, 2016-04-06 18:21:14]

https://codereview.chromium.org/1866703002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/svg/text/surrogate-pair-attribute-positions.html
(right):

https://codereview.chromium.org/1866703002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/svg/text/surrogate-pair-attribute-positions.html:11:
document.body.offsetLeft; // Kick off loading @font-face
On 2016/04/06 at 18:10:57, pdr wrote:
> Is the test really flaky without this? I know we had this issue with Ahem, but
there have been some loading changes lately and I wonder if it's still needed.
> 
> Is there any way we can generalize this in a helper js file?

I guess this was a "better safe than sorry" from my side (but these issue never
really showed up on my dev. platform before either...) I guess we can take it
out and see what happens.

I was pondering something like:

<script src="font-loader.js" data-family="MyFont"
data-font-src="...otf"></script>

and use the Font Loading API. But it did feel like overkill for a single test.
(Don't know how many others there are that might be interested in something like
it.)

[pdr., 2016-04-06 22:47:03]

On 2016/04/06 at 18:21:14, fs wrote:
>
https://codereview.chromium.org/1866703002/diff/1/third_party/WebKit/LayoutTe...
> File
third_party/WebKit/LayoutTests/svg/text/surrogate-pair-attribute-positions.html
(right):
> 
>
https://codereview.chromium.org/1866703002/diff/1/third_party/WebKit/LayoutTe...
>
third_party/WebKit/LayoutTests/svg/text/surrogate-pair-attribute-positions.html:11:
document.body.offsetLeft; // Kick off loading @font-face
> On 2016/04/06 at 18:10:57, pdr wrote:
> > Is the test really flaky without this? I know we had this issue with Ahem,
but there have been some loading changes lately and I wonder if it's still
needed.
> > 
> > Is there any way we can generalize this in a helper js file?
> 
> I guess this was a "better safe than sorry" from my side (but these issue
never really showed up on my dev. platform before either...) I guess we can take
it out and see what happens.
> 
> I was pondering something like:
> 
> <script src="font-loader.js" data-family="MyFont"
data-font-src="...otf"></script>
> 
> and use the Font Loading API. But it did feel like overkill for a single test.
(Don't know how many others there are that might be interested in something like
it.)

Do you remember if the font issue was specific to svg files?

Other tests that use CSSFWOrientationTest aren't flaky without the layout, so
I'd be okay with just removing that and committing (keeping an keeping an eye on
the flakiness dashboard for any flakiness).

[fs, 2016-04-07 08:45:21]

On 2016/04/06 at 22:47:03, pdr wrote:
> On 2016/04/06 at 18:21:14, fs wrote:
> >
https://codereview.chromium.org/1866703002/diff/1/third_party/WebKit/LayoutTe...
> > File
third_party/WebKit/LayoutTests/svg/text/surrogate-pair-attribute-positions.html
(right):
> > 
> >
https://codereview.chromium.org/1866703002/diff/1/third_party/WebKit/LayoutTe...
> >
third_party/WebKit/LayoutTests/svg/text/surrogate-pair-attribute-positions.html:11:
document.body.offsetLeft; // Kick off loading @font-face
> > On 2016/04/06 at 18:10:57, pdr wrote:
> > > Is the test really flaky without this? I know we had this issue with Ahem,
but there have been some loading changes lately and I wonder if it's still
needed.
> > > 
> > > Is there any way we can generalize this in a helper js file?
> > 
> > I guess this was a "better safe than sorry" from my side (but these issue
never really showed up on my dev. platform before either...) I guess we can take
it out and see what happens.
> > 
> > I was pondering something like:
> > 
> > <script src="font-loader.js" data-family="MyFont"
data-font-src="...otf"></script>
> > 
> > and use the Font Loading API. But it did feel like overkill for a single
test. (Don't know how many others there are that might be interested in
something like it.)
> 
> Do you remember if the font issue was specific to svg files?

I don't think so. It's possible it was specific to Ahem though (all flakes I saw
used Ahem, although that was just a coincident surely...)

> Other tests that use CSSFWOrientationTest aren't flaky without the layout, so
I'd be okay with just removing that and committing (keeping an keeping an eye on
the flakiness dashboard for any flakiness).

I've dropped the offsetLeft bits - and actually the entire <script>, because it
didn't seem useful at all...

[pdr., 2016-04-07 18:29:57]

On 2016/04/07 at 08:45:21, fs wrote:
> On 2016/04/06 at 22:47:03, pdr wrote:
> > On 2016/04/06 at 18:21:14, fs wrote:
> > >
https://codereview.chromium.org/1866703002/diff/1/third_party/WebKit/LayoutTe...
> > > File
third_party/WebKit/LayoutTests/svg/text/surrogate-pair-attribute-positions.html
(right):
> > > 
> > >
https://codereview.chromium.org/1866703002/diff/1/third_party/WebKit/LayoutTe...
> > >
third_party/WebKit/LayoutTests/svg/text/surrogate-pair-attribute-positions.html:11:
document.body.offsetLeft; // Kick off loading @font-face
> > > On 2016/04/06 at 18:10:57, pdr wrote:
> > > > Is the test really flaky without this? I know we had this issue with
Ahem, but there have been some loading changes lately and I wonder if it's still
needed.
> > > > 
> > > > Is there any way we can generalize this in a helper js file?
> > > 
> > > I guess this was a "better safe than sorry" from my side (but these issue
never really showed up on my dev. platform before either...) I guess we can take
it out and see what happens.
> > > 
> > > I was pondering something like:
> > > 
> > > <script src="font-loader.js" data-family="MyFont"
data-font-src="...otf"></script>
> > > 
> > > and use the Font Loading API. But it did feel like overkill for a single
test. (Don't know how many others there are that might be interested in
something like it.)
> > 
> > Do you remember if the font issue was specific to svg files?
> 
> I don't think so. It's possible it was specific to Ahem though (all flakes I
saw used Ahem, although that was just a coincident surely...)
> 
> > Other tests that use CSSFWOrientationTest aren't flaky without the layout,
so I'd be okay with just removing that and committing (keeping an keeping an eye
on the flakiness dashboard for any flakiness).
> 
> I've dropped the offsetLeft bits - and actually the entire <script>, because
it didn't seem useful at all...

LGTM

[fs, 2016-04-09 18:12:25]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2016-04-09 18:12:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1866703002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1866703002/40001

[commit-bot: I haz the power, 2016-04-09 19:13:53]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-09 19:15:38]

Description was changed from

==========
Use characters (not code units) when computing value list positions

The value list position is updated by one for each character, and not
at all when spaces are skipped (collapsed). When assigning value list
positions, we are currently counting surrogates as two (on for each
code unit.)
Use the text metrics data to count the number of (non-collapsed)
characters instead.

BUG=597312, 594058
==========

to

==========
Use characters (not code units) when computing value list positions

The value list position is updated by one for each character, and not
at all when spaces are skipped (collapsed). When assigning value list
positions, we are currently counting surrogates as two (on for each
code unit.)
Use the text metrics data to count the number of (non-collapsed)
characters instead.

BUG=597312, 594058

Committed: https://crrev.com/9331211c22aee7b499131aa1c58ce515deceeb45
Cr-Commit-Position: refs/heads/master@{#386305}
==========

[commit-bot: I haz the power, 2016-04-09 19:15:39]

Patchset 3 (id:??) landed as
https://crrev.com/9331211c22aee7b499131aa1c58ce515deceeb45
Cr-Commit-Position: refs/heads/master@{#386305}