Invalidate non-self-painting-layer descendants when creating a layer

Invalidate non-self-painting-layer descendants when creating a layer

When a layer is created, its descendants may change paint invalidation
container. For example, previously the descendants had a containing
layer which is composited but was not a stacking context, and now their
paint invalidation container becomes the nearest stacking context
ancestor.

The real bug status is hard to reproduce with a test because it depends
on reuse of display item client addresses (deleted display item client
not invalidated on the correct paint invalidation container, then a new
display item client is created at the same address which will be treated
as validly cached).

BUG=541544
TEST=paint/invalidation/invalidate-descendants-when-receiving-paint-layer.html

Committed: https://crrev.com/84891953f9681beb9ae099e69b5403aa182f4522
Cr-Commit-Position: refs/heads/master@{#354202}

[Xianzhu, 2015-10-13 17:41:08]

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

[chrishtr, 2015-10-13 17:49:01]

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

https://codereview.chromium.org/1403963002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:3441: void
LayoutObject::invalidatePaintIncludingNonLayerDescendantsInternal(const
LayoutBoxModelObject& paintInvalidationContainer)
NonPaintLayer

https://codereview.chromium.org/1403963002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:3446: if
(!child->hasLayer())
What about non-self-painting layers?

[Xianzhu, 2015-10-13 23:58:12]

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

https://codereview.chromium.org/1403963002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:3441: void
LayoutObject::invalidatePaintIncludingNonLayerDescendantsInternal(const
LayoutBoxModelObject& paintInvalidationContainer)
On 2015/10/13 17:49:01, chrishtr wrote:
> NonPaintLayer

Changed to invalidatePaintIncludingNonSelfPaintingLayerDescendants

https://codereview.chromium.org/1403963002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:3446: if
(!child->hasLayer())
On 2015/10/13 17:49:00, chrishtr wrote:
> What about non-self-painting layers?

We should also invalidate them. Done.

https://codereview.chromium.org/1403963002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):

https://codereview.chromium.org/1403963002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:290:
invalidatePaintIncludingNonSelfPaintingLayerDescendants(*currentPaintInvalidationContainer);
Conditions are made stricter here to avoid duplicated invalidations and avoid
potential performance issues when creating large number of layers.

Paint invalidation container changes caused by compositing update are already
handled by CompositingLayerMapping. Here we handle paint invalidation container
change not covered by compositing update only.

[chrishtr, 2015-10-14 17:43:00]

https://codereview.chromium.org/1403963002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):

https://codereview.chromium.org/1403963002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:287: if
(!RuntimeEnabledFeatures::slimmingPaintV2Enabled() && isRooted() &&
styleRef().isTreatedAsOrStackingContext()) {
If an element gets a paint layer but isTreatedAsOrStackingContext is false,
can't it still change paint invalidation container and require eager
invalidation?

https://codereview.chromium.org/1403963002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1403963002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:3443: if
(!child->hasLayer() ||
!toLayoutBoxModelObject(child)->layer()->isSelfPaintingLayer())
Also non-compositing right? Non-self-painting paint layers can get composited
layers I think.

[Xianzhu, 2015-10-14 18:17:48]

https://codereview.chromium.org/1403963002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):

https://codereview.chromium.org/1403963002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:287: if
(!RuntimeEnabledFeatures::slimmingPaintV2Enabled() && isRooted() &&
styleRef().isTreatedAsOrStackingContext()) {
On 2015/10/14 17:43:00, chrishtr wrote:
> If an element gets a paint layer but isTreatedAsOrStackingContext is false,
> can't it still change paint invalidation container and require eager
> invalidation?

If isTreatedAsOrStackingContext is false, its paint invalidation container is in
the chain of parent->stackingContextAncestor->..., which won't be affected by
creation of this layer.

https://codereview.chromium.org/1403963002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1403963002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:3441:
setPreviousPaintInvalidationRect(LayoutRect());
I removed this line (and line 3411 above) because it caused wrong invalidation
when the object is removed later.

https://codereview.chromium.org/1403963002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:3443: if
(!child->hasLayer() ||
!toLayoutBoxModelObject(child)->layer()->isSelfPaintingLayer())
On 2015/10/14 17:43:00, chrishtr wrote:
> Also non-compositing right? Non-self-painting paint layers can get composited
> layers I think.

I think all composited layers are self-painting, otherwise we'll paint them
twice on the parent composited layer (by layout object painter walk) and its own
composited layer (by CompositedLayerMapping::paintContents()).
PaintLayer::shouldBeSelfPaintingLayer() seems to ensure this.

https://codereview.chromium.org/1403963002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/platform/linux/fast/layers/scroll-descendant-with-cached-cliprects-expected.txt
(left):

https://codereview.chromium.org/1403963002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/platform/linux/fast/layers/scroll-descendant-with-cached-cliprects-expected.txt:17:
[0, 0, 100, 100]
This was wrong. The original invalidation code in
LayoutBoxModelObject::createLayer() invalidates the current
boundsRectForPaintInvalidation() of the object. We should invalidate the
previous paint invalidation rect instead. There is no new invalidation because
the new conditions exclude this case from the potential reasons of paint
invalidation container change.

[chrishtr, 2015-10-14 18:22:32]

https://codereview.chromium.org/1403963002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):

https://codereview.chromium.org/1403963002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:288: if (const
LayoutBoxModelObject* currentPaintInvalidationContainer =
containerForPaintInvalidation()) {
Re testing: can you build on the test in
https://codereview.chromium.org/1337233002 by adding some block-flow
descendants?

[Xianzhu, 2015-10-14 19:47:21]

https://codereview.chromium.org/1403963002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):

https://codereview.chromium.org/1403963002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:288: if (const
LayoutBoxModelObject* currentPaintInvalidationContainer =
containerForPaintInvalidation()) {
On 2015/10/14 18:22:32, chrishtr wrote:
> Re testing: can you build on the test in
> https://codereview.chromium.org/1337233002 by adding some block-flow
> descendants?

Done.

[chrishtr, 2015-10-14 19:49:15]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2015-10-14 19:49:15]

lgtm

[commit-bot: I haz the power, 2015-10-14 19:50:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1403963002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1403963002/100001

[commit-bot: I haz the power, 2015-10-14 21:21:53]

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

[commit-bot: I haz the power, 2015-10-14 21:21:54]

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

[Xianzhu, 2015-10-14 22:03:19]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2015-10-14 22:03:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/1403963002/#ps120001
(title: "Fix layout test (a background made difference)")

[commit-bot: I haz the power, 2015-10-14 22:03:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1403963002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1403963002/120001

[commit-bot: I haz the power, 2015-10-14 23:05:04]

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

[commit-bot: I haz the power, 2015-10-14 23:05:05]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Xianzhu, 2015-10-15 00:50:12]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2015-10-15 00:50:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/1403963002/#ps130001
(title: "Add spv2 expectation for the test")

[commit-bot: I haz the power, 2015-10-15 00:51:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1403963002/130001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1403963002/130001

[commit-bot: I haz the power, 2015-10-15 02:13:17]

Committed patchset #8 (id:130001)

[commit-bot: I haz the power, 2015-10-15 02:14:07]

Patchset 8 (id:??) landed as
https://crrev.com/84891953f9681beb9ae099e69b5403aa182f4522
Cr-Commit-Position: refs/heads/master@{#354202}