Add flag for tracking descendant paint property updates

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

 /*
  * Copyright (C) 2000 Lars Knoll (knoll@kde.org)
  *           (C) 2000 Antti Koivisto (koivisto@kde.org)
  *           (C) 2000 Dirk Mueller (mueller@kde.org)
  *           (C) 2004 Allan Sandfeld Jensen (kde@carewolf.com)
  * Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2012 Apple Inc.
  *               All rights reserved.
  * Copyright (C) 2009 Google Inc. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
@@ skipping 1689 matching lines @@
     }
     void clearPreviousVisualRects() {
       m_layoutObject.clearPreviousVisualRects();
     }
     void setNeedsPaintPropertyUpdate() {
       m_layoutObject.setNeedsPaintPropertyUpdate();
     }
     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;
     // The following two functions can be called from PaintPropertyTreeBuilder
     // only.
     ObjectPaintProperties& ensurePaintProperties() {
       return m_layoutObject.ensurePaintProperties();
     }
     ObjectPaintProperties* paintProperties() {
       return const_cast<ObjectPaintProperties*>(
           m_layoutObject.paintProperties());
     }
 
     friend class LayoutObject;
     MutableForPainting(const LayoutObject& layoutObject)
         : m_layoutObject(const_cast<LayoutObject&>(layoutObject)) {}
 
     LayoutObject& m_layoutObject;
   };
   MutableForPainting getMutableForPainting() const {
     return MutableForPainting(*this);
   }
 
   // Paint properties (see: |ObjectPaintProperties|) are built from an object's
   // 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();
   }
-  // TODO(pdr): This can be removed once we have more granular update flags.
+  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

   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() {
     DCHECK_EQ(document().lifecycle().state(), DocumentLifecycle::InPrePaint);
     m_bitfields.setNeedsPaintPropertyUpdate(false);
   }
   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
   // reference this LayoutObject.
   void maybeClearIsScrollAnchorObject();
 
   bool scrollAnchorDisablingStyleChanged() {
     return m_bitfields.scrollAnchorDisablingStyleChanged();
   }
   void setScrollAnchorDisablingStyleChanged(bool changed) {
@@ skipping 376 matching lines @@
           m_alwaysCreateLineBoxesForLayoutInline(false),
           m_previousBackgroundObscured(false),
           m_isBackgroundAttachmentFixedObject(false),
           m_isScrollAnchorObject(false),
           m_scrollAnchorDisablingStyleChanged(false),
           m_hasBoxDecorationBackground(false),
           m_hasPreviousLocationInBacking(false),
           m_hasPreviousSelectionVisualRect(false),
           m_hasPreviousBoxGeometries(false),
           m_needsPaintPropertyUpdate(true),
+          m_descendantNeedsPaintPropertyUpdate(true),
           m_backgroundChangedSinceLastPaintInvalidation(false),
           m_positionedState(IsStaticallyPositioned),
           m_selectionState(SelectionNone),
           m_backgroundObscurationState(BackgroundObscurationStatusInvalid),
           m_fullPaintInvalidationReason(PaintInvalidationNone) {}
 
     // Self needs layout means that this layout object is marked for a full
     // layout. This is the default layout but it is expensive as it recomputes
     // everything. For CSS boxes, this includes the width (laying out the line
     // boxes again), the margins (due to block collapsing margins), the
@@ skipping 152 matching lines @@
 
     ADD_BOOLEAN_BITFIELD(hasPreviousLocationInBacking,
                          HasPreviousLocationInBacking);
     ADD_BOOLEAN_BITFIELD(hasPreviousSelectionVisualRect,
                          HasPreviousSelectionVisualRect);
     ADD_BOOLEAN_BITFIELD(hasPreviousBoxGeometries, HasPreviousBoxGeometries);
 
     // 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
     // (see ComputedStyle::position).
     unsigned m_positionedState : 2;  // PositionedState
     unsigned m_selectionState : 3;   // SelectionState
     // Mutable for getter which lazily update this field.
     mutable unsigned
         m_backgroundObscurationState : 2;        // BackgroundObscurationState
     unsigned m_fullPaintInvalidationReason : 5;  // PaintInvalidationReason
@@ skipping 304 matching lines @@
 CORE_EXPORT void showLineTree(const blink::LayoutObject*);
 CORE_EXPORT void showLayoutTree(const blink::LayoutObject* object1);
 // We don't make object2 an optional parameter so that showLayoutTree
 // can be called from gdb easily.
 CORE_EXPORT void showLayoutTree(const blink::LayoutObject* object1,
                                 const blink::LayoutObject* object2);
 
 #endif
 
 #endif  // LayoutObject_h