Fix gap for "text-underline-position:under"

Fix gap for "text-underline-position:under"

This patch fixes the gap for "under" to match to the spec and to the use
cases.

Unlike Roman baseline, "under" position is at the bottom of em-box. It
should not be offset by the gap.

To avoid computing the gap for "under", roman baseline is extracted to a
separate function |computeUnderlineOffsetForRoman|.

BUG=677240
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2643293004
Cr-Commit-Position: refs/heads/master@{#447345}
Committed: https://chromium.googlesource.com/chromium/src/+/eb6c3ce7ce15059e6411b8b018df85b2dbf0e18d

[kojii, 2017-01-21 11:00:57]

Description was changed from

==========
ul-gap

BUG=
==========

to

==========
ul-gap

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[kojii, 2017-01-21 11:01:06]

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

[commit-bot: I haz the power, 2017-01-21 11:01:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kojii, 2017-01-21 11:13:29]

Description was changed from

==========
ul-gap

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Fix gap for "text-underline-position:under"

This patch fixes the gap for "under" to match to the spec nor to the use
cases.

Unlike Roman baseline, "under" position is at the bottom of em-box. It
should not be offset by the gap.

BUG=677240
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[kojii, 2017-01-21 11:14:01]

kojii@chromium.org changed reviewers:
+ drott@chromium.org

[kojii, 2017-01-21 11:14:02]

PTAL. Split the "gap" fix.

[kojii, 2017-01-21 11:18:58]

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

[commit-bot: I haz the power, 2017-01-21 11:19:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kojii, 2017-01-21 11:19:48]

Description was changed from

==========
Fix gap for "text-underline-position:under"

This patch fixes the gap for "under" to match to the spec nor to the use
cases.

Unlike Roman baseline, "under" position is at the bottom of em-box. It
should not be offset by the gap.

BUG=677240
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Fix gap for "text-underline-position:under"

This patch fixes the gap for "under" to match to the spec nor to the use
cases.

Unlike Roman baseline, "under" position is at the bottom of em-box. It
should not be offset by the gap.

To avoid computing the gap for "under", roman baseline is extracted to a
separate function |computeUnderlineOffsetForRoman|.

BUG=677240
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[commit-bot: I haz the power, 2017-01-21 12:38:10]

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

[commit-bot: I haz the power, 2017-01-21 12:38:10]

Dry run: This issue passed the CQ dry run.

[kojii, 2017-01-21 20:20:53]

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

[commit-bot: I haz the power, 2017-01-21 20:21:06]

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-01-21 21:39:02]

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

[commit-bot: I haz the power, 2017-01-21 21:39:03]

Dry run: This issue passed the CQ dry run.

[drott, 2017-01-23 08:20:23]

Mostly looks good, one question regarding alternative values, and could you
please clarify/elaborate on this part of the commit msg:

"This patch fixes the gap for "under" to match to the spec nor to the use
cases."

What do you meany by "to match to the spec nor to the use
cases." Do you mean "to neither match the spec nor the use cases?"

Thanks!

https://codereview.chromium.org/2643293004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp (right):

https://codereview.chromium.org/2643293004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp:108: default:
Do we parse other values than Auto and Under in CSSPropertyParser?

[kojii, 2017-01-23 09:02:55]

Description was changed from

==========
Fix gap for "text-underline-position:under"

This patch fixes the gap for "under" to match to the spec nor to the use
cases.

Unlike Roman baseline, "under" position is at the bottom of em-box. It
should not be offset by the gap.

To avoid computing the gap for "under", roman baseline is extracted to a
separate function |computeUnderlineOffsetForRoman|.

BUG=677240
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Fix gap for "text-underline-position:under"

This patch fixes the gap for "under" to match to the spec and to the use
cases.

Unlike Roman baseline, "under" position is at the bottom of em-box. It
should not be offset by the gap.

To avoid computing the gap for "under", roman baseline is extracted to a
separate function |computeUnderlineOffsetForRoman|.

BUG=677240
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[kojii, 2017-01-23 09:04:46]

On 2017/01/23 at 08:20:23, drott wrote:
> Mostly looks good, one question regarding alternative values, and could you
please clarify/elaborate on this part of the commit msg:
> 
> "This patch fixes the gap for "under" to match to the spec nor to the use
> cases."
> 
> What do you meany by "to match to the spec nor to the use
> cases." Do you mean "to neither match the spec nor the use cases?"

s/nor/and/, spec has some ambiguities, filled in UA-defined behavior from the
use case. Does this make sense?

>
https://codereview.chromium.org/2643293004/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp (right):
> 
>
https://codereview.chromium.org/2643293004/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp:108: default:
> Do we parse other values than Auto and Under in CSSPropertyParser?

I can't add them yet, there are still open issues in github. I'll work on it
once WG solves issues and other impls reach consensus.

[drott, 2017-01-23 10:29:42]

On 2017/01/23 at 09:04:46, kojii wrote:
> On 2017/01/23 at 08:20:23, drott wrote:
> > Mostly looks good, one question regarding alternative values, and could you
please clarify/elaborate on this part of the commit msg:
> > 
> > "This patch fixes the gap for "under" to match to the spec nor to the use
> > cases."
> > 
> > What do you meany by "to match to the spec nor to the use
> > cases." Do you mean "to neither match the spec nor the use cases?"
> 
> s/nor/and/, spec has some ambiguities, filled in UA-defined behavior from the
use case. Does this make sense?

It makes more sense, although now I don't understand what you mean by: "filled
in UA-defined behavior from the use case".

> >
https://codereview.chromium.org/2643293004/diff/40001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp (right):
> > 
> >
https://codereview.chromium.org/2643293004/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp:108: default:
> > Do we parse other values than Auto and Under in CSSPropertyParser?
> 
> I can't add them yet, there are still open issues in github. I'll work on it
once WG solves issues and other impls reach consensus.

No rush with that, the direction of my question was mainly that only if we parse
them, we could get to the NOTREACHED() here. So I am assuming you meant: No, we
don't parse them? If that's the case: LGTM. Otherwise, they should either be
removed from parsing, or the NOTREACHED should be softened into a default: break
or so.

Thanks for the clarification - as mentioned, LGTM.

[kojii, 2017-01-23 10:35:12]

On 2017/01/23 at 10:29:42, drott wrote:
> On 2017/01/23 at 09:04:46, kojii wrote:
> > On 2017/01/23 at 08:20:23, drott wrote:
> > > Mostly looks good, one question regarding alternative values, and could
you please clarify/elaborate on this part of the commit msg:
> > > 
> > > "This patch fixes the gap for "under" to match to the spec nor to the use
> > > cases."
> > > 
> > > What do you meany by "to match to the spec nor to the use
> > > cases." Do you mean "to neither match the spec nor the use cases?"
> > 
> > s/nor/and/, spec has some ambiguities, filled in UA-defined behavior from
the use case. Does this make sense?
> 
> It makes more sense, although now I don't understand what you mean by: "filled
in UA-defined behavior from the use case".

The spec defines "under the element’s text content", and doesn't define whether
to add a gap or not, for both "under" and "auto". For CJK use case, gap is
undesired, so removing gap matches to the use case better. Sorry my word choice
often looks not good ;(

> 
> > >
https://codereview.chromium.org/2643293004/diff/40001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp
(right):
> > > 
> > >
https://codereview.chromium.org/2643293004/diff/40001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp:108:
default:
> > > Do we parse other values than Auto and Under in CSSPropertyParser?
> > 
> > I can't add them yet, there are still open issues in github. I'll work on it
once WG solves issues and other impls reach consensus.
> 
> No rush with that, the direction of my question was mainly that only if we
parse them, we could get to the NOTREACHED() here. So I am assuming you meant:
No, we don't parse them? If that's the case: LGTM. Otherwise, they should either
be removed from parsing, or the NOTREACHED should be softened into a default:
break or so.

Yes, that's the case. Syntax is also on the discussion, so I'd like not to parse
until the discussion is settled down.

[kojii, 2017-01-23 10:35:28]

kojii@chromium.org changed reviewers:
+ eae@chromium.org

[kojii, 2017-01-23 10:35:28]

eae@, PTAL.

[drott, 2017-01-23 10:39:13]

On 2017/01/23 at 10:35:12, kojii wrote:
> On 2017/01/23 at 10:29:42, drott wrote:
> > On 2017/01/23 at 09:04:46, kojii wrote:
> > > On 2017/01/23 at 08:20:23, drott wrote:
> > > > Mostly looks good, one question regarding alternative values, and could
you please clarify/elaborate on this part of the commit msg:
> > > > 
> > > > "This patch fixes the gap for "under" to match to the spec nor to the
use
> > > > cases."
> > > > 
> > > > What do you meany by "to match to the spec nor to the use
> > > > cases." Do you mean "to neither match the spec nor the use cases?"
> > > 
> > > s/nor/and/, spec has some ambiguities, filled in UA-defined behavior from
the use case. Does this make sense?
> > 
> > It makes more sense, although now I don't understand what you mean by:
"filled in UA-defined behavior from the use case".
> 
> The spec defines "under the element’s text content", and doesn't define
whether to add a gap or not, for both "under" and "auto". For CJK use case, gap
is undesired, so removing gap matches to the use case better. Sorry my word
choice often looks not good ;(


Thanks, got it now, I just needed a bit more background to understand.

[kojii, 2017-01-23 13:21:34]

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

[commit-bot: I haz the power, 2017-01-23 13:21:44]

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-01-23 13:23:06]

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

[commit-bot: I haz the power, 2017-01-23 13:23:07]

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

[kojii, 2017-01-24 02:23:53]

eae@, PTAL (sorry many similar ones in a row)

[kojii, 2017-01-27 06:24:54]

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

[commit-bot: I haz the power, 2017-01-27 06:25:13]

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-01-27 08:27:16]

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

[commit-bot: I haz the power, 2017-01-27 08:27:17]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[eae, 2017-01-31 19:31:07]

LGTM!

[kojii, 2017-01-31 19:32:11]

The CQ bit was checked by kojii@chromium.org

[kojii, 2017-01-31 19:32:11]

The patchset sent to the CQ was uploaded after l-g-t-m from drott@chromium.org
Link to the patchset: https://codereview.chromium.org/2643293004/#ps60001
(title: "Resolved merge conflict")

[commit-bot: I haz the power, 2017-01-31 19:33:02]

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-01-31 22:19:47]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1485891131387990,
"parent_rev": "e4e4b0140002c58d760bf8d9f3fcfd7d29343486", "commit_rev":
"eb6c3ce7ce15059e6411b8b018df85b2dbf0e18d"}

[commit-bot: I haz the power, 2017-01-31 22:20:26]

Description was changed from

==========
Fix gap for "text-underline-position:under"

This patch fixes the gap for "under" to match to the spec and to the use
cases.

Unlike Roman baseline, "under" position is at the bottom of em-box. It
should not be offset by the gap.

To avoid computing the gap for "under", roman baseline is extracted to a
separate function |computeUnderlineOffsetForRoman|.

BUG=677240
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Fix gap for "text-underline-position:under"

This patch fixes the gap for "under" to match to the spec and to the use
cases.

Unlike Roman baseline, "under" position is at the bottom of em-box. It
should not be offset by the gap.

To avoid computing the gap for "under", roman baseline is extracted to a
separate function |computeUnderlineOffsetForRoman|.

BUG=677240
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2643293004
Cr-Commit-Position: refs/heads/master@{#447345}
Committed:
https://chromium.googlesource.com/chromium/src/+/eb6c3ce7ce15059e6411b8b018df...
==========

[commit-bot: I haz the power, 2017-01-31 22:20:28]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/eb6c3ce7ce15059e6411b8b018df...