Move scroll paint property nodes to be owned by the transform tree

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

Index: third_party/WebKit/Source/core/paint/PaintLayer.cpp
diff --git a/third_party/WebKit/Source/core/paint/PaintLayer.cpp b/third_party/WebKit/Source/core/paint/PaintLayer.cpp
index 168383e4077bc5da12b3a5d8b84841d30bb4efcc..8184373ef20c11ad6d0a94d4ec294c6fa8411c62 100644
--- a/third_party/WebKit/Source/core/paint/PaintLayer.cpp
+++ b/third_party/WebKit/Source/core/paint/PaintLayer.cpp
@@ -327,31 +327,44 @@ void PaintLayer::dirtyAncestorChainHasSelfPaintingLayerDescendantStatus() {
   }
 }
 
+static const ScrollPaintPropertyNode* nearestScrollNode(
+    const TransformPaintPropertyNode* transform) {
+  if (transform->isRoot())
+    return nullptr;
+  if (const auto* scrollNode = transform->scrollNode())
+    return scrollNode;
+  return nearestScrollNode(transform->parent());

[chrishtr, 2017/01/26 21:22:58]

Please make this an iteration instead of recursion, to avoid stack overflows.

[pdr., 2017/01/27 20:09:33]

Done.

I found a bug fixing this: the root transform node referenced a null scroll node
which allowed for the root of the scroll tree to be null if no scrolling
occurred. This will be fine when we remove the non-null root concept, but is
wrong now. I fixed this by making the root transform node reference the root
scroll node. This is tested in ScrollsWithViewportFixedPosition.

+}
+
 bool PaintLayer::sticksToViewport() const {
   if (layoutObject()->style()->position() != FixedPosition &&
       layoutObject()->style()->position() != StickyPosition)
     return false;
 
+  // TODO(pdr): This approach of calculating the nearest scroll node is O(n).
+  // An option for improving this is to cache the nearest scroll node in
+  // the local border box properties.
   if (RuntimeEnabledFeatures::slimmingPaintV2Enabled()) {
     const ScrollPaintPropertyNode* ancestorTargetScrollNode;
     if (layoutObject()->style()->position() == FixedPosition) {
-      ancestorTargetScrollNode = layoutObject()
-                                     ->view()
-                                     ->paintProperties()
-                                     ->localBorderBoxProperties()
-                                     ->scroll();
+      ancestorTargetScrollNode =
+          nearestScrollNode(layoutObject()

[pdr., 2017/01/26 20:04:54]

This part of the change is not entirely trivial and is a downside to the new
approach.

[wkorman, 2017/01/26 21:01:10]

Do existing tests cover this logic sufficiently or is it worth adding new
specifically for this?

[pdr., 2017/01/27 20:09:33]

I think we have pretty good coverage here. I wanted to fix this in a followup to
keep this patch simple, but found there were too many test failures. Some
examples:
PaintLayerTest.ScrollsWithViewportFixedPosition
PaintLayerTest.ScrollsWithViewportFixedPositionInsideTransform
PaintLayerTest.ScrollsWithViewportFixedPositionInsideTransformNoScroll

+                                ->view()
+                                ->paintProperties()
+                                ->localBorderBoxProperties()
+                                ->transform());
     } else {
-      ancestorTargetScrollNode = layoutObject()
-                                     ->view()
-                                     ->paintProperties()
-                                     ->contentsProperties()
-                                     ->scroll();
+      ancestorTargetScrollNode = nearestScrollNode(layoutObject()
+                                                       ->view()
+                                                       ->paintProperties()
+                                                       ->contentsProperties()
+                                                       ->transform());
     }
 
-    return layoutObject()
-               ->paintProperties()
-               ->localBorderBoxProperties()
-               ->scroll() == ancestorTargetScrollNode;
+    return nearestScrollNode(layoutObject()
+                                 ->paintProperties()
+                                 ->localBorderBoxProperties()
+                                 ->transform()) == ancestorTargetScrollNode;
   }
 
   return (layoutObject()->style()->position() == FixedPosition &&