Add flag for tracking descendant paint property updates

third_party/WebKit/Source/core/layout/LayoutObject.h

Index: third_party/WebKit/Source/core/layout/LayoutObject.h
diff --git a/third_party/WebKit/Source/core/layout/LayoutObject.h b/third_party/WebKit/Source/core/layout/LayoutObject.h
index 8478f86e332c7f6041d231b7552db5f38e8c24dc..061ef652c3601955383c94d41e771573959f3202 100644
--- a/third_party/WebKit/Source/core/layout/LayoutObject.h
+++ b/third_party/WebKit/Source/core/layout/LayoutObject.h
@@ -1707,6 +1707,12 @@ class CORE_EXPORT LayoutObject : public ImageResourceObserver,
     void clearNeedsPaintPropertyUpdate() {
       m_layoutObject.clearNeedsPaintPropertyUpdate();
     }
+    void setDescendantNeedsPaintPropertyUpdate() {

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

Why do these need to be exposed publicly? Shouldn't setNeedsPaintPropertyUpdate
just call this on the parent?

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

Good catch, was blindly following the original code.

Privatized in the latest patch.

clearDescendantNeedsPaintPropertyUpdate does need to be there though.

+      m_layoutObject.setDescendantNeedsPaintPropertyUpdate();
+    }
+    void clearDescendantNeedsPaintPropertyUpdate() {
+      m_layoutObject.clearDescendantNeedsPaintPropertyUpdate();
+    }
 
    protected:
     friend class PaintPropertyTreeBuilder;
@@ -1734,16 +1740,36 @@ class CORE_EXPORT LayoutObject : public ImageResourceObserver,
   // state (location, transform, etc) as well as properties from ancestors.
   // When these inputs change, setNeedsPaintPropertyUpdate will cause a property
   // tree update during the next document lifecycle update.
-  // TODO(pdr): Add additional granularity such as the ability to signal that
-  // only a local paint property update is needed.
+  //
+  // In addition to tracking if an object needs its own paint properties
+  // updated, |descendantNeedsPaintPropertyUpdate| is used to track if any
+  // descendant needs an update too. This bit is up the tree, crossing frames.
+  //
+  // TODO(pdr): We may want to add more granular paint property update types
+  // such as the ability to only update one property type.
   void setNeedsPaintPropertyUpdate() {
     m_bitfields.setNeedsPaintPropertyUpdate(true);
+    // TODO(pdr): paintInvalidationParent is not inlined. May want to optimize
+    // this to check m_parent first.

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

I suspect if the TODO would work because even if m_parent is nullptr you can't
skip paintInvalidationParent().

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

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

+    if (auto* parent = paintInvalidationParent())
+      parent->setDescendantNeedsPaintPropertyUpdate();
+  }
+  void setDescendantNeedsPaintPropertyUpdate() {
+    if (!m_bitfields.descendantNeedsPaintPropertyUpdate()) {
+      m_bitfields.setDescendantNeedsPaintPropertyUpdate(true);
+      // TODO(pdr): paintInvalidationParent is not inlined. May want to optimize
+      // this to check m_parent first.
+      if (auto* parent = paintInvalidationParent())
+        parent->setDescendantNeedsPaintPropertyUpdate();
+    } else {
+      auto* parent = paintInvalidationParent();
+      DCHECK(!parent || parent->descendantNeedsPaintPropertyUpdate());
+    }
   }

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

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.

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

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

-  // TODO(pdr): This can be removed once we have more granular update flags.
   void setAllAncestorsNeedPaintPropertyUpdate() {
-    if (m_parent) {
-      m_parent->setNeedsPaintPropertyUpdate();
-      m_parent->setAllAncestorsNeedPaintPropertyUpdate();
+    if (auto* parent = paintInvalidationParent()) {
+      parent->setNeedsPaintPropertyUpdate();
+      parent->setAllAncestorsNeedPaintPropertyUpdate();
     }
   }

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

Where will this function be used?

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

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().

   void clearNeedsPaintPropertyUpdate() {
@@ -1753,6 +1779,13 @@ class CORE_EXPORT LayoutObject : public ImageResourceObserver,
   bool needsPaintPropertyUpdate() const {
     return m_bitfields.needsPaintPropertyUpdate();
   }
+  void clearDescendantNeedsPaintPropertyUpdate() {
+    DCHECK_EQ(document().lifecycle().state(), DocumentLifecycle::InPrePaint);
+    m_bitfields.setDescendantNeedsPaintPropertyUpdate(false);
+  }
+  bool descendantNeedsPaintPropertyUpdate() const {
+    return m_bitfields.descendantNeedsPaintPropertyUpdate();
+  }
 
   void setIsScrollAnchorObject() { m_bitfields.setIsScrollAnchorObject(true); }
   // Clears the IsScrollAnchorObject bit if and only if no ScrollAnchors still
@@ -2149,6 +2182,7 @@ class CORE_EXPORT LayoutObject : public ImageResourceObserver,
           m_hasPreviousSelectionVisualRect(false),
           m_hasPreviousBoxGeometries(false),
           m_needsPaintPropertyUpdate(true),
+          m_descendantNeedsPaintPropertyUpdate(true),
           m_backgroundChangedSinceLastPaintInvalidation(false),
           m_positionedState(IsStaticallyPositioned),
           m_selectionState(SelectionNone),
@@ -2321,13 +2355,17 @@ class CORE_EXPORT LayoutObject : public ImageResourceObserver,
     // Whether the paint properties need to be updated. For more details, see
     // LayoutObject::needsPaintPropertyUpdate().
     ADD_BOOLEAN_BITFIELD(needsPaintPropertyUpdate, NeedsPaintPropertyUpdate);
+    // Whether the paint properties of a descendant need to be updated. For more
+    // details, see LayoutObject::descendantNeedsPaintPropertyUpdate().
+    ADD_BOOLEAN_BITFIELD(descendantNeedsPaintPropertyUpdate,
+                         DescendantNeedsPaintPropertyUpdate);
 
     ADD_BOOLEAN_BITFIELD(backgroundChangedSinceLastPaintInvalidation,
                          BackgroundChangedSinceLastPaintInvalidation);
 
    protected:
     // Use protected to avoid warning about unused variable.
-    unsigned m_unusedBits : 8;
+    unsigned m_unusedBits : 7;
 
    private:
     // This is the cached 'position' value of this object