Check subtree paint invalidation when overflow clip status changes

Check subtree paint invalidation when overflow clip status changes

Before https://crrev.com/392652 because we included descendant overflow
into container overflow and the paint invalidation rect covered the
changed part of descendants.

Call setNeedsPaintInvalidationSubtree() to force checking of paint
invalidation of descendants when overflow clip status changes.

The other changes are needed because we need to move
clearPaintInvalidationFlags() after
newPaintInvalidationState.updateForChildren().

Several overriding invalidateTreeIfNeeded() methods in subclasses are
removed because they have been the same as inherited ones after we
changed the paint invalidation tree walk order.

BUG=613475
TEST=paint/invalidation/overflow-hidden-to-visible.html
TEST=paint/invalidation/overflow-visible-to-hidden.html

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

[Xianzhu, 2016-05-20 23:42:01]

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

[chrishtr, 2016-05-23 20:10:32]

https://codereview.chromium.org/2000763003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (left):

https://codereview.chromium.org/2000763003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBlock.cpp:248: for (LayoutObject*
child = firstChild(); child; child = child->nextSibling())
Even before https://crrev.com/392652 it was a bug though, right? Just harder to
exhibit?

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

https://codereview.chromium.org/2000763003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:418:
clearPaintInvalidationFlags(newPaintInvalidationState);
What does this have to do with the fix here?

https://codereview.chromium.org/2000763003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGInline.cpp (left):

https://codereview.chromium.org/2000763003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/LayoutSVGInline.cpp:146: void
LayoutSVGInline::invalidateTreeIfNeeded(const PaintInvalidationState&
paintInvalidationState)
This looks different than LayoutBoxModelObject::invalidateTreeIfNeeded, which it
inherits from.

https://codereview.chromium.org/2000763003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGText.cpp (left):

https://codereview.chromium.org/2000763003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/LayoutSVGText.cpp:397: void
LayoutSVGText::invalidateTreeIfNeeded(const PaintInvalidationState&
paintInvalidationState)
Ditto.

[Xianzhu, 2016-05-23 21:35:50]

https://codereview.chromium.org/2000763003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (left):

https://codereview.chromium.org/2000763003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBlock.cpp:248: for (LayoutObject*
child = firstChild(); child; child = child->nextSibling())
On 2016/05/23 20:10:32, chrishtr wrote:
> Even before https://crrev.com/392652 it was a bug though, right? Just harder
to
> exhibit?

I think so, but couldn't create a test case to fail before 392652.

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

https://codereview.chromium.org/2000763003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:418:
clearPaintInvalidationFlags(newPaintInvalidationState);
On 2016/05/23 20:10:32, chrishtr wrote:
> What does this have to do with the fix here?

Now we check m_currentObject.mayNeedPaintInvalidationSubtree() in
PaintInvalidaitonState::updateForChildren(), so we must clear the paint
invalidation flags  after updateForChildren().

https://codereview.chromium.org/2000763003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGInline.cpp (left):

https://codereview.chromium.org/2000763003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/LayoutSVGInline.cpp:146: void
LayoutSVGInline::invalidateTreeIfNeeded(const PaintInvalidationState&
paintInvalidationState)
On 2016/05/23 20:10:32, chrishtr wrote:
> This looks different than LayoutBoxModelObject::invalidateTreeIfNeeded, which
it
> inherits from.

I noticed this but thought LayoutBoxModelObject::invalidateTreeIfNeeded() is
also applicable to LayoutSVGInline. However, I defer this change for more
verifications. In the new patch this method calls
LayoutObject::invalidateTreeIfNeeded().

[chrishtr, 2016-05-23 21:44:00]

On 2016/05/23 at 21:35:50, wangxianzhu wrote:
>
https://codereview.chromium.org/2000763003/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (left):
> 
>
https://codereview.chromium.org/2000763003/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/LayoutBlock.cpp:248: for (LayoutObject*
child = firstChild(); child; child = child->nextSibling())
> On 2016/05/23 20:10:32, chrishtr wrote:
> > Even before https://crrev.com/392652 it was a bug though, right? Just harder
to
> > exhibit?
> 
> I think so, but couldn't create a test case to fail before 392652.
> 
>
https://codereview.chromium.org/2000763003/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):
> 
>
https://codereview.chromium.org/2000763003/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:418:
clearPaintInvalidationFlags(newPaintInvalidationState);
> On 2016/05/23 20:10:32, chrishtr wrote:
> > What does this have to do with the fix here?
> 
> Now we check m_currentObject.mayNeedPaintInvalidationSubtree() in
PaintInvalidaitonState::updateForChildren(), so we must clear the paint
invalidation flags  after updateForChildren().

Since you had to modify all subclass that override invalidateTreeIfNeeded, would
it be cleaner to put the code in each of those subclasses rather than
PaintInvalidationState?
That would make it maybe more in line with how the same forcing flag is set for
other reasons in LayoutBoxModelObject.

> 
>
https://codereview.chromium.org/2000763003/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/layout/svg/LayoutSVGInline.cpp (left):
> 
>
https://codereview.chromium.org/2000763003/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/svg/LayoutSVGInline.cpp:146: void
LayoutSVGInline::invalidateTreeIfNeeded(const PaintInvalidationState&
paintInvalidationState)
> On 2016/05/23 20:10:32, chrishtr wrote:
> > This looks different than LayoutBoxModelObject::invalidateTreeIfNeeded,
which it
> > inherits from.
> 
> I noticed this but thought LayoutBoxModelObject::invalidateTreeIfNeeded() is
also applicable to LayoutSVGInline. However, I defer this change for more
verifications. In the new patch this method calls
LayoutObject::invalidateTreeIfNeeded().

[chrishtr, 2016-05-23 21:44:00]

On 2016/05/23 at 21:35:50, wangxianzhu wrote:
>
https://codereview.chromium.org/2000763003/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (left):
> 
>
https://codereview.chromium.org/2000763003/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/LayoutBlock.cpp:248: for (LayoutObject*
child = firstChild(); child; child = child->nextSibling())
> On 2016/05/23 20:10:32, chrishtr wrote:
> > Even before https://crrev.com/392652 it was a bug though, right? Just harder
to
> > exhibit?
> 
> I think so, but couldn't create a test case to fail before 392652.
> 
>
https://codereview.chromium.org/2000763003/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):
> 
>
https://codereview.chromium.org/2000763003/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:418:
clearPaintInvalidationFlags(newPaintInvalidationState);
> On 2016/05/23 20:10:32, chrishtr wrote:
> > What does this have to do with the fix here?
> 
> Now we check m_currentObject.mayNeedPaintInvalidationSubtree() in
PaintInvalidaitonState::updateForChildren(), so we must clear the paint
invalidation flags  after updateForChildren().

Since you had to modify all subclass that override invalidateTreeIfNeeded, would
it be cleaner to put the code in each of those subclasses rather than
PaintInvalidationState?
That would make it maybe more in line with how the same forcing flag is set for
other reasons in LayoutBoxModelObject.

> 
>
https://codereview.chromium.org/2000763003/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/layout/svg/LayoutSVGInline.cpp (left):
> 
>
https://codereview.chromium.org/2000763003/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/svg/LayoutSVGInline.cpp:146: void
LayoutSVGInline::invalidateTreeIfNeeded(const PaintInvalidationState&
paintInvalidationState)
> On 2016/05/23 20:10:32, chrishtr wrote:
> > This looks different than LayoutBoxModelObject::invalidateTreeIfNeeded,
which it
> > inherits from.
> 
> I noticed this but thought LayoutBoxModelObject::invalidateTreeIfNeeded() is
also applicable to LayoutSVGInline. However, I defer this change for more
verifications. In the new patch this method calls
LayoutObject::invalidateTreeIfNeeded().

[Xianzhu, 2016-05-23 22:35:54]

On 2016/05/23 21:44:00, chrishtr wrote:
> On 2016/05/23 at 21:35:50, wangxianzhu wrote:
> >
>
https://codereview.chromium.org/2000763003/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (left):
> > 
> >
>
https://codereview.chromium.org/2000763003/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/layout/LayoutBlock.cpp:248: for
(LayoutObject*
> child = firstChild(); child; child = child->nextSibling())
> > On 2016/05/23 20:10:32, chrishtr wrote:
> > > Even before https://crrev.com/392652 it was a bug though, right? Just
harder
> to
> > > exhibit?
> > 
> > I think so, but couldn't create a test case to fail before 392652.
> > 
> >
>
https://codereview.chromium.org/2000763003/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2000763003/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:418:
> clearPaintInvalidationFlags(newPaintInvalidationState);
> > On 2016/05/23 20:10:32, chrishtr wrote:
> > > What does this have to do with the fix here?
> > 
> > Now we check m_currentObject.mayNeedPaintInvalidationSubtree() in
> PaintInvalidaitonState::updateForChildren(), so we must clear the paint
> invalidation flags  after updateForChildren().
> 
> Since you had to modify all subclass that override invalidateTreeIfNeeded,
would
> it be cleaner to put the code in each of those subclasses rather than
> PaintInvalidationState?
> That would make it maybe more in line with how the same forcing flag is set
for
> other reasons in LayoutBoxModelObject.
> 

Done.

[Xianzhu, 2016-05-23 22:36:50]

On 2016/05/23 22:35:54, Xianzhu wrote:
> On 2016/05/23 21:44:00, chrishtr wrote:
> > On 2016/05/23 at 21:35:50, wangxianzhu wrote:
> > >
> >
>
https://codereview.chromium.org/2000763003/diff/1/third_party/WebKit/Source/c...
> > > File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (left):
> > > 
> > >
> >
>
https://codereview.chromium.org/2000763003/diff/1/third_party/WebKit/Source/c...
> > > third_party/WebKit/Source/core/layout/LayoutBlock.cpp:248: for
> (LayoutObject*
> > child = firstChild(); child; child = child->nextSibling())
> > > On 2016/05/23 20:10:32, chrishtr wrote:
> > > > Even before https://crrev.com/392652 it was a bug though, right? Just
> harder
> > to
> > > > exhibit?
> > > 
> > > I think so, but couldn't create a test case to fail before 392652.
> > > 
> > >
> >
>
https://codereview.chromium.org/2000763003/diff/1/third_party/WebKit/Source/c...
> > > File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp
(right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2000763003/diff/1/third_party/WebKit/Source/c...
> > > third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:418:
> > clearPaintInvalidationFlags(newPaintInvalidationState);
> > > On 2016/05/23 20:10:32, chrishtr wrote:
> > > > What does this have to do with the fix here?
> > > 
> > > Now we check m_currentObject.mayNeedPaintInvalidationSubtree() in
> > PaintInvalidaitonState::updateForChildren(), so we must clear the paint
> > invalidation flags  after updateForChildren().
> > 
> > Since you had to modify all subclass that override invalidateTreeIfNeeded,
> would
> > it be cleaner to put the code in each of those subclasses rather than
> > PaintInvalidationState?
> > That would make it maybe more in line with how the same forcing flag is set
> for
> > other reasons in LayoutBoxModelObject.
> > 
> 
> Done.

Sorry, encountered some problem during upload. Investigating.

[Xianzhu, 2016-05-23 22:41:08]

On 2016/05/23 22:36:50, Xianzhu wrote:
> On 2016/05/23 22:35:54, Xianzhu wrote:
> > On 2016/05/23 21:44:00, chrishtr wrote:
> > > On 2016/05/23 at 21:35:50, wangxianzhu wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2000763003/diff/1/third_party/WebKit/Source/c...
> > > > File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (left):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2000763003/diff/1/third_party/WebKit/Source/c...
> > > > third_party/WebKit/Source/core/layout/LayoutBlock.cpp:248: for
> > (LayoutObject*
> > > child = firstChild(); child; child = child->nextSibling())
> > > > On 2016/05/23 20:10:32, chrishtr wrote:
> > > > > Even before https://crrev.com/392652 it was a bug though, right? Just
> > harder
> > > to
> > > > > exhibit?
> > > > 
> > > > I think so, but couldn't create a test case to fail before 392652.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2000763003/diff/1/third_party/WebKit/Source/c...
> > > > File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp
> (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2000763003/diff/1/third_party/WebKit/Source/c...
> > > > third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:418:
> > > clearPaintInvalidationFlags(newPaintInvalidationState);
> > > > On 2016/05/23 20:10:32, chrishtr wrote:
> > > > > What does this have to do with the fix here?
> > > > 
> > > > Now we check m_currentObject.mayNeedPaintInvalidationSubtree() in
> > > PaintInvalidaitonState::updateForChildren(), so we must clear the paint
> > > invalidation flags  after updateForChildren().
> > > 
> > > Since you had to modify all subclass that override invalidateTreeIfNeeded,
> > would
> > > it be cleaner to put the code in each of those subclasses rather than
> > > PaintInvalidationState?
> > > That would make it maybe more in line with how the same forcing flag is
set
> > for
> > > other reasons in LayoutBoxModelObject.
> > > 
> > 
> > Done.
> 
> Sorry, encountered some problem during upload. Investigating.

Successfully uploaded.

[chrishtr, 2016-05-23 22:46:03]

lgtm

[Xianzhu, 2016-05-23 22:49:26]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2016-05-23 22:50:17]

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

[commit-bot: I haz the power, 2016-05-24 00:50:17]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-24 00:51:47]

Description was changed from

==========
Check subtree paint invalidation when overflow clip status changes

Before https://crrev.com/392652 because we included descendant overflow
into container overflow and the paint invalidation rect covered the
changed part of descendants.

Call setNeedsPaintInvalidationSubtree() to force checking of paint
invalidation of descendants when overflow clip status changes.

The other changes are needed because we need to move
clearPaintInvalidationFlags() after
newPaintInvalidationState.updateForChildren().

Several overriding invalidateTreeIfNeeded() methods in subclasses are
removed because they have been the same as inherited ones after we
changed the paint invalidation tree walk order.

BUG=613475
TEST=paint/invalidation/overflow-hidden-to-visible.html
TEST=paint/invalidation/overflow-visible-to-hidden.html
==========

to

==========
Check subtree paint invalidation when overflow clip status changes

Before https://crrev.com/392652 because we included descendant overflow
into container overflow and the paint invalidation rect covered the
changed part of descendants.

Call setNeedsPaintInvalidationSubtree() to force checking of paint
invalidation of descendants when overflow clip status changes.

The other changes are needed because we need to move
clearPaintInvalidationFlags() after
newPaintInvalidationState.updateForChildren().

Several overriding invalidateTreeIfNeeded() methods in subclasses are
removed because they have been the same as inherited ones after we
changed the paint invalidation tree walk order.

BUG=613475
TEST=paint/invalidation/overflow-hidden-to-visible.html
TEST=paint/invalidation/overflow-visible-to-hidden.html

Committed: https://crrev.com/d35533844c94bece904fa16ca47acf43e659cb14
Cr-Commit-Position: refs/heads/master@{#395491}
==========

[commit-bot: I haz the power, 2016-05-24 00:51:48]

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