Use appropriate background object visual rect for composited table cell backgrounds.

Use appropriate background object visual rect for composited table cell backgrounds.

Previously painting table cell backgrounds always used the visual rect for the
cell. It turns out that even for composited table cells:
1. section, col and colgroup backgrounds are painted on the table section object's layer
2. row backgrounds are painted on the table row object's layer

Thus this change revises cell background paint logic to use the actual involved
background object's visual rect.

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

[wkorman, 2016-11-01 21:06:15]

Description was changed from

==========
Use table visual rect when painting composited table cell background.

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

to

==========
Use table visual rect when painting composited table cell background.

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

[wkorman, 2016-11-01 21:14:47]

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

[wkorman, 2016-11-01 21:14:48]

I discovered that the issue we resolved in http://crrev.com/2432663002 also
manifests without collapsed borders. This change is one approach to address.

We could alternatively construct m_collapsedBorderValues in this case even when
there are no collapsed borders, but IMO that would make the code confusing re:
the class/variable name and its usage.

We could rename the class/variable, but we'd still be allocating the border ints
unnecessarily.

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

https://codereview.chromium.org/2469903002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableCell.cpp:1450: if
(usesTableAsAdditionalDisplayItemClient()) {
Is it worth also making sure compositedTableCell() is not nullptr here? We
should know that it is per LayoutTableCell::ensureIsReadyForPaintInvalidation().
Could add DCHECK, open to thought.

[wkorman, 2016-11-01 21:15:47]

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

[commit-bot: I haz the power, 2016-11-01 21:16:22]

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-01 21:28:14]

lgtm

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

https://codereview.chromium.org/2469903002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableCell.cpp:1450: if
(usesTableAsAdditionalDisplayItemClient()) {
On 2016/11/01 21:14:48, wkorman wrote:
> Is it worth also making sure compositedTableCell() is not nullptr here? We
> should know that it is per
LayoutTableCell::ensureIsReadyForPaintInvalidation().
> Could add DCHECK, open to thought.

DCHECK sgtm.

[wkorman, 2016-11-03 01:39:03]

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

[commit-bot: I haz the power, 2016-11-03 01:39:25]

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

[wkorman, 2016-11-03 01:41:24]

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

[wkorman, 2016-11-03 01:41:25]

+chrishtr as we discussed in person this evening.

PTAL, updated change to keep collapsed borders the same as it was, and instead
use an add'l row-specific display item client for painting table row
backgrounds.

I see http://crrev.com/2430313004 but likely we should land this in some form
ahead of that change to allow more easily merging a fix for composited cell
background painting to M55.

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

https://codereview.chromium.org/2469903002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutTableCell.cpp:1450: if
(usesTableAsAdditionalDisplayItemClient()) {
On 2016/11/01 21:28:14, Xianzhu wrote:
> On 2016/11/01 21:14:48, wkorman wrote:
> > Is it worth also making sure compositedTableCell() is not nullptr here? We
> > should know that it is per
> LayoutTableCell::ensureIsReadyForPaintInvalidation().
> > Could add DCHECK, open to thought.
> 
> DCHECK sgtm.

Done.

https://codereview.chromium.org/2469903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableCell.cpp (right):

https://codereview.chromium.org/2469903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableCell.cpp:1447: bool
LayoutTableCell::usesCompositedCellDisplayItemClients() const {
This is a bit of a misnomer since it's also true for SPv2, not just composited
cells, but I couldn't think of something better and I believe we want to use
this same logic for both the table and row case. 'isCompositedOrSPv2' or similar
just seemed unwieldy.

https://codereview.chromium.org/2469903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableCell.cpp:1468:
invalidator.invalidateDisplayItemClient(*m_rowBackgroundDisplayItemClient,
This wasn't necessary for all tests to pass but I believe we need to do this to
be correct, unless it's sufficient for us to rely on the one explicit
invalidation of this display item client in TablePaintInvalidator?

[wkorman, 2016-11-03 01:41:38]

Description was changed from

==========
Use table visual rect when painting composited table cell background.

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

to

==========
Use table row visual rect when painting composited table cell background.

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

[commit-bot: I haz the power, 2016-11-03 02:26:58]

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

[commit-bot: I haz the power, 2016-11-03 02:26:59]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Xianzhu, 2016-11-03 03:06:49]

https://codereview.chromium.org/2469903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableCell.cpp (right):

https://codereview.chromium.org/2469903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableCell.cpp:1447: bool
LayoutTableCell::usesCompositedCellDisplayItemClients() const {
On 2016/11/03 01:41:25, wkorman wrote:
> This is a bit of a misnomer since it's also true for SPv2, not just composited
> cells, but I couldn't think of something better and I believe we want to use
> this same logic for both the table and row case. 'isCompositedOrSPv2' or
similar
> just seemed unwieldy.

I think this is OK. When we remove non-SPv2 code, we'll remove this function and
always use rowBackgroundDisplayItemClient.

https://codereview.chromium.org/2469903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/TableCellPainter.cpp (right):

https://codereview.chromium.org/2469903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/TableCellPainter.cpp:206: type))
This seems incorrect when we paint background of the containing column or
section behind a cell. In the case we should use table or section visual rect.

Should we paint all backgrounds from a container as a whole display item with
the container as the DisplayItemClient?

Or should we paint all backgrounds from the containers of a cell on the cell's
backing instead of containers' backing, still with the cell as the
DisplayItemClient?

The latter seems more correct, considering rowspan, colspan, etc.

[wkorman, 2016-11-03 04:02:39]

https://codereview.chromium.org/2469903002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/TableCellPainter.cpp (right):

https://codereview.chromium.org/2469903002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/TableCellPainter.cpp:206: type))
On 2016/11/03 03:06:49, Xianzhu wrote:
> This seems incorrect when we paint background of the containing column or
> section behind a cell. In the case we should use table or section visual rect.
> 
> Should we paint all backgrounds from a container as a whole display item with
> the container as the DisplayItemClient?
> 
> Or should we paint all backgrounds from the containers of a cell on the cell's
> backing instead of containers' backing, still with the cell as the
> DisplayItemClient?
> 
> The latter seems more correct, considering rowspan, colspan, etc. 

Hmm, ok. I missed that this was called from non-row code paths, but of course it
would be, I see it now in TableSectionPainter::paintBackgroundsBehindCell.

It's unfortunate we're missing so much test coverage for composited table
cells/rows/etc., given this is wrong we should have had some failing tests. I'd
like to add more to validate that those things break with this change, and then
fix them.

I am however OOO until Monday so I will have to pick this up then unless someone
else can take it.

On your questions, I don't know enough yet on the tradeoffs for the two choices
to pick one. The latter sounds conceptually better, but potentially less
performant?

[Xianzhu, 2016-11-03 05:27:45]

On 2016/11/03 04:02:39, wkorman wrote:
>
https://codereview.chromium.org/2469903002/diff/120001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/paint/TableCellPainter.cpp (right):
> 
>
https://codereview.chromium.org/2469903002/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/paint/TableCellPainter.cpp:206: type))
> On 2016/11/03 03:06:49, Xianzhu wrote:
> > This seems incorrect when we paint background of the containing column or
> > section behind a cell. In the case we should use table or section visual
rect.
> > 
> > Should we paint all backgrounds from a container as a whole display item
with
> > the container as the DisplayItemClient?
> > 
> > Or should we paint all backgrounds from the containers of a cell on the
cell's
> > backing instead of containers' backing, still with the cell as the
> > DisplayItemClient?
> > 
> > The latter seems more correct, considering rowspan, colspan, etc. 
> 
> Hmm, ok. I missed that this was called from non-row code paths, but of course
it
> would be, I see it now in TableSectionPainter::paintBackgroundsBehindCell.
> 
> It's unfortunate we're missing so much test coverage for composited table
> cells/rows/etc., given this is wrong we should have had some failing tests.
I'd
> like to add more to validate that those things break with this change, and
then
> fix them.
> 
> I am however OOO until Monday so I will have to pick this up then unless
someone
> else can take it.
> 
> On your questions, I don't know enough yet on the tradeoffs for the two
choices
> to pick one. The latter sounds conceptually better, but potentially less
> performant?

Just looked into them a bit and found I was wrong about that the latter was more
correct than the former. I thought the visual rects of table sections and rows
didn't cover spanning cells but they do. The latter will break paint order of
backgrounds and shadows. For each element (section, row or cell) we need to
paint backgrounds and shadows in the order of outset shadow, background and
inset shadow, so we can't let the cell paint all container backgrounds and its
own background together because they need to be interlaced with shadows.

However, similar to the collapsed border changes, the whole display item method
may be riskier about functionality and performance than your
special-DisplayItemClient-in-cell method. I'd suggest to continue with this CL
(or with subsequent CLs) by adding special DisplayItemClients for section (for
section background) and table (for col, colgroup). It seems fine that you pick
this up next week.

[wkorman, 2016-11-12 01:45:32]

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

[commit-bot: I haz the power, 2016-11-12 01:45:55]

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

[wkorman, 2016-11-12 01:51:43]

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

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

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-12 02:51:33]

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

[commit-bot: I haz the power, 2016-11-12 02:51:34]

Dry run: Exceeded global retry quota

[wkorman, 2016-11-14 19:52:55]

Description was changed from

==========
Use table row visual rect when painting composited table cell background.

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

to

==========
Use appropriate background object visual rect for composited table cell
backgrounds.

Previously painting table cell backgrounds always used the visual rect for the
cell. It turns out that even for composited table cells:
1. section, col and colgroup backgrounds are painted on the table section
object's layer
2. row backgrounds are painted on the table row object's layer

Thus this change revises cell background paint logic to use the actual involved
background object's visual rect.

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

[wkorman, 2016-11-15 01:05:34]

wangxianzhu@ -- PTAL, updated per our discussion today. Should now cover row,
col, colgroup, col w/ span, and section.

https://codereview.chromium.org/2469903002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableCell.cpp (right):

https://codereview.chromium.org/2469903002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableCell.cpp:502: // Below handles
the case where a page starts out in a state where the table
I ended up keeping this as it handles one case (described in this comment) which
is not handled by the logic added to TablePaintInvalidator. TPI currently walks
through looking for any background style changes, doesn't see any, and bails
out. We could add a cell-walk to do this, but using this method for what it's
intended on initial paint seems more appropriate.

https://codereview.chromium.org/2469903002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableCell.h (right):

https://codereview.chromium.org/2469903002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableCell.h:486:
std::unique_ptr<RowBackgroundDisplayItemClient>
We could use RareData to turn this batch of pointers into one, but probably
better to do later if memory/perf shows warranted.

https://codereview.chromium.org/2469903002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/TablePaintInvalidator.cpp (right):

https://codereview.chromium.org/2469903002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/TablePaintInvalidator.cpp:26: // Note there
are explicit calls to
This documents the explicit calls to create the clients noted below, if you've
an alternate preference for how to structure in light of my note on const-ness
and Painter, let me know.

[wkorman, 2016-11-15 01:05:57]

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

[commit-bot: I haz the power, 2016-11-15 01:07:01]

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

[wkorman, 2016-11-15 02:30:14]

https://codereview.chromium.org/2469903002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableCell.h (right):

https://codereview.chromium.org/2469903002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableCell.h:320: class
RowBackgroundDisplayItemClient : public DisplayItemClient {
This class and the two others below are essentially all the same. We could use a
single class def, such as ProxyDisplayItemClient, that takes LayoutObject, and
debugName() could just use that object's debugName with "BackgroundProxy" suffix
or somesuch.

[Xianzhu, 2016-11-15 02:58:23]

https://codereview.chromium.org/2469903002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableCell.cpp (right):

https://codereview.chromium.org/2469903002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableCell.cpp:1510:
RuntimeEnabledFeatures::slimmingPaintV2Enabled();
This misses the case that the row is composited but the cell is not, and the
section has background. In the case, we should create section display item
clients in cells for section backgrounds painted on the parent layer of the row.
Another similar case is that a section is composited and a col has background.

At first I thought of for simplicity that we could just embed the display item
clients in DisplayItemClients. Then we could replace the "*cell"s in
TablePaintInvalidator with the special display item clients without adding more
code there.

Now I think to make the CL small enough for merging we could just fix the case
that a composited cell in non-composited row and section first (which is
basically one of the previous patch sets) and merge it, then investigate
painting all backgrounds for a row or section in one display item to fix all the
cases. Wdyt?

https://codereview.chromium.org/2469903002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/TablePaintInvalidator.cpp (right):

https://codereview.chromium.org/2469903002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/TablePaintInvalidator.cpp:37: // which
would fail const-ness.
We can use mutable keyword or the LayoutObject::getMutableForPainting() pattern.
We do need and allow some part of layout objects to be mutable for painting
code, but are using const to avoid unintended mutations.

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

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

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

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[wkorman, 2016-11-15 19:46:02]

https://codereview.chromium.org/2469903002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableCell.cpp (right):

https://codereview.chromium.org/2469903002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableCell.cpp:1510:
RuntimeEnabledFeatures::slimmingPaintV2Enabled();
On 2016/11/15 02:58:23, Xianzhu wrote:
> This misses the case that the row is composited but the cell is not, and the
> section has background. In the case, we should create section display item
> clients in cells for section backgrounds painted on the parent layer of the
row.
> Another similar case is that a section is composited and a col has background.

Ah. I will look at adding tests for these cases (even though this change may be
tabled for now).

> At first I thought of for simplicity that we could just embed the display item
> clients in DisplayItemClients. Then we could replace the "*cell"s in
> TablePaintInvalidator with the special display item clients without adding
more
> code there.

Can you elaborate on this, I am not sure what you mean by "embed the display
item
clients in DisplayItemClients". Maybe you mean, essentially, what this change
currently does -- by adding add'l display item clients within LayoutTableCell?

> Now I think to make the CL small enough for merging we could just fix the case
> that a composited cell in non-composited row and section first (which is
> basically one of the previous patch sets) and merge it, then investigate
> painting all backgrounds for a row or section in one display item to fix all
the
> cases. Wdyt?

Sounds ok to me, I am flexible, I will look to prepare separate change minimized
to that.

I may have misunderstood, from your comment in
https://codereview.chromium.org/2469903002#msg16 I thought we'd decided to
pursue this path. Just to make sure I understand, is this a result of:
- concern re: the addition of add'l pointers to LayoutTableCell
- concern re: the general complexity, potential performance/memory impact
- on reconsideration, the appeal/desire to reduce the number of display
items/display item clients for performance purposes

I had come away from your previous comments thinking you were unsure whether one
display item approach would be feasible correctness-wise. While I have not
investigated in depth yet myself or read the W3C table spec in detail, from
discussion and code reading to date it sounds like there is considerable
flexibility and correctness challenges around backgrounds, shadows and opacity
(which has some TODOs noted in code and sounds like it may not currently work in
all cases, if at all) particularly given I believe they can be applied in one or
multiple of table, section, col, cell, and should stack/layer in a particular
manner.

We should also consider, before embarking on single display item for
backgrounds, how much add'l work to invest in tables as I believe focus for
further work is shifting to Grid. I believe Table is in updated re-spec to
document current behavior for improved cross-browser compatibility, and may not
evolve heavily from here.

[Xianzhu, 2016-11-15 20:16:58]

https://codereview.chromium.org/2469903002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableCell.cpp (right):

https://codereview.chromium.org/2469903002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableCell.cpp:1510:
RuntimeEnabledFeatures::slimmingPaintV2Enabled();
On 2016/11/15 19:46:02, wkorman wrote:
> On 2016/11/15 02:58:23, Xianzhu wrote:
> > This misses the case that the row is composited but the cell is not, and the
> > section has background. In the case, we should create section display item
> > clients in cells for section backgrounds painted on the parent layer of the
> row.
> > Another similar case is that a section is composited and a col has
background.
> 
> Ah. I will look at adding tests for these cases (even though this change may
be
> tabled for now).
> 
> > At first I thought of for simplicity that we could just embed the display
item
> > clients in DisplayItemClients. Then we could replace the "*cell"s in
> > TablePaintInvalidator with the special display item clients without adding
> more
> > code there.
> 
> Can you elaborate on this, I am not sure what you mean by "embed the display
> item
> clients in DisplayItemClients". Maybe you mean, essentially, what this change
> currently does -- by adding add'l display item clients within LayoutTableCell?

Sorry, "DisplayItemClients" should be "LayoutTableCells". I meant to just put
the display item clients as direct data members of LayoutTableCell instead of
pointers, so that we always use them for backgrounds from containers. However,
this would cause rasterization regressions in normal cases so it's not a good
way.

> 
> > Now I think to make the CL small enough for merging we could just fix the
case
> > that a composited cell in non-composited row and section first (which is
> > basically one of the previous patch sets) and merge it, then investigate
> > painting all backgrounds for a row or section in one display item to fix all
> the
> > cases. Wdyt?
> 
> Sounds ok to me, I am flexible, I will look to prepare separate change
minimized
> to that.

Let's land a simple CL for the most common case (composited cell in
non-composited row) first. 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.

For other cases, I can run cluster-telemetry to see how rare they are in real
world. I think we can keep the regressions as-is in M55 for the very rare cases,
and fix them on trunk only with a more complete CL.

> 
> I may have misunderstood, from your comment in
> https://codereview.chromium.org/2469903002#msg16 I thought we'd decided to
> pursue this path. Just to make sure I understand, is this a result of:
> - concern re: the addition of add'l pointers to LayoutTableCell
> - concern re: the general complexity, potential performance/memory impact
> - on reconsideration, the appeal/desire to reduce the number of display
> items/display item clients for performance purposes

My concern is mainly about complexity which may lead to risks of
functional/performance regressions, 
 
> 
> I had come away from your previous comments thinking you were unsure whether
one
> display item approach would be feasible correctness-wise. While I have not
> investigated in depth yet myself or read the W3C table spec in detail, from
> discussion and code reading to date it sounds like there is considerable
> flexibility and correctness challenges around backgrounds, shadows and opacity
> (which has some TODOs noted in code and sounds like it may not currently work
in
> all cases, if at all) particularly given I believe they can be applied in one
or
> multiple of table, section, col, cell, and should stack/layer in a particular
> manner.
> 
> We should also consider, before embarking on single display item for
> backgrounds, how much add'l work to invest in tables as I believe focus for
> further work is shifting to Grid. I believe Table is in updated re-spec to
> document current behavior for improved cross-browser compatibility, and may
not
> evolve heavily from here.

I think it will be fine just to fix and avoid regressions about tables. I would
put the existing bugs in the lowest priority even if we want to fix them.