Invalidate row/section for backgrounds when cell's geometry changes (issue 2851553002 by wangxianzhu@chromium.org)

[wangx...@chromium.org]

Reviewers: rhogan, wkorman
CL: https://codereview.chromium.org/2851553002/

Description:
Invalidate row/section for backgrounds when cell's geometry changes

https://codereview.chromium.org/2786463004/ caused the regression.
Now we paint cell's container backgrounds as display item of row/section
instead of cell, so when cell's geometry changes we need to invalidate
the container's backgrounds.

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


Affected files (+161, -0 lines):
A third_party/WebKit/LayoutTests/paint/invalidation/table/container-backgrounds-on-cell-resize.html
A third_party/WebKit/LayoutTests/paint/invalidation/table/container-backgrounds-on-cell-resize-expected.html
M third_party/WebKit/Source/core/layout/LayoutTableCell.h
M third_party/WebKit/Source/core/layout/LayoutTableCell.cpp
M third_party/WebKit/Source/core/paint/BUILD.gn
A third_party/WebKit/Source/core/paint/TableCellPaintInvalidator.h
A third_party/WebKit/Source/core/paint/TableCellPaintInvalidator.cpp

[wko...@chromium.org]

https://codereview.chromium.org/2851553002/diff/1/third_party/WebKit/LayoutTests/paint/invalidation/table/container-backgrounds-on-cell-resize.html
File
third_party/WebKit/LayoutTests/paint/invalidation/table/container-backgrounds-on-cell-resize.html
(right):

https://codereview.chromium.org/2851553002/diff/1/third_party/WebKit/LayoutTests/paint/invalidation/table/container-backgrounds-on-cell-resize.html#newcode6
third_party/WebKit/LayoutTests/paint/invalidation/table/container-backgrounds-on-cell-resize.html:6:
<table id="table" style="width: 300px">
id not used

https://codereview.chromium.org/2851553002/diff/1/third_party/WebKit/Source/core/layout/LayoutTableCell.cpp
File third_party/WebKit/Source/core/layout/LayoutTableCell.cpp (right):

https://codereview.chromium.org/2851553002/diff/1/third_party/WebKit/Source/core/layout/LayoutTableCell.cpp#newcode1493
third_party/WebKit/Source/core/layout/LayoutTableCell.cpp:1493: const
PaintInvalidatorContext& context) const {
Noting while reading --

Perhaps worth enhancing
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/README.md?l=116
to incorporate discussion of PaintInvalidatorContext replacing
PaintInvalidationState.

I guess it's this TODO here:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/README.md?l=215

Also adding some class header docs noting similar for
PaintInvalidatorContext and PaintInvalidator classes.

https://codereview.chromium.org/2851553002/diff/1/third_party/WebKit/Source/core/paint/TableCellPaintInvalidator.cpp
File third_party/WebKit/Source/core/paint/TableCellPaintInvalidator.cpp
(right):

https://codereview.chromium.org/2851553002/diff/1/third_party/WebKit/Source/core/paint/TableCellPaintInvalidator.cpp#newcode19
third_party/WebKit/Source/core/paint/TableCellPaintInvalidator.cpp:19:
PaintInvalidationReason
TableCellPaintInvalidator::InvalidatePaintIfNeeded() {
Can we add a unit test for this new class/logic? I know it may be a
hassle since we probably don't have great mocks/fakes for context and so
forth.

https://codereview.chromium.org/2851553002/diff/1/third_party/WebKit/Source/core/paint/TableCellPaintInvalidator.h
File third_party/WebKit/Source/core/paint/TableCellPaintInvalidator.h
(right):

https://codereview.chromium.org/2851553002/diff/1/third_party/WebKit/Source/core/paint/TableCellPaintInvalidator.h#newcode16
third_party/WebKit/Source/core/paint/TableCellPaintInvalidator.h:16:
class TableCellPaintInvalidator {
Add class header doc noting why we need a table cell specific paint
invalidator.

https://codereview.chromium.org/2851553002/

[wangx...@chromium.org]

https://codereview.chromium.org/2851553002/diff/1/third_party/WebKit/LayoutTests/paint/invalidation/table/container-backgrounds-on-cell-resize.html
File
third_party/WebKit/LayoutTests/paint/invalidation/table/container-backgrounds-on-cell-resize.html
(right):

https://codereview.chromium.org/2851553002/diff/1/third_party/WebKit/LayoutTests/paint/invalidation/table/container-backgrounds-on-cell-resize.html#newcode6
third_party/WebKit/LayoutTests/paint/invalidation/table/container-backgrounds-on-cell-resize.html:6:
<table id="table" style="width: 300px">

On 2017/04/27 22:30:23, wkorman wrote:
> id not used

Done.

https://codereview.chromium.org/2851553002/diff/1/third_party/WebKit/Source/core/layout/LayoutTableCell.cpp
File third_party/WebKit/Source/core/layout/LayoutTableCell.cpp (right):

https://codereview.chromium.org/2851553002/diff/1/third_party/WebKit/Source/core/layout/LayoutTableCell.cpp#newcode1493
third_party/WebKit/Source/core/layout/LayoutTableCell.cpp:1493: const
PaintInvalidatorContext& context) const {

On 2017/04/27 22:30:23, wkorman wrote:
> Noting while reading --
>
> Perhaps worth enhancing
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/README.md?l=116
> to incorporate discussion of PaintInvalidatorContext replacing
> PaintInvalidationState.
>
> I guess it's this TODO here:
>
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/README.md?l=215
>
> Also adding some class header docs noting similar for
PaintInvalidatorContext
> and PaintInvalidator classes.

Good suggestion.

I would like to do this in a separate CL. For PaintInvalidationState I
would like to add "Deprecated" postfix instead of explaining it in
README.md because it will be removed soon (after M59 or M60 becomes
stable).

https://codereview.chromium.org/2851553002/diff/1/third_party/WebKit/Source/core/layout/LayoutTableCell.cpp#newcode1497
third_party/WebKit/Source/core/layout/LayoutTableCell.cpp:1497:
PaintInvalidationReason LayoutTableCell::InvalidatePaintIfNeeded(
I would like to rename this to InvalidatePaintDeprecated() in a
follow-up so that we don't need overriding the deprecated version when
overriding the new version.

https://codereview.chromium.org/2851553002/diff/1/third_party/WebKit/Source/core/paint/TableCellPaintInvalidator.cpp
File third_party/WebKit/Source/core/paint/TableCellPaintInvalidator.cpp
(right):

https://codereview.chromium.org/2851553002/diff/1/third_party/WebKit/Source/core/paint/TableCellPaintInvalidator.cpp#newcode19
third_party/WebKit/Source/core/paint/TableCellPaintInvalidator.cpp:19:
PaintInvalidationReason
TableCellPaintInvalidator::InvalidatePaintIfNeeded() {

On 2017/04/27 22:30:23, wkorman wrote:
> Can we add a unit test for this new class/logic? I know it may be a
hassle since
> we probably don't have great mocks/fakes for context and so forth.

I'm not sure how a unit test would benefit us for this case. To test, I
need to create a table containing at least one column, row, section and
cell which meet the conditions here, call the function, then check if
the layers are set needsRepaint and the display item clients are
invalidated. The test would basically repeat the code in another form.
If there is any error (e.g. we invalidated the wrong layer or wrong
display item client) preventing this code from working, the unit test
can hardly discover the error.

A layout test reproducing the bug can easily discover errors in this
code.

https://codereview.chromium.org/2851553002/diff/1/third_party/WebKit/Source/core/paint/TableCellPaintInvalidator.h
File third_party/WebKit/Source/core/paint/TableCellPaintInvalidator.h
(right):

https://codereview.chromium.org/2851553002/diff/1/third_party/WebKit/Source/core/paint/TableCellPaintInvalidator.h#newcode16
third_party/WebKit/Source/core/paint/TableCellPaintInvalidator.h:16:
class TableCellPaintInvalidator {

On 2017/04/27 22:30:23, wkorman wrote:
> Add class header doc noting why we need a table cell specific paint
invalidator.

I tried to add header doc but all would be implementation details that
the callers don't care about. This is like a private/protected
overriding method whose reason is explained by the implementation.
Actually if we didn't want to separate paint invalidation code from
layout code, this would be just in
LayoutTableCell::invalidatePaintIfNeeded().

https://codereview.chromium.org/2851553002/

[wko...@chromium.org]

lgtm

https://codereview.chromium.org/2851553002/diff/1/third_party/WebKit/Source/core/layout/LayoutTableCell.cpp
File third_party/WebKit/Source/core/layout/LayoutTableCell.cpp (right):

https://codereview.chromium.org/2851553002/diff/1/third_party/WebKit/Source/core/layout/LayoutTableCell.cpp#newcode1493
third_party/WebKit/Source/core/layout/LayoutTableCell.cpp:1493: const
PaintInvalidatorContext& context) const {

On 2017/04/28 00:31:06, Xianzhu wrote:
> On 2017/04/27 22:30:23, wkorman wrote:
> > Noting while reading --
> >
> > Perhaps worth enhancing
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/README.md?l=116
> > to incorporate discussion of PaintInvalidatorContext replacing
> > PaintInvalidationState.
> >
> > I guess it's this TODO here:
> >
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/README.md?l=215
> >
> > Also adding some class header docs noting similar for
PaintInvalidatorContext
> > and PaintInvalidator classes.
>
> Good suggestion.
>
> I would like to do this in a separate CL. For PaintInvalidationState I
would
> like to add "Deprecated" postfix instead of explaining it in README.md
because
> it will be removed soon (after M59 or M60 becomes stable).

Acknowledged.

https://codereview.chromium.org/2851553002/diff/1/third_party/WebKit/Source/core/layout/LayoutTableCell.cpp#newcode1497
third_party/WebKit/Source/core/layout/LayoutTableCell.cpp:1497:
PaintInvalidationReason LayoutTableCell::InvalidatePaintIfNeeded(

On 2017/04/28 00:31:06, Xianzhu wrote:
> I would like to rename this to InvalidatePaintDeprecated() in a
follow-up so
> that we don't need overriding the deprecated version when overriding
the new
> version.

Sounds good.

https://codereview.chromium.org/2851553002/diff/1/third_party/WebKit/Source/core/paint/TableCellPaintInvalidator.cpp
File third_party/WebKit/Source/core/paint/TableCellPaintInvalidator.cpp
(right):

https://codereview.chromium.org/2851553002/diff/1/third_party/WebKit/Source/core/paint/TableCellPaintInvalidator.cpp#newcode19
third_party/WebKit/Source/core/paint/TableCellPaintInvalidator.cpp:19:
PaintInvalidationReason
TableCellPaintInvalidator::InvalidatePaintIfNeeded() {

On 2017/04/28 00:31:06, Xianzhu wrote:
> On 2017/04/27 22:30:23, wkorman wrote:
> > Can we add a unit test for this new class/logic? I know it may be a
hassle
> since
> > we probably don't have great mocks/fakes for context and so forth.
>
> I'm not sure how a unit test would benefit us for this case. To test,
I need to
> create a table containing at least one column, row, section and cell
which meet
> the conditions here, call the function, then check if the layers are
set
> needsRepaint and the display item clients are invalidated. The test
would
> basically repeat the code in another form. If there is any error (e.g.
we
> invalidated the wrong layer or wrong display item client) preventing
this code
> from working, the unit test can hardly discover the error.
>
> A layout test reproducing the bug can easily discover errors in this
code.

Well, unit tests are faster, less flaky, and are conceptually capable of
testing this logic, so I don't agree that a unit test can't find errors
(presuming we take on the hassle of adding some way to track and
scrutinize invalidations, display item clients, etc.). Agree that it
would be sort of the inverse of this class logic, but it would live on
in that state even if this class is rewritten and ensure future changes
don't break it.

Given your point that this is sort of an internal class used only by
LayoutTableCell, a unit test would maybe make more sense within
LayoutTableCellTest.

In any case I am ok with just the layout test but I think it's worth
keeping in mind ongoing to consider expanding where/when we write unit
tests. Today it feels like we still sometimes rely on heavier weight
layout tests. Invalidation state specifically seems like something we
could cover in unit tests as well at some point.

https://codereview.chromium.org/2851553002/diff/1/third_party/WebKit/Source/core/paint/TableCellPaintInvalidator.h
File third_party/WebKit/Source/core/paint/TableCellPaintInvalidator.h
(right):

https://codereview.chromium.org/2851553002/diff/1/third_party/WebKit/Source/core/paint/TableCellPaintInvalidator.h#newcode16
third_party/WebKit/Source/core/paint/TableCellPaintInvalidator.h:16:
class TableCellPaintInvalidator {

On 2017/04/28 00:31:06, Xianzhu wrote:
> On 2017/04/27 22:30:23, wkorman wrote:
> > Add class header doc noting why we need a table cell specific paint
> invalidator.
>
> I tried to add header doc but all would be implementation details that
the
> callers don't care about. This is like a private/protected overriding
method
> whose reason is explained by the implementation. Actually if we didn't
want to
> separate paint invalidation code from layout code, this would be just
in
> LayoutTableCell::invalidatePaintIfNeeded().

Yeah, this makes sense. Could add a brief header comment essentially
just stating that then, but am ok either way.

We did similar for PropertyTreeManager which is basically a private
class used by PaintArtifactCompositor:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graphics/compositing/PropertyTreeManager.h?q=propertytreemanager.h&sq=package:chromium&dr=CSs&l=30

https://codereview.chromium.org/2851553002/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2851553002/40001

https://codereview.chromium.org/2851553002/

[commit-bot@chromium.org via codereview.chromium.org]

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_ng/builds/441104)

https://codereview.chromium.org/2851553002/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch.

Follow status at:

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2851553002/60001

https://codereview.chromium.org/2851553002/

[commit-bot@chromium.org via codereview.chromium.org]

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

https://codereview.chromium.org/2851553002/