Fix underline position and thickness calculation.

Fix underline position and thickness calculation.

This change is dependent on https://codereview.chromium.org/271333002 ,
and should not be committed until Blink has that change.
This change will require ~600 rebaselines on each platform.

[bungeman-chromium, 2014-05-10 00:00:07]

See https://codereview.chromium.org/280003002/ for what this will look like when
finished.

The steps here are:

1. Put SK_IGNORE_UNDERLINE_POSITION_FIX in blink_skia_config.gypi. (Done with
blink r173791.)

2. Roll Blink into Chromium (past r173791).

3. Commit the Skia changes at https://codereview.chromium.org/271333002/ .

4. Roll these Skia changes into Chromium.

5. Commit the changes here.

6. Rebaseline.

[bungeman-chromium, 2014-05-10 00:03:57]

Adding reed as he is an owner of the blink_skia_config.gypi file.

[eae, 2014-05-12 08:19:32]

On 2014/05/10 00:03:57, bungeman2 wrote:
> Adding reed as he is an owner of the blink_skia_config.gypi file.

I'll land this together with the new baselines as soon as the skia change is in.

[reed1, 2014-05-12 12:34:11]

lgtm

https://codereview.chromium.org/275333002/diff/1/Source/platform/fonts/skia/S...
File Source/platform/fonts/skia/SimpleFontDataSkia.cpp (right):

https://codereview.chromium.org/275333002/diff/1/Source/platform/fonts/skia/S...
Source/platform/fonts/skia/SimpleFontDataSkia.cpp:136: &&
metrics.hasUnderlinePosition(&underlinePosition)) {
nit: not sure you need the LF

[bungeman-chromium, 2014-05-16 01:53:31]

On 2014/05/12 08:19:32, eae wrote:
> On 2014/05/10 00:03:57, bungeman2 wrote:
> > Adding reed as he is an owner of the blink_skia_config.gypi file.
> 
> I'll land this together with the new baselines as soon as the skia change is
in.

The skia change is in, eae, feel free to commit and rebaseline (either this or
something like it).

[eae, 2014-05-16 08:59:02]

On 2014/05/16 01:53:31, bungeman2 wrote:
> On 2014/05/12 08:19:32, eae wrote:
> > On 2014/05/10 00:03:57, bungeman2 wrote:
> > > Adding reed as he is an owner of the blink_skia_config.gypi file.
> > 
> > I'll land this together with the new baselines as soon as the skia change is
> in.
> 
> The skia change is in, eae, feel free to commit and rebaseline (either this or
> something like it).

Awesome, will do.

[eae, 2014-05-16 11:27:56]

While the underline thickness improves with this patch the position seems off
and for many tests the position shifts throughout a line.

See
https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/...
It is especially noticeable for the first few tests, like this one
https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/...

[bungeman-chromium, 2014-05-17 03:03:51]

On 2014/05/16 11:27:56, eae wrote:
> While the underline thickness improves with this patch the position seems off
> and for many tests the position shifts throughout a line.
> 
> See
>
https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/...
> It is especially noticeable for the first few tests, like this one
>
https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/...

This is expected for this change. Different fonts put their underlines in
different places. This is actually already an issue (as described in the
original bug) except that we're currently even doing that wrong (what this is
trying to fix). The longer term solution is probably to do the same thing which
is done to create the baseline. That is, find all the underline offsets, pick
the largest, then use that as the offset from the computed baseline to the top
of the underline, but use the underline thickness of each font for its part of
the run. Or something like that. Not sure I'm the one to write such a thing, but
if you wanted to write that on top of this (so we only rebaseline once) I have
no problem with that.

[Stephen Chennney, 2014-05-19 14:56:40]

On 2014/05/17 03:03:51, bungeman2 wrote:
> On 2014/05/16 11:27:56, eae wrote:
> > While the underline thickness improves with this patch the position seems
off
> > and for many tests the position shifts throughout a line.
> > 
> > See
> >
>
https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/...
> > It is especially noticeable for the first few tests, like this one
> >
>
https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/...
> 
> This is expected for this change. Different fonts put their underlines in
> different places. This is actually already an issue (as described in the
> original bug) except that we're currently even doing that wrong (what this is
> trying to fix). The longer term solution is probably to do the same thing
which
> is done to create the baseline. That is, find all the underline offsets, pick
> the largest, then use that as the offset from the computed baseline to the top
> of the underline, but use the underline thickness of each font for its part of
> the run. Or something like that. Not sure I'm the one to write such a thing,
but
> if you wanted to write that on top of this (so we only rebaseline once) I have
> no problem with that.

Note this patch, https://codereview.chromium.org/203273003/, which has been
pending for ages, fixes (or tries to) the different fonts putting underlines in
different places problem, and also unifies font widths. Actually, maybe it
doesn't fix the positions problem, but it does fix the thickness.

[h.joshi, 2014-05-20 07:16:58]

I feel comment "find all the underline offsets, pick the largest, then use that 
as the offset from the computed baseline to the top of the underline." 
is correct, in Blink we need parsing of offset data and get correct offset to
draw Underline.

If no is working on this, I can start on the above part to compute offset and
get best
possible offset.

[bungeman-skia, 2014-05-20 23:21:11]

Closing in favor of https://codereview.chromium.org/295923006/ which will also
handle the rebaselines.