Fix painting background for composited table cells in a non-composited row.

Fix painting background for composited table cells in a non-composited row.

Use a background specific display item client and the LayoutTableRow's
visual rect.

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

Committed: https://crrev.com/9c8fded973e0422c68dd85f3745b26a0a8d96085
Cr-Commit-Position: refs/heads/master@{#432693}

[wkorman, 2016-11-16 20:04:27]

Description was changed from

==========
Fix painting background for composited table cells in a non-composited row.

Use a row background specific display item client, and the LayoutTableRow's
visual rect.

BUG=658874
==========

to

==========
Fix painting background for composited table cells in a non-composited row.

Use a row background specific display item client, and the LayoutTableRow's
visual rect.

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

[wkorman, 2016-11-16 20:15:46]

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

[wkorman, 2016-11-16 20:15:47]

https://codereview.chromium.org/2507893002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/paint/invalidation/table/composited-table-background-composited-row.html
(right):

https://codereview.chromium.org/2507893002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/paint/invalidation/table/composited-table-background-composited-row.html:16:
<table>
This test is intended to validate your comment from
https://codereview.chromium.org/2469903002 re: "It should also be able to
correctly handle the case that a row is composited but the intersecting col and
section don't have background.". It's the same as the base
paint/invalidation/table/composited-table-background.html test except that the
second row is composited. If I've misinterpreted in some way let me know.

Also all tests have two cases, the base one where initially the background area
in question is red, and a variant with -initial-empty.html where the background
area is initially not set. This is to validate that we deal gracefully with the
case where we don't need to create the special display item client initially,
but we do need it later on style background change.

https://codereview.chromium.org/2507893002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/TablePaintInvalidator.cpp (right):

https://codereview.chromium.org/2507893002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/TablePaintInvalidator.cpp:89:
cell->createCompositedCellDisplayItemClients();
I've punted on using mutable or getMutableForPainting() in the interest of time
and since it's only this one call now. If you feel strongly I can rework.

[wkorman, 2016-11-16 20:38:57]

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

[Xianzhu, 2016-11-16 20:39:03]

https://codereview.chromium.org/2507893002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTableCell.cpp (right):

https://codereview.chromium.org/2507893002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableCell.cpp:471: if
(row()->styleRef().hasBackground() && !m_rowBackgroundDisplayItemClient) {
We should remove the hasbackground() condition for cases that the row
doesn't have background, but section or col/colgroup has, in case that the row
is not composited.

https://codereview.chromium.org/2507893002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableCell.cpp:482:
createCompositedCellDisplayItemClients();
With the call to createCompositedCellDisplayItemClients() in
TablePaintInvalidator replaced with ensureIsreadyForPaintInvalidation(), you can
directly inline the function here.

https://codereview.chromium.org/2507893002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/TablePaintInvalidator.cpp (right):

https://codereview.chromium.org/2507893002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/TablePaintInvalidator.cpp:89:
cell->createCompositedCellDisplayItemClients();
On 2016/11/16 20:15:46, wkorman wrote:
> I've punted on using mutable or getMutableForPainting() in the interest of
time
> and since it's only this one call now. If you feel strongly I can rework.

This can be simplified if you have
LayoutTableCell::backgroundDisplayItemClient():

 cell->ensureIsReadyForPaintInvalidation();
 ObjectPaintInvalidator(*row)
    .slowSetPaintLayerNeedsRepaintAndInvalidateDisplayItemClient(
        cell->backgroundDisplayItemClient(), PaintInvalidaitonStyleChange);

And you can also replace the other "*cell" parameters to
"cell->backgroundDisplayItemClient()" to increase the coverage of failing cases
(e.g. composited cell on background change of section, in case that the row is
not composited).

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

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 21:26:07]

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

[commit-bot: I haz the power, 2016-11-16 21:26:08]

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

[wkorman, 2016-11-16 21:42:33]

Description was changed from

==========
Fix painting background for composited table cells in a non-composited row.

Use a row background specific display item client, and the LayoutTableRow's
visual rect.

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

to

==========
Fix painting background for composited table cells in a non-composited row.

Use a background specific display item client and the LayoutTableRow's
visual rect.

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

[wkorman, 2016-11-16 21:42:45]

https://codereview.chromium.org/2507893002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutTableCell.cpp (right):

https://codereview.chromium.org/2507893002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableCell.cpp:471: if
(row()->styleRef().hasBackground() && !m_rowBackgroundDisplayItemClient) {
On 2016/11/16 20:39:03, Xianzhu wrote:
> We should remove the hasbackground() condition for cases that the row
> doesn't have background, but section or col/colgroup has, in case that the row
> is not composited.

Done.

https://codereview.chromium.org/2507893002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableCell.cpp:482:
createCompositedCellDisplayItemClients();
On 2016/11/16 20:39:03, Xianzhu wrote:
> With the call to createCompositedCellDisplayItemClients() in
> TablePaintInvalidator replaced with ensureIsreadyForPaintInvalidation(), you
can
> directly inline the function here.

Done.

https://codereview.chromium.org/2507893002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/TablePaintInvalidator.cpp (right):

https://codereview.chromium.org/2507893002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/TablePaintInvalidator.cpp:89:
cell->createCompositedCellDisplayItemClients();
On 2016/11/16 20:39:03, Xianzhu wrote:
> On 2016/11/16 20:15:46, wkorman wrote:
> > I've punted on using mutable or getMutableForPainting() in the interest of
> time
> > and since it's only this one call now. If you feel strongly I can rework.
> 
> This can be simplified if you have
> LayoutTableCell::backgroundDisplayItemClient():
> 
>  cell->ensureIsReadyForPaintInvalidation();
>  ObjectPaintInvalidator(*row)
>     .slowSetPaintLayerNeedsRepaintAndInvalidateDisplayItemClient(
>         cell->backgroundDisplayItemClient(), PaintInvalidaitonStyleChange);
> 
> And you can also replace the other "*cell" parameters to
> "cell->backgroundDisplayItemClient()" to increase the coverage of failing
cases
> (e.g. composited cell on background change of section, in case that the row is
> not composited).

Done.

[wkorman, 2016-11-16 21:46:10]

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

[commit-bot: I haz the power, 2016-11-16 21:47:15]

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

[Xianzhu, 2016-11-16 21:54:33]

lgtm

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

https://codereview.chromium.org/2507893002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/TablePaintInvalidator.cpp:63:
cell->ensureIsReadyForPaintInvalidation();
Nit: can this be put at the beginning of the 'for' loop body?

[wkorman, 2016-11-16 22:07:42]

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

https://codereview.chromium.org/2507893002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/TablePaintInvalidator.cpp:63:
cell->ensureIsReadyForPaintInvalidation();
On 2016/11/16 21:54:32, Xianzhu wrote:
> Nit: can this be put at the beginning of the 'for' loop body?

Done.

[wkorman, 2016-11-16 22:39:34]

The CQ bit was checked by wkorman@chromium.org

[wkorman, 2016-11-16 22:39:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
wangxianzhu@chromium.org
Link to the patchset: https://codereview.chromium.org/2507893002/#ps120001
(title: "Add rebaselines to TestExpectations due to http://crbug.com/665765
preventing rebaseline-cl happine…")

[commit-bot: I haz the power, 2016-11-16 22:40:32]

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 01:51:35]

Description was changed from

==========
Fix painting background for composited table cells in a non-composited row.

Use a background specific display item client and the LayoutTableRow's
visual rect.

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

to

==========
Fix painting background for composited table cells in a non-composited row.

Use a background specific display item client and the LayoutTableRow's
visual rect.

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

[commit-bot: I haz the power, 2016-11-17 01:51:37]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-11-17 02:07:07]

Description was changed from

==========
Fix painting background for composited table cells in a non-composited row.

Use a background specific display item client and the LayoutTableRow's
visual rect.

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

to

==========
Fix painting background for composited table cells in a non-composited row.

Use a background specific display item client and the LayoutTableRow's
visual rect.

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

Committed: https://crrev.com/9c8fded973e0422c68dd85f3745b26a0a8d96085
Cr-Commit-Position: refs/heads/master@{#432693}
==========

[commit-bot: I haz the power, 2016-11-17 02:07:08]

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

[Xianzhu, 2016-11-17 07:17:58]

Here are statistics of composited table component backgrounds
(https://ct.skia.org/chromium_perf_runs/ run 1466):
- Total cells: 514,873
  Composited cells: 1,082
  Composited cells, in any row/section/col with background: 27
- Total rows: 367,275
  Composited rows: 33
  Composited rows, in any section with background, or intersecting any col with
background: 0
- Total sections: 320,503
  Composited sections: 28
  Composited sections, intersecting any col with background: 0

The cases of composited rows and sections are very rare, and there is no single
case breaking this CL.

[wkorman, 2016-11-17 19:11:30]

On 2016/11/17 07:17:58, Xianzhu wrote:
> Here are statistics of composited table component backgrounds
> (https://ct.skia.org/chromium_perf_runs/ run 1466):
> - Total cells: 514,873
>   Composited cells: 1,082
>   Composited cells, in any row/section/col with background: 27
> - Total rows: 367,275
>   Composited rows: 33
>   Composited rows, in any section with background, or intersecting any col
with
> background: 0
> - Total sections: 320,503
>   Composited sections: 28
>   Composited sections, intersecting any col with background: 0
> 
> The cases of composited rows and sections are very rare, and there is no
single
> case breaking this CL.

OK, thanks. It is worth noting the bug originally was found on Mac Retina where
we prioritize compositing over lcd text. I expect the perf run doesn't cover
this. We could run with --enable-prefer-compositing-to-lcd-text flag as well if
you think worthwhile. However our main concern was re: composited rows/sections
which I wouldn't expect to be affected by that flag.