Implement incremental paint property tree rebuilding

third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp

Index: third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp
diff --git a/third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp b/third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp
index 83ebd785bdce76b3621a14e01a18a264c8db71fe..886b2cee6bf5aaab041221d6ea94493146afcbd3 100644
--- a/third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp
+++ b/third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp
@@ -20,6 +20,7 @@ struct PrePaintTreeWalkContext {
         paintInvalidatorContext(treeBuilderContext,
                                 parentContext.paintInvalidatorContext) {}
 
+  bool needToUpdatePaintPropertySubtree = false;
   PaintPropertyTreeBuilderContext treeBuilderContext;
   PaintInvalidatorContext paintInvalidatorContext;
 };
@@ -41,8 +42,26 @@ void PrePaintTreeWalk::walk(FrameView& frameView,
     return;
 
   PrePaintTreeWalkContext localContext(context);
-  m_propertyTreeBuilder.buildTreeNodes(frameView,
-                                       localContext.treeBuilderContext);
+
+  // Check whether we need to update the paint property trees.
+  if (!localContext.needToUpdatePaintPropertySubtree) {
+    if (context.paintInvalidatorContext.forcedSubtreeInvalidationFlags) {
+      // forcedSubtreeInvalidationFlags will be true if locations have changed
+      // which will affect paint properties (e.g., PaintOffset).
+      localContext.needToUpdatePaintPropertySubtree = true;
+    } else if (frameView.needsPaintPropertyUpdate()) {
+      // Paint properties can depend on their ancestor properties so ensure
+      // the entire subtree is rebuilt on any changes.
+      // TODO(pdr): Add additional granularity to the needs update approach
+      // such as the ability to do local updates that don't change the subtree.
+      localContext.needToUpdatePaintPropertySubtree = true;
+    }
+  }
+  if (localContext.needToUpdatePaintPropertySubtree)
+    frameView.setNeedsPaintPropertyUpdate();
+
+  m_propertyTreeBuilder.updateProperties(frameView,
+                                         localContext.treeBuilderContext);
 
   if (RuntimeEnabledFeatures::slimmingPaintInvalidationEnabled()) {
     m_paintInvalidator.invalidatePaintIfNeeded(
@@ -56,12 +75,39 @@ void PrePaintTreeWalk::walk(FrameView& frameView,
   if (RuntimeEnabledFeatures::slimmingPaintInvalidationEnabled())
     frameView.layoutView()->assertSubtreeClearedPaintInvalidationFlags();
 #endif
+
+  frameView.clearNeedsPaintPropertyUpdate();
 }
 
 void PrePaintTreeWalk::walk(const LayoutObject& object,
                             const PrePaintTreeWalkContext& context) {
   PrePaintTreeWalkContext localContext(context);
 
+  // Check whether we need to update the paint property trees.
+  if (!localContext.needToUpdatePaintPropertySubtree) {
+    if (context.paintInvalidatorContext.forcedSubtreeInvalidationFlags) {
+      // forcedSubtreeInvalidationFlags will be true if locations have changed
+      // which will affect paint properties (e.g., PaintOffset).
+      localContext.needToUpdatePaintPropertySubtree = true;
+    } else if (object.needsPaintPropertyUpdate()) {
+      // Paint properties can depend on their ancestor properties so ensure
+      // the entire subtree is rebuilt on any changes.
+      // TODO(pdr): Add additional granularity to the needs update approach
+      // such as the ability to do local updates that don't change the subtree.
+      localContext.needToUpdatePaintPropertySubtree = true;
+    } else if (
+        object
+            .shouldCheckForPaintInvalidationRegardlessOfPaintInvalidationState()) {
+      // TODO(pdr): Better document the exact reason we need to check this
+      // paint invalidation state.
+      localContext.needToUpdatePaintPropertySubtree = true;

[Xianzhu, 2016/10/26 19:21:51]

I think we don't need to set needToUpdatePaintPropertySubtree but just
object.setNeedsPaintPropertyUpdate().

About your question in https://codereview.chromium.org/2404213004/#msg13, I
think we can use mayNeedpaintInvalidation() || shouldDoFullPaintInvalidation()
here to exclude the cases that the object needs checking paint invalidation just
because of having selection and/or childShouldCheckForPaintInvalidation().

[pdr., 2016/10/26 23:10:19]

Initially I'd like to rebuild entire subtrees anytime
object.needsPaintPropertyUpdate is set. This way we don't yet have to worry
about descendant properties that point to stale properties on this object. I'd
then like to improve this in the future. Does that sound reasonable to you, or
do you think this should be thought out more before landing parts of this patch?

I have two ideas for improving setNeedsPaintPropertyUpdate:
* We could introduce a local-only (or even transform-only, clip-only, etc) bit
such as setNeedsLocalPaintPropertyUpdate that does not rebuild the entire
subtree. This would be for the case when a transform changes, for example.
* Another option is to only rebuild a property subtree if nodes change. If just
the values of property nodes change (as opposed to new ones added or deleted),
all child nodes will still be valid and we do not need to rebuild the subtree at
all.

Does that sound like a good idea? Thoughts welcome.
Updated! Can you take another look at this conditional and see if my comments
can be improved?

[Xianzhu, 2016/10/26 23:19:40]

SGTM.

(I overlooked the previous needToUpdatePaintPropertySubtree = true and
misunderstood the above code when writing the previous comment.)

+    }
+  }
+  if (localContext.needToUpdatePaintPropertySubtree)
+    object.getMutableForPainting().setNeedsPaintPropertyUpdate();
+
+  // TODO(pdr): Ensure multi column works with incremental property tree
+  // construction.
   if (object.isLayoutMultiColumnSpannerPlaceholder()) {
     // Walk multi-column spanner as if it replaces the placeholder.
     // Set the flag so that the tree builder can specially handle out-of-flow
@@ -75,12 +121,14 @@ void PrePaintTreeWalk::walk(const LayoutObject& object,
     return;
   }
 
-  m_propertyTreeBuilder.buildTreeNodesForSelf(object,
-                                              localContext.treeBuilderContext);
+  m_propertyTreeBuilder.updatePropertiesForSelf(
+      object, localContext.treeBuilderContext);
+
   if (RuntimeEnabledFeatures::slimmingPaintInvalidationEnabled())
     m_paintInvalidator.invalidatePaintIfNeeded(
         object, localContext.paintInvalidatorContext);
-  m_propertyTreeBuilder.buildTreeNodesForChildren(
+
+  m_propertyTreeBuilder.updatePropertiesForChildren(
       object, localContext.treeBuilderContext);
 
   for (const LayoutObject* child = object.slowFirstChild(); child;
@@ -104,6 +152,8 @@ void PrePaintTreeWalk::walk(const LayoutObject& object,
     }
     // TODO(pdr): Investigate RemoteFrameView (crbug.com/579281).
   }
+
+  object.getMutableForPainting().clearNeedsPaintPropertyUpdate();
 }
 
 }  // namespace blink