Workaround rect-based paint invalidation on ancestor clip change

Workaround rect-based paint invalidation on ancestor clip change

BUG=490725
TEST=fast/repaint/clip-unclip-and-change.html

Committed: https://crrev.com/e4da87517db5b2dab98207f7c4b3049a6b7e44a8
Cr-Commit-Position: refs/heads/master@{#351239}

[Xianzhu, 2015-09-28 16:10:25]

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

[chrishtr, 2015-09-28 17:38:14]

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

https://codereview.chromium.org/1368163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:385: &&
previousPaintInvalidationRect != this->previousPaintInvalidationRect()
If the rect changes, why didn't invalidatePaintIfNeeded return a paint
invalidation reason other than None?

[Xianzhu, 2015-09-28 17:49:05]

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

https://codereview.chromium.org/1368163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:385: &&
previousPaintInvalidationRect != this->previousPaintInvalidationRect()
On 2015/09/28 17:38:13, chrishtr wrote:
> If the rect changes, why didn't invalidatePaintIfNeeded return a paint
> invalidation reason other than None?

These condition are for the ancestor whose clip changed. The condition checking
paint invalidation rect is approximate to check for clip change (we don't have
enough information to check for clip change). If an object has clip and its
paint invalidation rect changed, it's likely that its clip changed. This is not
perfect, but catches some bug cases as a workaround. If the conditions are true,
we force descendants to update their paint invalidation rects.

[chrishtr, 2015-09-28 18:28:22]

On 2015/09/28 at 17:49:05, wangxianzhu wrote:
>
https://codereview.chromium.org/1368163002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):
> 
>
https://codereview.chromium.org/1368163002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:385: &&
previousPaintInvalidationRect != this->previousPaintInvalidationRect()
> On 2015/09/28 17:38:13, chrishtr wrote:
> > If the rect changes, why didn't invalidatePaintIfNeeded return a paint
> > invalidation reason other than None?
> 
> These condition are for the ancestor whose clip changed. The condition
checking paint invalidation rect is approximate to check for clip change (we
don't have enough information to check for clip change). If an object has clip
and its paint invalidation rect changed, it's likely that its clip changed. This
is not perfect, but catches some bug cases as a workaround. If the conditions
are true, we force descendants to update their paint invalidation rects.

What would you say is the proper solution to this problem in SP phase 2? Compare
unclipped rects?

[Xianzhu, 2015-09-28 19:38:30]

On 2015/09/28 18:28:22, chrishtr wrote:
> On 2015/09/28 at 17:49:05, wangxianzhu wrote:
> >
>
https://codereview.chromium.org/1368163002/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1368163002/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:385: &&
> previousPaintInvalidationRect != this->previousPaintInvalidationRect()
> > On 2015/09/28 17:38:13, chrishtr wrote:
> > > If the rect changes, why didn't invalidatePaintIfNeeded return a paint
> > > invalidation reason other than None?
> > 
> > These condition are for the ancestor whose clip changed. The condition
> checking paint invalidation rect is approximate to check for clip change (we
> don't have enough information to check for clip change). If an object has clip
> and its paint invalidation rect changed, it's likely that its clip changed.
This
> is not perfect, but catches some bug cases as a workaround. If the conditions
> are true, we force descendants to update their paint invalidation rects.
> 
> What would you say is the proper solution to this problem in SP phase 2?
Compare
> unclipped rects?

SPv2 paint invalidation geometry checking will be for LayoutBoxs and
LayoutSVGModelObjects only, comparing border boxes, etc.

We are comparing paint invalidation rects to check paint invalidation just
because rect-based paint invalidation needs to save previous paint invalidation
rect. We still ned many other logics to deal with the cases that paint
invalidation rects are not suitable, e.g. the logic in LayoutBox to compare
border boxes, etc.

[chrishtr, 2015-09-28 21:22:05]

On 2015/09/28 at 19:38:30, wangxianzhu wrote:
> On 2015/09/28 18:28:22, chrishtr wrote:
> > On 2015/09/28 at 17:49:05, wangxianzhu wrote:
> > >
> >
https://codereview.chromium.org/1368163002/diff/20001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp
(right):
> > > 
> > >
> >
https://codereview.chromium.org/1368163002/diff/20001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:385: &&
> > previousPaintInvalidationRect != this->previousPaintInvalidationRect()
> > > On 2015/09/28 17:38:13, chrishtr wrote:
> > > > If the rect changes, why didn't invalidatePaintIfNeeded return a paint
> > > > invalidation reason other than None?
> > > 
> > > These condition are for the ancestor whose clip changed. The condition
> > checking paint invalidation rect is approximate to check for clip change (we
> > don't have enough information to check for clip change). If an object has
clip
> > and its paint invalidation rect changed, it's likely that its clip changed.
This
> > is not perfect, but catches some bug cases as a workaround. If the
conditions
> > are true, we force descendants to update their paint invalidation rects.
> > 
> > What would you say is the proper solution to this problem in SP phase 2?
Compare
> > unclipped rects?
> 
> SPv2 paint invalidation geometry checking will be for LayoutBoxs and
LayoutSVGModelObjects only, comparing border boxes, etc.

i.e propose it to be checking the bounds and offset of the LayoutBox exactly,
without ancestor clipping? So the implementation will simply be to, during
paint, check whether paint offset or size
of the LayoutObject has changed, and if so invalidate the cache?


> 
> We are comparing paint invalidation rects to check paint invalidation just
because rect-based paint invalidation needs to save previous paint invalidation
rect. We still ned many other logics to deal with the cases that paint
invalidation rects are not suitable, e.g. the logic in LayoutBox to compare
border boxes, etc.

Can you pllease link to the code you are referring to with the "logic in
LayoutBox to compare border boxes" comment?

[chrishtr, 2015-09-28 22:16:29]

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

https://codereview.chromium.org/1368163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:385: &&
previousPaintInvalidationRect != this->previousPaintInvalidationRect()
On 2015/09/28 at 17:49:05, Xianzhu wrote:
> On 2015/09/28 17:38:13, chrishtr wrote:
> > If the rect changes, why didn't invalidatePaintIfNeeded return a paint
> > invalidation reason other than None?
> 
> These condition are for the ancestor whose clip changed. The condition
checking paint invalidation rect is approximate to check for clip change (we
don't have enough information to check for clip change). If an object has clip
and its paint invalidation rect changed, it's likely that its clip changed. This
is not perfect, but catches some bug cases as a workaround. If the conditions
are true, we force descendants to update their paint invalidation rects.

Ok, but in what situation is invalidatePaintIfNeeded going to return
PaintInvalidationReasonNone yet the conditionals you're adding hold?
If something other than PaintInvalidationReasonNone is returned, why
compare the rects?

[Xianzhu, 2015-09-28 22:56:26]

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

https://codereview.chromium.org/1368163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:385: &&
previousPaintInvalidationRect != this->previousPaintInvalidationRect()
On 2015/09/28 22:16:29, chrishtr wrote:
> On 2015/09/28 at 17:49:05, Xianzhu wrote:
> > On 2015/09/28 17:38:13, chrishtr wrote:
> > > If the rect changes, why didn't invalidatePaintIfNeeded return a paint
> > > invalidation reason other than None?
> > 
> > These condition are for the ancestor whose clip changed. The condition
> checking paint invalidation rect is approximate to check for clip change (we
> don't have enough information to check for clip change). If an object has clip
> and its paint invalidation rect changed, it's likely that its clip changed.
This
> is not perfect, but catches some bug cases as a workaround. If the conditions
> are true, we force descendants to update their paint invalidation rects.
> 
> Ok, but in what situation is invalidatePaintIfNeeded going to return
> PaintInvalidationReasonNone yet the conditionals you're adding hold?
> If something other than PaintInvalidationReasonNone is returned, why
> compare the rects?

Let's take the added layout test as an example:

<div id="container" style="position: absolute; top: 100px; width: 100px; height:
10px; overflow: hidden">
  <div style="height: 20px"></div>
  <div style="width: 100px; height: 300px">
    <div id="content" style="width: 100px; height: 300px; background-color:
red"></div>
  </div>
</div>

Previously, when 'container' size changes, 'content' will skip paint
invalidation because neither style nor layout changes so it has no paint
invalidation flag set, leaving its previousPaintInvalidaitonRect incorrect and
causing problems of the next paint invalidation. There is no under-invalidation
of rects or objects.

With the CL, 'container' meets the conditions here, and force all descendants to
update their paint invalidation rects. Because change of the paint invalidation
rects is caused by overflow clip of 'container', the descendants don't need to
invalidate either rect or object. They just need to update
previousPaintInvalidationRect for the next paint invalidation.

This bug can be also fixed with synchronized painting which will remove
rect-based paint invalidation.

[chrishtr, 2015-09-28 23:38:13]

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

https://codereview.chromium.org/1368163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:385: &&
previousPaintInvalidationRect != this->previousPaintInvalidationRect()
On 2015/09/28 at 22:56:26, Xianzhu wrote:
> On 2015/09/28 22:16:29, chrishtr wrote:
> > On 2015/09/28 at 17:49:05, Xianzhu wrote:
> > > On 2015/09/28 17:38:13, chrishtr wrote:
> > > > If the rect changes, why didn't invalidatePaintIfNeeded return a paint
> > > > invalidation reason other than None?
> > > 
> > > These condition are for the ancestor whose clip changed. The condition
> > checking paint invalidation rect is approximate to check for clip change (we
> > don't have enough information to check for clip change). If an object has
clip
> > and its paint invalidation rect changed, it's likely that its clip changed.
This
> > is not perfect, but catches some bug cases as a workaround. If the
conditions
> > are true, we force descendants to update their paint invalidation rects.
> > 
> > Ok, but in what situation is invalidatePaintIfNeeded going to return
> > PaintInvalidationReasonNone yet the conditionals you're adding hold?
> > If something other than PaintInvalidationReasonNone is returned, why
> > compare the rects?
> 
> Let's take the added layout test as an example:
> 
> <div id="container" style="position: absolute; top: 100px; width: 100px;
height: 10px; overflow: hidden">
>   <div style="height: 20px"></div>
>   <div style="width: 100px; height: 300px">
>     <div id="content" style="width: 100px; height: 300px; background-color:
red"></div>
>   </div>
> </div>
> 
> Previously, when 'container' size changes, 'content' will skip paint
invalidation because neither style nor layout changes so it has no paint
invalidation flag set, leaving its previousPaintInvalidaitonRect incorrect and
causing problems of the next paint invalidation. There is no under-invalidation
of rects or objects.
> 
> With the CL, 'container' meets the conditions here, and force all descendants
to update their paint invalidation rects. Because change of the paint
invalidation rects is caused by overflow clip of 'container', the descendants
don't need to invalidate either rect or object. They just need to update
previousPaintInvalidationRect for the next paint invalidation.
> 
> This bug can be also fixed with synchronized painting which will remove
rect-based paint invalidation.

This all sounds great, and makes sense. What I don't understand yet is why you
aren't simply
checking whether the paint invalidation reason here for the container is any
non-None value, and instead trying to compare the rects. Is it because you're
trying to optimize the situation where the container changes box decorations to
not trigger this logic?

[Xianzhu, 2015-09-28 23:51:48]

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

https://codereview.chromium.org/1368163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:385: &&
previousPaintInvalidationRect != this->previousPaintInvalidationRect()
On 2015/09/28 23:38:13, chrishtr wrote:

> This all sounds great, and makes sense. What I don't understand yet is why you
> aren't simply
> checking whether the paint invalidation reason here for the container is any
> non-None value, and instead trying to compare the rects. Is it because you're
> trying to optimize the situation where the container changes box decorations
to
> not trigger this logic?

I see your question now :)

Simple style change (e.g. background) causes non-None paint invalidation reason
but we don't need to traverse the descendants. I'm afraid making this condition
too loose may degrade performance.

[chrishtr, 2015-09-28 23:57:54]

On 2015/09/28 at 23:51:48, wangxianzhu wrote:
>
https://codereview.chromium.org/1368163002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):
> 
>
https://codereview.chromium.org/1368163002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:385: &&
previousPaintInvalidationRect != this->previousPaintInvalidationRect()
> On 2015/09/28 23:38:13, chrishtr wrote:
> 
> > This all sounds great, and makes sense. What I don't understand yet is why
you
> > aren't simply
> > checking whether the paint invalidation reason here for the container is any
> > non-None value, and instead trying to compare the rects. Is it because
you're
> > trying to optimize the situation where the container changes box decorations
to
> > not trigger this logic?
> 
> I see your question now :)
> 
> Simple style change (e.g. background) causes non-None paint invalidation
reason but we don't need to traverse the descendants. I'm afraid making this
condition too loose may degrade performance.

Hmm. Ok.

[chrishtr, 2015-09-28 23:57:59]

lgtm

[Xianzhu, 2015-09-29 00:26:49]

Let's land this to fix the bug first before invalidation-rect-after-paint works.

[Xianzhu, 2015-09-29 00:26:55]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2015-09-29 00:28:29]

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

[commit-bot: I haz the power, 2015-09-29 02:25:51]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-09-29 02:26:51]

Patchset 2 (id:??) landed as
https://crrev.com/e4da87517db5b2dab98207f7c4b3049a6b7e44a8
Cr-Commit-Position: refs/heads/master@{#351239}