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

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

Review-Url: https://codereview.chromium.org/2851553002
Cr-Commit-Position: refs/heads/master@{#468056}
Committed: https://chromium.googlesource.com/chromium/src/+/91f1e343eb3fcc6694c740548e2928519b0350f3

[Xianzhu, 2017-04-27 17:10:54]

Description was changed from

==========
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
==========

to

==========
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
==========

[Xianzhu, 2017-04-27 17:11:14]

wangxianzhu@chromium.org changed reviewers:
+ robhogan@gmail.com, wkorman@chromium.org

[Xianzhu, 2017-04-27 17:12:44]

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

[commit-bot: I haz the power, 2017-04-27 17:13:28]

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-27 17:30:47]

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

[commit-bot: I haz the power, 2017-04-27 17:30:48]

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

[wkorman, 2017-04-27 22:30:23]

https://codereview.chromium.org/2851553002/diff/1/third_party/WebKit/LayoutTe...
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/LayoutTe...
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/c...
File third_party/WebKit/Source/core/layout/LayoutTableCell.cpp (right):

https://codereview.chromium.org/2851553002/diff/1/third_party/WebKit/Source/c...
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/REA...
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/REA...

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

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

https://codereview.chromium.org/2851553002/diff/1/third_party/WebKit/Source/c...
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/c...
File third_party/WebKit/Source/core/paint/TableCellPaintInvalidator.h (right):

https://codereview.chromium.org/2851553002/diff/1/third_party/WebKit/Source/c...
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.

[Xianzhu, 2017-04-28 00:31:07]

https://codereview.chromium.org/2851553002/diff/1/third_party/WebKit/LayoutTe...
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/LayoutTe...
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/c...
File third_party/WebKit/Source/core/layout/LayoutTableCell.cpp (right):

https://codereview.chromium.org/2851553002/diff/1/third_party/WebKit/Source/c...
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/REA...
> 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/REA...
> 
> 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/c...
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/c...
File third_party/WebKit/Source/core/paint/TableCellPaintInvalidator.cpp (right):

https://codereview.chromium.org/2851553002/diff/1/third_party/WebKit/Source/c...
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/c...
File third_party/WebKit/Source/core/paint/TableCellPaintInvalidator.h (right):

https://codereview.chromium.org/2851553002/diff/1/third_party/WebKit/Source/c...
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().

[wkorman, 2017-04-28 02:24:55]

lgtm

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

https://codereview.chromium.org/2851553002/diff/1/third_party/WebKit/Source/c...
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/REA...
> > 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/REA...
> > 
> > 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/c...
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/c...
File third_party/WebKit/Source/core/paint/TableCellPaintInvalidator.cpp (right):

https://codereview.chromium.org/2851553002/diff/1/third_party/WebKit/Source/c...
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/c...
File third_party/WebKit/Source/core/paint/TableCellPaintInvalidator.h (right):

https://codereview.chromium.org/2851553002/diff/1/third_party/WebKit/Source/c...
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/graph...

[Xianzhu, 2017-04-28 04:34:55]

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

[commit-bot: I haz the power, 2017-04-28 04:35:21]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Xianzhu, 2017-04-28 05:57:11]

The CQ bit was unchecked by wangxianzhu@chromium.org

[Xianzhu, 2017-04-28 05:57:16]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2017-04-28 05:57:16]

The patchset sent to the CQ was uploaded after l-g-t-m from wkorman@chromium.org
Link to the patchset: https://codereview.chromium.org/2851553002/#ps40001
(title: "-")

[commit-bot: I haz the power, 2017-04-28 05:57:31]

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-28 08:16:27]

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

[commit-bot: I haz the power, 2017-04-28 08:16:28]

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, 2017-04-28 16:04:42]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2017-04-28 16:04:43]

The patchset sent to the CQ was uploaded after l-g-t-m from wkorman@chromium.org
Link to the patchset: https://codereview.chromium.org/2851553002/#ps60001
(title: "Fix non-spinvalidation")

[commit-bot: I haz the power, 2017-04-28 16:05:21]

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-28 17:58:44]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493395482651560,
"parent_rev": "ff3fa0df8039326ede8f5f246312e3109836aff3", "commit_rev":
"91f1e343eb3fcc6694c740548e2928519b0350f3"}

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

Description was changed from

==========
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
==========

to

==========
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

Review-Url: https://codereview.chromium.org/2851553002
Cr-Commit-Position: refs/heads/master@{#468056}
Committed:
https://chromium.googlesource.com/chromium/src/+/91f1e343eb3fcc6694c740548e29...
==========

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

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