Paint text underline/overline decorations before painting text.

Paint text underline/overline decorations before painting text.

We've been painting them after text. This is incorrect per:

https://www.w3.org/TR/css-text-decor-3/#painting-order

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

Review-Url: https://codereview.chromium.org/2820743003
Cr-Commit-Position: refs/heads/master@{#465746}
Committed: https://chromium.googlesource.com/chromium/src/+/7b4caa4232e5c7246d43fc954264754b6f396bf8

[wkorman, 2017-04-14 22:10:08]

Description was changed from

==========
Paint text decorations before painting text.

Reference:

https://www.w3.org/TR/css-text-decor-3/#painting-order

BUG=547174
==========

to

==========
Paint text decorations before painting text.

Reference:

https://www.w3.org/TR/css-text-decor-3/#painting-order

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

[wkorman, 2017-04-14 22:10:25]

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

[commit-bot: I haz the power, 2017-04-14 22:10:52]

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-04-14 23:35:54]

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

[commit-bot: I haz the power, 2017-04-14 23:35:55]

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

[wkorman, 2017-04-15 02:50:40]

Description was changed from

==========
Paint text decorations before painting text.

Reference:

https://www.w3.org/TR/css-text-decor-3/#painting-order

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

to

==========
Paint text underline/overline decorations before painting text.

We've been painting them after text. This is incorrect per:

https://www.w3.org/TR/css-text-decor-3/#painting-order

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

[wkorman, 2017-04-15 03:01:03]

wkorman@chromium.org changed reviewers:
+ pdr@chromium.org

[wkorman, 2017-04-15 03:01:04]

Seeking initial feedback. See further comments on bug in particular the layout
tests mentioned in http://crbug.com/547174#c9. I'll start a dry run to generate
updated layout test failures for review.

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

https://codereview.chromium.org/2820743003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp:509: static void
ComputeDecorationInfo(
I made this (and the two paint methods below) static because they use
DecorationInfo, which we want to keep private if possible, but we also want to
use DecorationInfo with AppliedDecorationPainter, which is itself static to this
file.

I had AppliedDecorationPainter marked as a friend of InlineTextBoxPainter (see
two patchsets previously) but on reflection thought this was cleaner since
PaintDecorations() was only a member method in order to access the
InlineTextBox.

Unfortunately this also means we have a larger diff since I had to move them to
be defined prior to their callsite. The patch is not as large as it looks re:
new code written since much of below is a minor modification of the original
logic.

https://codereview.chromium.org/2820743003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp:862:
DecorationInfo decoration_info;
I had this as an Optional (see two patchsets previous to this one) but that
prevents stack-allocation. Allocating on heap is likely expensive given how hot
this code path is, so I think better to make stack-allocated, but open to
thoughts.

[wkorman, 2017-04-15 03:02:27]

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

[commit-bot: I haz the power, 2017-04-15 03:02:43]

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-04-15 03:34:09]

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

[commit-bot: I haz the power, 2017-04-15 03:34:09]

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

[wkorman, 2017-04-17 17:57:59]

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

https://codereview.chromium.org/2820743003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/InlineTextBoxPainter.cpp:241: float
start_point_y_offset,
This patch changes this to a float, which preserves fractional pixel values and
matches previous behavior, fixing bug noted in http://crbug.com/547174#c10 and
seen in that test's image failure diff in previous patchset layout test run.

[wkorman, 2017-04-17 17:58:09]

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

[commit-bot: I haz the power, 2017-04-17 17:58:39]

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

[wkorman, 2017-04-17 18:30:20]

wkorman@chromium.org changed reviewers:
+ schenney@chromium.org

[wkorman, 2017-04-17 18:30:21]

+schenney as another set of paint eyes.

[pdr., 2017-04-17 18:41:01]

On 2017/04/17 at 18:30:21, wkorman wrote:
> +schenney as another set of paint eyes.

This looks good to me. I think
fast/css3-text/css3-text-decoration/repaint/repaint-text-decoration-style.html
is a good example of the changes from this patch, and the diffs seem good to me.

[commit-bot: I haz the power, 2017-04-17 18:45:57]

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

[commit-bot: I haz the power, 2017-04-17 18:45:58]

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

[wkorman, 2017-04-17 18:46:16]

On 2017/04/17 18:41:01, pdr. wrote:
> On 2017/04/17 at 18:30:21, wkorman wrote:
> > +schenney as another set of paint eyes.
> 
> This looks good to me. I think
> fast/css3-text/css3-text-decoration/repaint/repaint-text-decoration-style.html
> is a good example of the changes from this patch, and the diffs seem good to
me.

Need an actual LGTM if you are on board!

[pdr., 2017-04-17 18:47:03]

On 2017/04/17 at 18:46:16, wkorman wrote:
> On 2017/04/17 18:41:01, pdr. wrote:
> > On 2017/04/17 at 18:30:21, wkorman wrote:
> > > +schenney as another set of paint eyes.
> > 
> > This looks good to me. I think
> >
fast/css3-text/css3-text-decoration/repaint/repaint-text-decoration-style.html
> > is a good example of the changes from this patch, and the diffs seem good to
me.
> 
> Need an actual LGTM if you are on board!

LGTM

[wkorman, 2017-04-19 18:36:20]

The CQ bit was checked by wkorman@chromium.org

[wkorman, 2017-04-19 18:36:22]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/2820743003/#ps140001
(title: "Sync to head.")

[commit-bot: I haz the power, 2017-04-19 18:36:55]

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-04-19 20:23:27]

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

[commit-bot: I haz the power, 2017-04-19 20:23:29]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pdr., 2017-04-19 20:24:46]

On 2017/04/19 at 20:23:29, commit-bot wrote:
> Try jobs failed on following builders:
>   linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

these look unrelated

[pdr., 2017-04-19 20:24:49]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2017-04-19 20:25:16]

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-04-19 21:11:30]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1492633489767280,
"parent_rev": "c751fcc699b1d5e905870e62f453b326195e55e9", "commit_rev":
"7b4caa4232e5c7246d43fc954264754b6f396bf8"}

[commit-bot: I haz the power, 2017-04-19 21:13:01]

Description was changed from

==========
Paint text underline/overline decorations before painting text.

We've been painting them after text. This is incorrect per:

https://www.w3.org/TR/css-text-decor-3/#painting-order

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

to

==========
Paint text underline/overline decorations before painting text.

We've been painting them after text. This is incorrect per:

https://www.w3.org/TR/css-text-decor-3/#painting-order

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

Review-Url: https://codereview.chromium.org/2820743003
Cr-Commit-Position: refs/heads/master@{#465746}
Committed:
https://chromium.googlesource.com/chromium/src/+/7b4caa4232e5c7246d43fc954264...
==========

[commit-bot: I haz the power, 2017-04-19 21:13:03]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/7b4caa4232e5c7246d43fc954264...