Add flag for tracking descendant paint property updates

Add flag for tracking descendant paint property updates

This patch adds a flag for tracking whether a descendant needs a
paint property update. When marking an object as needing a paint
property update, we now set this flag for all ancestors. This flag
will be used to stop the paint property update (and, prepaint tree
walk) entirely if no descendant needs an update.

This flag is similar to layout's |normalChildNeedsLayout| flag, and
paint invalidation's |childShouldCheckForPaintInvalidation| flag.

BUG=645667
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/2459701934af4c8297ee26d351a1fbf9596c6bd5
Cr-Commit-Position: refs/heads/master@{#434080}

[pdr., 2016-11-21 19:10:50]

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

[pdr., 2016-11-21 19:11:01]

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

[commit-bot: I haz the power, 2016-11-21 19:11:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pdr., 2016-11-21 19:11:29]

For context when reviewing this, here is a proof-of-concept that uses this patch
to prune the prepaint tree walk: https://codereview.chromium.org/2515113002.
Here's the main doc describing this work:
https://docs.google.com/document/d/1_GkBfvameyhnLV7ODIRsOoTedQEG5liAcHwAxmwS-...

[chrishtr, 2016-11-21 19:23:03]

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

https://codereview.chromium.org/2521613003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutObject.h:1710: void
setDescendantNeedsPaintPropertyUpdate() {
Why do these need to be exposed publicly? Shouldn't setNeedsPaintPropertyUpdate
just call this on the parent?

[pdr., 2016-11-21 19:28:17]

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

https://codereview.chromium.org/2521613003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutObject.h:1710: void
setDescendantNeedsPaintPropertyUpdate() {
On 2016/11/21 at 19:23:03, chrishtr wrote:
> Why do these need to be exposed publicly? Shouldn't
setNeedsPaintPropertyUpdate
> just call this on the parent?

Good catch, was blindly following the original code.

Privatized in the latest patch.

clearDescendantNeedsPaintPropertyUpdate does need to be there though.

[Xianzhu, 2016-11-21 19:41:34]

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

https://codereview.chromium.org/2521613003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutObject.h:1753: // this to check
m_parent first.
I suspect if the TODO would work because even if m_parent is nullptr you can't
skip paintInvalidationParent().

https://codereview.chromium.org/2521613003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutObject.h:1768: }
Could the impl be moved into LayoutObject.cpp and modified to a non-recursive
version (like markAncestorsForPaintInvalidation())? In the markAncestorsXXX way
you could also avoid checking paintInvalidaitonParent in
setNeedsPaintPropertyUpdate().

Then paintInvalidation would be called only from LayoutObject.cpp, so it could
be marked inline in LayoutObject.cpp.

https://codereview.chromium.org/2521613003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutObject.h:1774: }
Where will this function be used?

https://codereview.chromium.org/2521613003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp
(right):

https://codereview.chromium.org/2521613003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:2618:
overflowA->layoutObject()->setAllAncestorsNeedPaintPropertyUpdate();
Isn't the above call redundant?

[pdr., 2016-11-22 02:55:35]

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

https://codereview.chromium.org/2521613003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutObject.h:1753: // this to check
m_parent first.
On 2016/11/21 at 19:41:34, Xianzhu wrote:
> I suspect if the TODO would work because even if m_parent is nullptr you can't
skip paintInvalidationParent().

I was thinking more about optimizing the case when m_parent is not null, but
your idea below about inlining handles this better.

https://codereview.chromium.org/2521613003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutObject.h:1768: }
On 2016/11/21 at 19:41:34, Xianzhu wrote:
> Could the impl be moved into LayoutObject.cpp and modified to a non-recursive
version (like markAncestorsForPaintInvalidation())? In the markAncestorsXXX way
you could also avoid checking paintInvalidaitonParent in
setNeedsPaintPropertyUpdate().
> 
> Then paintInvalidation would be called only from LayoutObject.cpp, so it could
be marked inline in LayoutObject.cpp.

Good ideas.

I modified your approach slightly:
* setNeedsPaintPropertyUpdate has been moved to to LayoutObject.cpp where it
sets the needsPaintProperty bit and non-recursively sets
descendantNeedsPaintPropertyUpdate on all ancestors.
* removed setDescendantNeedsPaintPropertyUpdate entirely.
* inlined paintInvalidationParent
* Added the non-inline slowPaintInvalidationParentForTesting for
VisualRectMappingTest.cpp

https://codereview.chromium.org/2521613003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutObject.h:1774: }
On 2016/11/21 at 19:41:34, Xianzhu wrote:
> Where will this function be used?

setAllAncestorsNeedPaintPropertyUpdate is used for the main thread scrolling
reasons which can require updating all the way up the tree. Specifically, it's
used in FrameView::addBackgroundAttachmentFixedObject and
FrameView::removeBackgroundAttachmentFixedObject.

For example:
 A
/ \
B C

If both B and C have background-attachment: fixed, (A, B, and C) will all have
main thread scrolling reasons. If B is changed to not have
background-attachment: fixed, only (B and C) should have main thread scrolling
reasons. If C is changed to not have background-attachment: fixed, nothing
should have main thread scrolling reasons. So, when changing B and C, we do need
to mark A as needing an update (not just having a child that needs an update).


To make it more clear what this is for, I've renamed
setAllAncestorsNeedPaintPropertyUpdate ->
setAncestorsNeedPaintPropertyUpdateForMainThreadScrolling and put a comment
describing this in more detail. I think this new name will also prevent people
from accidentally calling
setAncestorsNeedPaintPropertyUpdateForMainThreadScrolling() when they should be
calling setNeedsPaintPropertyUpdate().

https://codereview.chromium.org/2521613003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp
(right):

https://codereview.chromium.org/2521613003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:2618:
overflowA->layoutObject()->setAllAncestorsNeedPaintPropertyUpdate();
On 2016/11/21 at 19:41:34, Xianzhu wrote:
> Isn't the above call redundant?

This is not needed yet so I have removed it from this patch. It will be needed
in a future patch that prunes the prepaint tree walk though, because this test
will fail (see comment above for details).

[pdr., 2016-11-22 02:57:18]

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

[commit-bot: I haz the power, 2016-11-22 02:57:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-22 04:40:05]

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

[commit-bot: I haz the power, 2016-11-22 04:40:06]

Dry run: This issue passed the CQ dry run.

[Xianzhu, 2016-11-23 00:12:48]

lgtm

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

https://codereview.chromium.org/2521613003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.h:2065: LayoutObject*
slowPaintInvalidationParentForTesting() const;
I wonder if it works to omit 'inline' in header but use 'inline' on impl in .cc.
I saw several examples like that, e.g. clearLayoutRootIfNeeded(), but not sure
what the result is.

[pdr., 2016-11-23 01:53:30]

On 2016/11/23 at 00:12:48, wangxianzhu wrote:
> lgtm
> 
>
https://codereview.chromium.org/2521613003/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/LayoutObject.h (right):
> 
>
https://codereview.chromium.org/2521613003/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutObject.h:2065: LayoutObject*
slowPaintInvalidationParentForTesting() const;
> I wonder if it works to omit 'inline' in header but use 'inline' on impl in
.cc. I saw several examples like that, e.g. clearLayoutRootIfNeeded(), but not
sure what the result is.

I think this works if all callers are in the same compilation target, but it
doesn't for us because we're calling paintInvalidationParent from tests. With
inline in the impl but not in the header we get a linker error:
Undefined symbols for architecture x86_64:
  "blink::LayoutObject::paintInvalidationParent() const", referenced from:
     
blink::VisualRectMappingTest::checkPaintInvalidationStateRectMapping(blink::LayoutRect
const&, blink::LayoutRect const&, blink::LayoutObject const&, blink::LayoutView
const&, blink::LayoutObject const&) in VisualRectMappingTest.o

[pdr., 2016-11-23 01:53:53]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2016-11-23 01:54:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-23 01:59:26]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1479866033291240,
"parent_rev": "8c6fa55a4d23cb38121b4c1fede2b546698cccf7", "commit_rev":
"82b65af56811dba76fdbc55db103dfdbaf3b5040"}

[commit-bot: I haz the power, 2016-11-23 01:59:56]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-23 02:02:53]

Description was changed from

==========
Add flag for tracking descendant paint property updates

This patch adds a flag for tracking whether a descendant needs a
paint property update. When marking an object as needing a paint
property update, we now set this flag for all ancestors. This flag
will be used to stop the paint property update (and, prepaint tree
walk) entirely if no descendant needs an update.

This flag is similar to layout's |normalChildNeedsLayout| flag, and
paint invalidation's |childShouldCheckForPaintInvalidation| flag.

BUG=645667
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Add flag for tracking descendant paint property updates

This patch adds a flag for tracking whether a descendant needs a
paint property update. When marking an object as needing a paint
property update, we now set this flag for all ancestors. This flag
will be used to stop the paint property update (and, prepaint tree
walk) entirely if no descendant needs an update.

This flag is similar to layout's |normalChildNeedsLayout| flag, and
paint invalidation's |childShouldCheckForPaintInvalidation| flag.

BUG=645667
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/2459701934af4c8297ee26d351a1fbf9596c6bd5
Cr-Commit-Position: refs/heads/master@{#434080}
==========

[commit-bot: I haz the power, 2016-11-23 02:02:54]

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