Ensure positioned descendants are invalidated if needed

Ensure positioned descendants are invalidated if needed

When a render object changes location, we should force paint
invalidation checking for all descendants on the same backing,
including positioned ones. However, previously we only force
invalidation checking for non-positioned descendants.

Now let RenderBlock override
RenderObject::invalidatePaintOfSubtreesIfNeeded() so that it
can use the childPaintInvalidationState, which has been forced
checking of invaldation, for the positioned descendants.

BUG=422812
TEST=fast/repaint/positioned-great-grandparent-change-location.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183910

[Xianzhu, 2014-10-15 21:59:06]

wangxianzhu@chromium.org changed reviewers:
+ dsinclair@chromium.org, jchaffraix@chromium.org

[dsinclair, 2014-10-16 13:32:37]

https://codereview.chromium.org/662483002/diff/1/LayoutTests/fast/repaint/pos...
File LayoutTests/fast/repaint/positioned-great-grandparent-change-location.html
(right):

https://codereview.chromium.org/662483002/diff/1/LayoutTests/fast/repaint/pos...
LayoutTests/fast/repaint/positioned-great-grandparent-change-location.html:9:
Tests repaint of the target when the great-grandparent changes location.
nits: s/repaint/invalidation

https://codereview.chromium.org/662483002/diff/1/LayoutTests/fast/repaint/pos...
LayoutTests/fast/repaint/positioned-great-grandparent-change-location.html:10:
Passes if finally there is only one green rectangle.
nit: remove finally

https://codereview.chromium.org/662483002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBlock.cpp (left):

https://codereview.chromium.org/662483002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlock.cpp:381: // we have to make sure we go through
any positioned objects as they won't be seen in
Since we no longer override invalidateTreeIfNeeded, the RenderObject version
will early out if !shouldCheckForInvalidationAfterLayout() is true. Which means,
won't we now miss the updating of positioned objects?

[Xianzhu, 2014-10-16 17:37:14]

https://codereview.chromium.org/662483002/diff/1/LayoutTests/fast/repaint/pos...
File LayoutTests/fast/repaint/positioned-great-grandparent-change-location.html
(right):

https://codereview.chromium.org/662483002/diff/1/LayoutTests/fast/repaint/pos...
LayoutTests/fast/repaint/positioned-great-grandparent-change-location.html:9:
Tests repaint of the target when the great-grandparent changes location.
On 2014/10/16 13:32:37, dsinclair wrote:
> nits: s/repaint/invalidation

Done.

https://codereview.chromium.org/662483002/diff/1/LayoutTests/fast/repaint/pos...
LayoutTests/fast/repaint/positioned-great-grandparent-change-location.html:10:
Passes if finally there is only one green rectangle.
On 2014/10/16 13:32:37, dsinclair wrote:
> nit: remove finally

Done.

https://codereview.chromium.org/662483002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBlock.cpp (left):

https://codereview.chromium.org/662483002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlock.cpp:381: // we have to make sure we go through
any positioned objects as they won't be seen in
On 2014/10/16 13:32:37, dsinclair wrote:
> Since we no longer override invalidateTreeIfNeeded, the RenderObject version
> will early out if !shouldCheckForInvalidationAfterLayout() is true. Which
means,
> won't we now miss the updating of positioned objects?

The previous logic walks 1 extra level into the positioned descendants for
RenderBlock which has no paint invalidation flag set.

With this change:
- If the RenderBlock did change location, we'll force invalidation checking for
descendants (non-positioned as well as positioned).
- Otherwise the positioned descendants should be only checked paint invalidation
if the RenderBlock has been laid out. In the case, we have set
mayNeedPaintInvalidation for the positioned descendants in
RenderBlock::layoutPositionedObjects().

[dsinclair, 2014-10-17 14:18:49]

LGTM. Is the failure of mac_blink_rel
editing/selection/deleteFromDocument-crash.html a flake or something to do with
this change?

https://storage.googleapis.com/chromium-layout-test-archives/mac_blink_rel/27...

[Xianzhu, 2014-10-17 17:20:25]

On 2014/10/17 14:18:49, dsinclair wrote:
> LGTM. Is the failure of mac_blink_rel
> editing/selection/deleteFromDocument-crash.html a flake or something to do
with
> this change?
> 
>
https://storage.googleapis.com/chromium-layout-test-archives/mac_blink_rel/27...

Retried and it still failed. Building on mac to reproduce.

[Xianzhu, 2014-10-17 17:29:41]

On 2014/10/17 17:20:25, Xianzhu wrote:
> On 2014/10/17 14:18:49, dsinclair wrote:
> > LGTM. Is the failure of mac_blink_rel
> > editing/selection/deleteFromDocument-crash.html a flake or something to do
> with
> > this change?
> > 
> >
>
https://storage.googleapis.com/chromium-layout-test-archives/mac_blink_rel/27...
> 
> Retried and it still failed. Building on mac to reproduce.

Based on flakiness dashboard, it's really flaky.
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=edi...

[Xianzhu, 2014-10-17 17:29:46]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2014-10-17 17:30:19]

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

[commit-bot: I haz the power, 2014-10-17 18:15:47]

Committed patchset #2 (id:20001) as 183910