Incrementally build main thread scrolling reasons [spv2]

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

Index: third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
diff --git a/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp b/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
index 2ca46226fd107407f6a2125a858841a3d2dd0040..d05d5374b45dfefa50cb86ca8688a51b14ccda4f 100644
--- a/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
+++ b/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
@@ -23,7 +23,6 @@ namespace blink {
 PaintPropertyTreeBuilderContext
 PaintPropertyTreeBuilder::setupInitialContext() {
   PaintPropertyTreeBuilderContext context;
-
   context.current.clip = context.absolutePosition.clip =
       context.fixedPosition.clip = ClipPaintPropertyNode::root();
   context.currentEffect = EffectPaintPropertyNode::root();
@@ -31,11 +30,6 @@ PaintPropertyTreeBuilder::setupInitialContext() {
       context.fixedPosition.transform = TransformPaintPropertyNode::root();
   context.current.scroll = context.absolutePosition.scroll =
       context.fixedPosition.scroll = ScrollPaintPropertyNode::root();
-
-  // Ensure scroll tree properties are reset. They will be rebuilt during the
-  // tree walk.
-  ScrollPaintPropertyNode::root()->clearMainThreadScrollingReasons();
-
   return context;
 }
 
@@ -84,21 +78,25 @@ void updateFrameViewScrollTranslation(
 
 void updateFrameViewScroll(

[Xianzhu, 2016/11/17 19:03:04]

Nit not necessarily to be addressed in this CL: we should put these global
functions into anonymous namespace or prefix them with 'static'. BTW, can we
also remove the declarations of private functions in PaintPropertyTreeBuilder.h
and make them also file-visibility functions?

[pdr., 2016/11/17 21:01:36]

Good ideas all around. I'll do this in a followup CL.

[pdr., 2016/11/17 22:30:03]

I started this in parallel and found out why we didn't do this: We use c++
friendship to allow PaintPropertyTreeBuilder to access MutableForPainting in
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La....
I think these have to be setup the way they are for that to work.

     FrameView& frameView,
-    PassRefPtr<ScrollPaintPropertyNode> parent,
+    PassRefPtr<const ScrollPaintPropertyNode> parent,
     PassRefPtr<const TransformPaintPropertyNode> scrollOffset,
     const IntSize& clip,
     const IntSize& bounds,
     bool userScrollableHorizontal,
-    bool userScrollableVertical) {
+    bool userScrollableVertical,
+    bool threadedScrollingDisabled,
+    bool hasBackgroundAttachmentFixedDescendants) {
   DCHECK(!RuntimeEnabledFeatures::rootLayerScrollingEnabled());
   if (auto* existingScroll = frameView.scroll()) {
     existingScroll->update(std::move(parent), std::move(scrollOffset), clip,
                            bounds, userScrollableHorizontal,
-                           userScrollableVertical);
+                           userScrollableVertical, threadedScrollingDisabled,
+                           hasBackgroundAttachmentFixedDescendants);
   } else {
     frameView.setScroll(ScrollPaintPropertyNode::create(
         std::move(parent), std::move(scrollOffset), clip, bounds,
-        userScrollableHorizontal, userScrollableVertical));
+        userScrollableHorizontal, userScrollableVertical,
+        threadedScrollingDisabled, hasBackgroundAttachmentFixedDescendants));
   }
 }
 
@@ -148,10 +146,15 @@ void PaintPropertyTreeBuilder::updateProperties(
           frameView.userInputScrollable(HorizontalScrollbar);
       bool userScrollableVertical =
           frameView.userInputScrollable(VerticalScrollbar);
+      bool threadedScrollingDisabled =
+          !frameView.frame().settings()->threadedScrollingEnabled();
+      bool hasBackgroundAttachmentFixedDescendants =
+          frameView.hasBackgroundAttachmentFixedObjects();

[Xianzhu, 2016/11/17 19:03:04]

Can the above code be moved into updateFrameViewScroll() to reduce the number of
parameters? It seems that we can just pass two parameters (frameView, context).

[pdr., 2016/11/17 21:01:36]

I'd prefer if we leave them here because this code matches the pattern used in
updateScrollAndScrollTranslation. Do you think that's okay?

Fortunately, most of the FrameView property code will go away when
RootLayerScrolls launches.

[Xianzhu, 2016/11/17 21:25:02]

Sounds good.

       updateFrameViewScroll(frameView, context.current.scroll,
                             frameView.scrollTranslation(), scrollClip,
                             scrollBounds, userScrollableHorizontal,
-                            userScrollableVertical);
+                            userScrollableVertical, threadedScrollingDisabled,
+                            hasBackgroundAttachmentFixedDescendants);
     } else {
       // Ensure pre-existing properties are cleared when there is no scrolling.
       frameView.setScrollTranslation(nullptr);
@@ -172,7 +175,7 @@ void PaintPropertyTreeBuilder::updateProperties(
   context.current.clip = frameView.contentClip();
   if (const auto* scrollTranslation = frameView.scrollTranslation())
     context.current.transform = scrollTranslation;
-  if (auto* scroll = frameView.scroll())
+  if (const auto* scroll = frameView.scroll())
     context.current.scroll = scroll;
   context.current.paintOffset = LayoutPoint();
   context.current.renderingContextID = 0;
@@ -502,32 +505,6 @@ void PaintPropertyTreeBuilder::updateScrollbarPaintOffset(
     properties->clearScrollbarPaintOffset();
 }
 
-void PaintPropertyTreeBuilder::updateMainThreadScrollingReasons(
-    const LayoutObject& object,
-    PaintPropertyTreeBuilderContext& context) {
-  // TODO(pdr): Mark properties as needing an update for main thread scroll
-  // reasons and ensure reason changes are propagated to ancestors to account
-  // for the parent walk below. https://crbug.com/664672.
-
-  if (context.current.scroll &&
-      !object.document().settings()->threadedScrollingEnabled()) {
-    context.current.scroll->addMainThreadScrollingReasons(
-        MainThreadScrollingReason::kThreadedScrollingDisabled);
-  }
-
-  if (object.isBackgroundAttachmentFixedObject()) {
-    auto* scrollNode = context.current.scroll;
-    while (
-        scrollNode &&
-        !scrollNode->hasMainThreadScrollingReasons(
-            MainThreadScrollingReason::kHasBackgroundAttachmentFixedObjects)) {
-      scrollNode->addMainThreadScrollingReasons(
-          MainThreadScrollingReason::kHasBackgroundAttachmentFixedObjects);
-      scrollNode = scrollNode->parent();
-    }
-  }
-}
-
 void PaintPropertyTreeBuilder::updateOverflowClip(
     const LayoutObject& object,
     PaintPropertyTreeBuilderContext& context) {
@@ -675,10 +652,21 @@ void PaintPropertyTreeBuilder::updateScrollAndScrollTranslation(
             scrollableArea->userInputScrollable(HorizontalScrollbar);
         bool userScrollableVertical =
             scrollableArea->userInputScrollable(VerticalScrollbar);
+        bool threadedScrollingDisabled =
+            !object.document().settings()->threadedScrollingEnabled();
+        // Checking for descendants in the layout tree has two downsides:
+        // 1) There can be more descendants in layout order than in paint
+        //    order (e.g., fixed position objects).
+        // 2) Iterating overall all background attachment fixed objects for
+        //    every scroll node can be slow, though there will be no objects
+        //    in the common case.
+        bool hasBackgroundAttachmentFixedDescendants =
+            object.frameView()->hasBackgroundAttachmentFixedDescendants(object);
         object.getMutableForPainting().ensurePaintProperties().updateScroll(
             context.current.scroll,
             object.paintProperties()->scrollTranslation(), scrollClip,
-            scrollBounds, userScrollableHorizontal, userScrollableVertical);
+            scrollBounds, userScrollableHorizontal, userScrollableVertical,
+            threadedScrollingDisabled, hasBackgroundAttachmentFixedDescendants);
       } else {
         // Ensure pre-existing properties are cleared when there is no
         // scrolling.
@@ -693,9 +681,7 @@ void PaintPropertyTreeBuilder::updateScrollAndScrollTranslation(
 
   if (object.paintProperties() && object.paintProperties()->scroll()) {
     context.current.transform = object.paintProperties()->scrollTranslation();
-    const auto* scroll = object.paintProperties()->scroll();
-    // TODO(pdr): Remove this const cast.
-    context.current.scroll = const_cast<ScrollPaintPropertyNode*>(scroll);
+    context.current.scroll = object.paintProperties()->scroll();
     context.current.shouldFlattenInheritedTransform = false;
   }
 }
@@ -711,7 +697,7 @@ void PaintPropertyTreeBuilder::updateOutOfFlowContext(
   if (object.isLayoutView()) {
     if (RuntimeEnabledFeatures::rootLayerScrollingEnabled()) {
       const auto* initialFixedTransform = context.fixedPosition.transform;
-      auto* initialFixedScroll = context.fixedPosition.scroll;
+      const auto* initialFixedScroll = context.fixedPosition.scroll;
 
       context.fixedPosition = context.current;
 
@@ -787,8 +773,7 @@ static void overrideContaineringBlockContextFromRealContainingBlock(
   context.renderingContextID =
       context.transform ? context.transform->renderingContextID() : 0;
   context.clip = properties->propertyTreeState.clip();
-  context.scroll = const_cast<ScrollPaintPropertyNode*>(
-      properties->propertyTreeState.scroll());
+  context.scroll = properties->propertyTreeState.scroll();
 }
 
 static void deriveBorderBoxFromContainerContext(
@@ -897,7 +882,6 @@ void PaintPropertyTreeBuilder::updatePropertiesForSelf(
   updateCssClip(object, context);
   updateLocalBorderBoxContext(object, context);
   updateScrollbarPaintOffset(object, context);
-  updateMainThreadScrollingReasons(object, context);
 }
 
 void PaintPropertyTreeBuilder::updatePropertiesForChildren(