Traverse into child LayoutViews during invalidateTreeIfNeeded

Traverse into child LayoutViews during invalidateTreeIfNeeded

If a LayoutView's paint invalidation container is not itself, we should
pass cachedOffsetsEnabled and forceCheckForPaintInvalidation flags in
parent's PaintInvalidationState to the LayoutView, otherwise the layout
view may miss required checking of paint invalidation, or calculate
cached offsets when not feasible.

BUG=441671
TEST=fast/repaint/in-scaled-iframe.html

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

[Xianzhu, 2015-06-25 21:36:33]

wangxianzhu@chromium.org changed reviewers:
+ chrishtr@chromium.org, dsinclair@chromium.org, leviw@chromium.org

[Xianzhu, 2015-06-26 16:11:04]

ping...

[chrishtr, 2015-06-26 23:32:47]

https://codereview.chromium.org/1209133002/diff/1/Source/core/layout/LayoutPa...
File Source/core/layout/LayoutPart.cpp (right):

https://codereview.chromium.org/1209133002/diff/1/Source/core/layout/LayoutPa...
Source/core/layout/LayoutPart.cpp:338: if
(!childLayoutView->isPaintInvalidationContainer()) {
Why can't you just pass down these parameters in the PaintInvalidationState
itself?

[Xianzhu, 2015-06-27 00:17:15]

https://codereview.chromium.org/1209133002/diff/1/Source/core/layout/LayoutPa...
File Source/core/layout/LayoutPart.cpp (right):

https://codereview.chromium.org/1209133002/diff/1/Source/core/layout/LayoutPa...
Source/core/layout/LayoutPart.cpp:338: if
(!childLayoutView->isPaintInvalidationContainer()) {
On 2015/06/26 23:32:46, chrishtr wrote:
> Why can't you just pass down these parameters in the PaintInvalidationState
> itself?

We don't walk into the sub frames during a paint invalidation of a frame. The
FrameView recursively call LayoutView::invalidateTreeIfNeeded() for each frame
each with a new PaintInvalidationState. This CL let the new
PaintInvalidationStates get the states saved during the parent frame's
invalidateTreeIfNeeded().

Another way might be to let LayoutView::invalidateTreeIfNeeded() walk into sub
frames, and mark ancestor paint invalidation flags across frame boundaries. I'm
not sure how risky it is to do this.

[chrishtr, 2015-06-29 17:42:28]

https://codereview.chromium.org/1209133002/diff/1/Source/core/layout/LayoutPa...
File Source/core/layout/LayoutPart.cpp (right):

https://codereview.chromium.org/1209133002/diff/1/Source/core/layout/LayoutPa...
Source/core/layout/LayoutPart.cpp:338: if
(!childLayoutView->isPaintInvalidationContainer()) {
On 2015/06/27 at 00:17:15, Xianzhu wrote:
> On 2015/06/26 23:32:46, chrishtr wrote:
> > Why can't you just pass down these parameters in the PaintInvalidationState
> > itself?
> 
> We don't walk into the sub frames during a paint invalidation of a frame. The
FrameView recursively call LayoutView::invalidateTreeIfNeeded() for each frame
each with a new PaintInvalidationState. This CL let the new
PaintInvalidationStates get the states saved during the parent frame's
invalidateTreeIfNeeded().
> 
> Another way might be to let LayoutView::invalidateTreeIfNeeded() walk into sub
frames, and mark ancestor paint invalidation flags across frame boundaries. I'm
not sure how risky it is to do this.

This does seem a cleaner approach. To make it easier/safer, maybe just
FrameView::invalidateTreeIfNeeded() into LayoutView so you can call it from
here?

[Xianzhu, 2015-06-29 22:58:09]

On 2015/06/29 17:42:28, chrishtr wrote:
>
https://codereview.chromium.org/1209133002/diff/1/Source/core/layout/LayoutPa...
> File Source/core/layout/LayoutPart.cpp (right):
> 
>
https://codereview.chromium.org/1209133002/diff/1/Source/core/layout/LayoutPa...
> Source/core/layout/LayoutPart.cpp:338: if
> (!childLayoutView->isPaintInvalidationContainer()) {
> On 2015/06/27 at 00:17:15, Xianzhu wrote:
> > On 2015/06/26 23:32:46, chrishtr wrote:
> > > Why can't you just pass down these parameters in the
PaintInvalidationState
> > > itself?
> > 
> > We don't walk into the sub frames during a paint invalidation of a frame.
The
> FrameView recursively call LayoutView::invalidateTreeIfNeeded() for each frame
> each with a new PaintInvalidationState. This CL let the new
> PaintInvalidationStates get the states saved during the parent frame's
> invalidateTreeIfNeeded().
> > 
> > Another way might be to let LayoutView::invalidateTreeIfNeeded() walk into
sub
> frames, and mark ancestor paint invalidation flags across frame boundaries.
I'm
> not sure how risky it is to do this.
> 
> This does seem a cleaner approach. To make it easier/safer, maybe just
> FrameView::invalidateTreeIfNeeded() into LayoutView so you can call it from
> here?

Done. Ptal.

[chrishtr, 2015-06-29 23:32:22]

https://codereview.chromium.org/1209133002/diff/20001/Source/core/frame/Frame...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1209133002/diff/20001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:1105: void
FrameView::invalidateTreeIfNeeded(PaintInvalidationState&
paintInvalidationState)
Do you need to clean out some of these fields if inherited from the parent
frame?

https://codereview.chromium.org/1209133002/diff/20001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:1107: // FIXME: We should be more aggressive at
cutting tree traversals.
Change to TODO(wangxianzhu)

https://codereview.chromium.org/1209133002/diff/20001/Source/core/frame/Frame...
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/1209133002/diff/20001/Source/core/frame/Frame...
Source/core/frame/FrameView.h:559: void
invalidateTreeIfNeeded(PaintInvalidationState&);
Add a note that you shouldn't add a new top-level caller to this that is not for
the root frame of a frame tree.

https://codereview.chromium.org/1209133002/diff/20001/Source/core/layout/Layo...
File Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1209133002/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutObject.cpp:2345: LayoutObject*
LayoutObject::containerAcrossFrameBoundary() const
containerCrossingFrameBoundaries to match
enclosingLayerForPaintInvalidationCrossingFrameBoundaries.

https://codereview.chromium.org/1209133002/diff/20001/Source/core/layout/Layo...
File Source/core/layout/LayoutPart.h (right):

https://codereview.chromium.org/1209133002/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutPart.h:65: virtual void
invalidatePaintOfSubtreesIfNeeded(PaintInvalidationState&) override;
invalidatePaintOfWidgetIfNeeded()

[Xianzhu, 2015-06-30 00:02:13]

https://codereview.chromium.org/1209133002/diff/20001/Source/core/frame/Frame...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1209133002/diff/20001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:1105: void
FrameView::invalidateTreeIfNeeded(PaintInvalidationState&
paintInvalidationState)
On 2015/06/29 23:32:22, chrishtr wrote:
> Do you need to clean out some of these fields if inherited from the parent
> frame?

No. This paintInvalidationState should be just for the LayoutView, properly
prepared by the caller.

https://codereview.chromium.org/1209133002/diff/20001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:1107: // FIXME: We should be more aggressive at
cutting tree traversals.
On 2015/06/29 23:32:22, chrishtr wrote:
> Change to TODO(wangxianzhu)

This is moved from invalidateTreeIfNeededRecursively. Actually I don't know what
it says. I will delete it :)

https://codereview.chromium.org/1209133002/diff/20001/Source/core/frame/Frame...
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/1209133002/diff/20001/Source/core/frame/Frame...
Source/core/frame/FrameView.h:559: void
invalidateTreeIfNeeded(PaintInvalidationState&);
On 2015/06/29 23:32:22, chrishtr wrote:
> Add a note that you shouldn't add a new top-level caller to this that is not
for
> the root frame of a frame tree.

In the new patch set I keep it private, and add 'friend class LayoutPart'.

https://codereview.chromium.org/1209133002/diff/20001/Source/core/layout/Layo...
File Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1209133002/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutObject.cpp:2345: LayoutObject*
LayoutObject::containerAcrossFrameBoundary() const
On 2015/06/29 23:32:22, chrishtr wrote:
> containerCrossingFrameBoundaries to match
> enclosingLayerForPaintInvalidationCrossingFrameBoundaries.

Done.

https://codereview.chromium.org/1209133002/diff/20001/Source/core/layout/Layo...
File Source/core/layout/LayoutPart.h (right):

https://codereview.chromium.org/1209133002/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutPart.h:65: virtual void
invalidatePaintOfSubtreesIfNeeded(PaintInvalidationState&) override;
On 2015/06/29 23:32:22, chrishtr wrote:
> invalidatePaintOfWidgetIfNeeded()

This overrides LayoutObject::invalidatePaintOfSubtreesIfNeeded().

Added a comment here.

https://codereview.chromium.org/1209133002/diff/40001/Source/core/frame/Frame...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1209133002/diff/40001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:2611:
childFrameView->invalidateTreeIfNeeded(childPaintInvalidationState);
This is a new complication. Covered by unit tests.

[Xianzhu, 2015-06-30 21:53:27]

ping...

[Xianzhu, 2015-07-01 19:52:21]

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

[commit-bot: I haz the power, 2015-07-01 19:52:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1209133002/100001

[Xianzhu, 2015-07-01 19:53:06]

Chris, could you take another look at this CL? Thanks.

[chrishtr, 2015-07-01 19:56:51]

https://codereview.chromium.org/1209133002/diff/100001/Source/core/frame/Fram...
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/1209133002/diff/100001/Source/core/frame/Fram...
Source/core/frame/FrameView.h:602: friend class LayoutPart; // For
invalidateTreeIfNeeded.
We don't do that elsewhere, since friend classes can access all private methods,
which might lead to a worse outcome. I think it's better to leave it public and
put a note.

[Xianzhu, 2015-07-01 20:10:46]

https://codereview.chromium.org/1209133002/diff/100001/Source/core/frame/Fram...
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/1209133002/diff/100001/Source/core/frame/Fram...
Source/core/frame/FrameView.h:602: friend class LayoutPart; // For
invalidateTreeIfNeeded.
On 2015/07/01 19:56:51, chrishtr wrote:
> We don't do that elsewhere, since friend classes can access all private
methods,
> which might lead to a worse outcome. I think it's better to leave it public
and
> put a note.

Done.

[chrishtr, 2015-07-01 21:11:52]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2015-07-01 21:11:53]

lgtm

[commit-bot: I haz the power, 2015-07-01 21:12:09]

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

[commit-bot: I haz the power, 2015-07-01 21:30:33]

Committed patchset #7 (id:100002) as
https://src.chromium.org/viewvc/blink?view=rev&revision=198169