Add compositor element id for scroll nodes in 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 0b176f6ac0a85e3ac2d03ac8c26e95ed4dd85595..4d2130e4e244adcc5fa27acb2919582655525f0e 100644
--- a/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
+++ b/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
@@ -86,6 +86,12 @@ static bool updateScrollTranslation(
   return true;
 }
 
+static CompositorElementId createDomNodeBasedCompositorElementId(

[wkorman, 2017/01/19 23:43:10]

Moved here without changes so that it's in scope for new call from static method
below.

+    const LayoutObject& object) {
+  return createCompositorElementId(DOMNodeIds::idForNode(object.node()),
+                                   CompositorSubElementId::Primary);
+}
+
 // True if a new property was created or a main thread scrolling reason changed
 // (which can affect descendants), false if an existing one was updated.
 static bool updateScroll(
@@ -98,17 +104,25 @@ static bool updateScroll(
     bool userScrollableVertical,
     MainThreadScrollingReasons mainThreadScrollingReasons) {
   DCHECK(!RuntimeEnabledFeatures::rootLayerScrollingEnabled());
+
+  // TODO(wkorman): Should we generate compositor element id conditionally on

[wkorman, 2017/01/19 23:43:10]

Seeking feedback here. The general things I'm uncertain of though of course I
did research briefly:

- am I missing any ScrollPaintPropertyNode create/update calls?
- am I building the compositor element id from the correct layout object?
- are we assured that these are only created for composited scrolling cases? If
not, what should we be checking?

If we get either of these wrong we can correct in subsequent change but worth
considering now.

[pdr., 2017/01/20 06:26:32]

Nope, you got the two.
This is the non-RLS codepath so it's for FrameView scrolling. I'm really not
sure what Element a FrameView would have.
These are currently created for composited and non-composited scrollers, and
will be in the future too. With SPV2's design, we won't know until
PaintArtifactCompositor which scrollers actually end up being composited. Is
there any harm in setting this id if it isn't used?
+1

[ajuma, 2017/01/20 14:41:57]

The only downside I can think of is that each DOMNodeId we generate gets
inserted into a hash map (see
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/DOMNo...),
so that hash map gets a bit bigger. So just something to keep in mind as a
possible performance issue we might want to look into later.

+  // something that connotes use of composited scrolling? Or pass it in as a
+  // param similar to scrolling reasons? See also similar comment in
+  // PaintPropertyTreeBuilder::updateScrollAndScrollTranslation.
+  CompositorElementId compositorElementId =
+      createDomNodeBasedCompositorElementId(*frameView.layoutView());
   if (auto* existingScroll = frameView.scroll()) {
     auto existingReasons = existingScroll->mainThreadScrollingReasons();
     existingScroll->update(std::move(parent), std::move(scrollOffset), clip,
                            bounds, userScrollableHorizontal,
-                           userScrollableVertical, mainThreadScrollingReasons);
+                           userScrollableVertical, mainThreadScrollingReasons,
+                           compositorElementId);
     return existingReasons != mainThreadScrollingReasons;
   }
   frameView.setScroll(ScrollPaintPropertyNode::create(
       std::move(parent), std::move(scrollOffset), clip, bounds,
       userScrollableHorizontal, userScrollableVertical,
-      mainThreadScrollingReasons));
+      mainThreadScrollingReasons, compositorElementId));
   return true;
 }
 
@@ -349,12 +363,6 @@ static FloatPoint3D transformOrigin(const LayoutBox& box) {
       style.transformOriginZ());
 }
 
-static CompositorElementId createDomNodeBasedCompositorElementId(
-    const LayoutObject& object) {
-  return createCompositorElementId(DOMNodeIds::idForNode(object.node()),
-                                   CompositorSubElementId::Primary);
-}
-
 void PaintPropertyTreeBuilder::updateTransform(
     const LayoutObject& object,
     PaintPropertyTreeBuilderContext& context) {
@@ -794,10 +802,18 @@ void PaintPropertyTreeBuilder::updateScrollAndScrollTranslation(
             context.forceSubtreeUpdate = true;
         }
 
+        // TODO(wkorman): Should we do this conditionally on something that

[wkorman, 2017/01/19 23:43:10]

And seeking feedback here.

[ajuma, 2017/01/20 14:41:57]

See comment above, but I think this is fine for now.

+        // connotes use of composited scrolling?
+        // ScrollableArea::usesCompositedScrolling is not currently SPv2
+        // compatible due to dependence on CompositedLayerMapping (see
+        // PaintLayerScrollableArea override).
+        CompositorElementId compositorElementId =
+            createDomNodeBasedCompositorElementId(object);
+
         context.forceSubtreeUpdate |= properties.updateScroll(
             context.current.scroll, properties.scrollTranslation(), scrollClip,
             scrollBounds, userScrollableHorizontal, userScrollableVertical,
-            reasons);
+            reasons, compositorElementId);
       }
     }