Add support for multiple text decorations with same line positioning

Add support for multiple text decorations with same line positioning

Original patch by Sasha Morrissey, rebased and adapted.

Add support for multiple propagated text decorations with the same line
positioning, as defined in http://www.w3.org/TR/css-text-decor-3/.
Before this, elements with multiple text decorations but the same line
positioning (e.g. two underlines) would only paint the 'topmost' one.
With this patch, both lines are painted.

This is important for CSS Text Decorations Module 3, where two text
decorations can have the same line position but different styles (e.g.
wavy and solid), and they should be painted on top of eachother.

This minimally affects some tests where two text decorations with the
same line positioning were present but only one was being painted.

Also cleaned up the existing logic for text decorations to only inherit
through StyleAdjuster, removing the need to traverse up the tree to
resolve colors for inherited text decorations.

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

Committed: https://crrev.com/3ffa4763a6a442e26f1ea2a87ee2bf3f0aa26e1b
Cr-Commit-Position: refs/heads/master@{#433191}

[drott, 2016-11-14 15:46:05]

Description was changed from

==========
Add support for multiple text decorations with same line positioning

Original patch by Sasha Morrissey, rebased and adapted.

Add support for multiple propagated text decorations with the same line
positioning, as defined in http://www.w3.org/TR/css-text-decor-3/.
Before this, elements with multiple text decorations but the same line
positioning (e.g. two underlines) would only paint the 'topmost' one.
With this patch, both lines are painted.

This is important for CSS Text Decorations Module 3, where two text
decorations can have the same line position but different styles (e.g.
wavy and solid), and they should be painted on top of eachother.

This minimally affects some tests where two text decorations with the
same line positioning were present but only one was being painted.

Also cleaned up the existing logic for text decorations to only inherit
through StyleAdjuster, removing the need to traverse up the tree to
resolve colors for inherited text decorations.

BUG=462142
==========

to

==========
Add support for multiple text decorations with same line positioning

Original patch by Sasha Morrissey, rebased and adapted.

Add support for multiple propagated text decorations with the same line
positioning, as defined in http://www.w3.org/TR/css-text-decor-3/.
Before this, elements with multiple text decorations but the same line
positioning (e.g. two underlines) would only paint the 'topmost' one.
With this patch, both lines are painted.

This is important for CSS Text Decorations Module 3, where two text
decorations can have the same line position but different styles (e.g.
wavy and solid), and they should be painted on top of eachother.

This minimally affects some tests where two text decorations with the
same line positioning were present but only one was being painted.

Also cleaned up the existing logic for text decorations to only inherit
through StyleAdjuster, removing the need to traverse up the tree to
resolve colors for inherited text decorations.

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

[drott, 2016-11-14 15:47:28]

drott@chromium.org changed reviewers:
+ sashab@chromium.org, timloh@chromium.org

[drott, 2016-11-14 15:50:39]

Thanks, Sasha, your well-written code still lent itself quite well for rebasing
/ adapting. 

Unfortunately, this is not 100% ready yet. In the simple underline optimization
case, a text-decoration-color update does not trigger a repaint: 
fast/css3-text/css3-text-decoration/text-decoration-color-recalc.html
fails. A style or line update works fine in this test. The test also work if the
style is non-solid in the first place, so in all cases where the optimization is
disabled.

I am not sure why that is, I'll try to dig deeper tomorrow. Tim or Sasha, if you
have any pointers, that would be great.

[drott, 2016-11-14 15:51:26]

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

[commit-bot: I haz the power, 2016-11-14 15:51:41]

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, 2016-11-14 16:39:42]

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

[commit-bot: I haz the power, 2016-11-14 16:39:42]

Dry run: Try jobs failed on following builders:
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)

[drott, 2016-11-15 15:23:54]

Only use m_hasSimpleUnderline for currentColor, as before

[drott, 2016-11-15 15:27:38]

I modified the CL to only use the m_hasSimpleUnderline optimization for when the
underline is of currentColor. This matches the previous logic of
isSimpleUnderline() in AppliedDecoration. Without this modification,
text-decoration-color-recalc.html keeps failing, and I am having trouble
pointing out exactly where which invalidation is failing. Even if I force a
paint invalidation in this case, the underline is painted with the old color.

[drott, 2016-11-15 15:28:07]

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

[commit-bot: I haz the power, 2016-11-15 15:28:26]

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

[drott, 2016-11-15 15:52:35]

With that, I think it's ready for review.

[commit-bot: I haz the power, 2016-11-15 16:13:33]

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

[commit-bot: I haz the power, 2016-11-15 16:13:34]

Dry run: Try jobs failed on following builders:
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)

[drott, 2016-11-15 17:31:39]

fix leftover test modification

[drott, 2016-11-15 17:31:40]

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

[commit-bot: I haz the power, 2016-11-15 17:32:27]

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, 2016-11-15 18:35:31]

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

[commit-bot: I haz the power, 2016-11-15 18:35:32]

Dry run: Try jobs failed on following builders:
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)

[sashab, 2016-11-16 04:25:02]

Yup, this is looking great so far :) just a few more failing tests -- did you
manage to figure out what's causing that? Good job rebasing the patch, it looks
awesome. :)

Sasha

[drott, 2016-11-16 07:12:54]

On 2016/11/16 at 04:25:02, sashab wrote:
> Yup, this is looking great so far :) just a few more failing tests -- did you
manage to figure out what's causing that? Good job rebasing the patch, it looks
awesome. :)

Thanks - I confirmed that the failing test cases match your diagnosis:
"This minimally affects some tests where two text decorations with the
same line positioning were present but only one was being painted."
The SVG test have nested underline specified, and the mozilla table tests have
links with additional <u> tags, so double underlines as well, which now get
painted slightly stronger. I'll rebaseline those.

[drott, 2016-11-16 07:21:08]

Bot rebaselines added

[drott, 2016-11-16 07:21:09]

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

[commit-bot: I haz the power, 2016-11-16 07:21:36]

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, 2016-11-16 09:02:21]

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

[commit-bot: I haz the power, 2016-11-16 09:02:22]

Dry run: Try jobs failed on following builders:
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)

[drott, 2016-11-16 09:25:52]

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

[commit-bot: I haz the power, 2016-11-16 09:26:23]

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

[drott, 2016-11-16 12:12:22]

pdr@, I believe this slimming paint failure is not critical, because it's just a
scrollbar difference?

[commit-bot: I haz the power, 2016-11-16 12:29:01]

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

[commit-bot: I haz the power, 2016-11-16 12:29:02]

Dry run: Try jobs failed on following builders:
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)

[drott, 2016-11-16 12:44:29]

timloh@, if it looks okay to you, could you cq+ after your review? Thanks.

[drott, 2016-11-16 14:36:39]

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

[commit-bot: I haz the power, 2016-11-16 14:36:55]

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, 2016-11-16 17:10:59]

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

[commit-bot: I haz the power, 2016-11-16 17:11:00]

Dry run: Try jobs failed on following builders:
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)

[Timothy Loh, 2016-11-17 15:09:42]

lgtm

https://codereview.chromium.org/2497963002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp (left):

https://codereview.chromium.org/2497963002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp:991: if ((deco &
TextDecorationUnderline) && fontData) {
I don't think you meant to drop the fontData check

[drott, 2016-11-17 16:23:26]

The CQ bit was checked by drott@chromium.org

[drott, 2016-11-17 16:23:40]

The CQ bit was unchecked by drott@chromium.org

[drott, 2016-11-17 16:34:53]

Added back fontData check

[drott, 2016-11-17 16:35:02]

The CQ bit was checked by drott@chromium.org

[drott, 2016-11-17 16:35:03]

The patchset sent to the CQ was uploaded after l-g-t-m from timloh@chromium.org
Link to the patchset: https://codereview.chromium.org/2497963002/#ps100001
(title: "Added back fontData check")

[commit-bot: I haz the power, 2016-11-17 16:35:51]

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, 2016-11-17 19:34:43]

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

[commit-bot: I haz the power, 2016-11-17 19:34:45]

Try jobs failed on following builders:
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[drott, 2016-11-18 12:32:40]

The CQ bit was checked by drott@chromium.org

[drott, 2016-11-18 12:32:41]

The patchset sent to the CQ was uploaded after l-g-t-m from timloh@chromium.org
Link to the patchset: https://codereview.chromium.org/2497963002/#ps120001
(title: "Update slimming paint expectations")

[commit-bot: I haz the power, 2016-11-18 12:32:50]

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, 2016-11-18 14:27:43]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-11-18 14:29:47]

Description was changed from

==========
Add support for multiple text decorations with same line positioning

Original patch by Sasha Morrissey, rebased and adapted.

Add support for multiple propagated text decorations with the same line
positioning, as defined in http://www.w3.org/TR/css-text-decor-3/.
Before this, elements with multiple text decorations but the same line
positioning (e.g. two underlines) would only paint the 'topmost' one.
With this patch, both lines are painted.

This is important for CSS Text Decorations Module 3, where two text
decorations can have the same line position but different styles (e.g.
wavy and solid), and they should be painted on top of eachother.

This minimally affects some tests where two text decorations with the
same line positioning were present but only one was being painted.

Also cleaned up the existing logic for text decorations to only inherit
through StyleAdjuster, removing the need to traverse up the tree to
resolve colors for inherited text decorations.

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

to

==========
Add support for multiple text decorations with same line positioning

Original patch by Sasha Morrissey, rebased and adapted.

Add support for multiple propagated text decorations with the same line
positioning, as defined in http://www.w3.org/TR/css-text-decor-3/.
Before this, elements with multiple text decorations but the same line
positioning (e.g. two underlines) would only paint the 'topmost' one.
With this patch, both lines are painted.

This is important for CSS Text Decorations Module 3, where two text
decorations can have the same line position but different styles (e.g.
wavy and solid), and they should be painted on top of eachother.

This minimally affects some tests where two text decorations with the
same line positioning were present but only one was being painted.

Also cleaned up the existing logic for text decorations to only inherit
through StyleAdjuster, removing the need to traverse up the tree to
resolve colors for inherited text decorations.

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

Committed: https://crrev.com/3ffa4763a6a442e26f1ea2a87ee2bf3f0aa26e1b
Cr-Commit-Position: refs/heads/master@{#433191}
==========

[commit-bot: I haz the power, 2016-11-18 14:29:49]

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