[SPv1] Always set a CompositorElementId on main graphics layers; use PaintLayer id.

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

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "core/paint/PaintPropertyTreeBuilder.h"
 
 #include <memory>
 #include "core/dom/DOMNodeIds.h"
 #include "core/frame/FrameView.h"
 #include "core/frame/LocalFrame.h"
@@ skipping 54 matching lines @@
   if (auto* existing_content_clip = frame_view.ContentClip()) {
     existing_content_clip->Update(std::move(parent),
                                   std::move(local_transform_space), clip_rect);
     return false;
   }
   frame_view.SetContentClip(ClipPaintPropertyNode::Create(
       std::move(parent), std::move(local_transform_space), clip_rect));
   return true;
 }
 
 static CompositorElementId CreateDomNodeBasedCompositorElementId(

[wkorman, 2017/05/18 00:45:58]

Suggest rename to CreatePaintLayerBasedCompositorElementId() or something, to
match logic change.

[chrishtr, 2017/05/18 01:25:47]

Done.

     const LayoutObject& object) {
+  DCHECK(object.IsBoxModelObject() && object.HasLayer());
   // TODO(wkorman): Centralize this implementation with similar across

[wkorman, 2017/05/18 00:45:58]

Maybe we can remove this now. It's not looking to me like we'd want to share
more than we do currently, but open to thoughts.

I mean, maybe there are a few other places where we use
ToLayoutBoxModelObject(object).Layer()->UniqueId(), and we could move this
method somewhere in CompositorElementId.h and have all of those use it, but not
sure it's worth it.

When I wrote the TODO I think we were using DOMNodeIds everywhere when creating
a CompositorElementId.

[chrishtr, 2017/05/18 01:25:47]

Done.

   // animation, scrolling and compositing logic.
-  return CompositorElementIdFromDOMNodeId(
-      DOMNodeIds::IdForNode(object.GetNode()),
+  return CompositorElementIdFromPaintLayerId(
+      ToLayoutBoxModelObject(object).Layer()->UniqueId(),
       CompositorElementIdNamespace::kPrimary);
 }
 
 // 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 UpdateScrollTranslation(
     FrameView& frame_view,
     PassRefPtr<const TransformPaintPropertyNode> parent,
     const TransformationMatrix& matrix,
     const FloatPoint3D& origin,
@@ skipping 1097 matching lines @@
       NeedsFilter(object) || NeedsCssClip(object) ||
       NeedsScrollbarPaintOffset(object) || NeedsOverflowClip(object) ||
       NeedsPerspective(object) || NeedsSVGLocalToBorderBoxTransform(object) ||
       NeedsScrollTranslation(object) || NeedsCssClipFixedPosition(object);
 
   bool had_paint_properties = object.PaintProperties();
 
   if (needs_paint_properties && !had_paint_properties) {
     ObjectPaintProperties& paint_properties =
         object.GetMutableForPainting().EnsurePaintProperties();
-    if (RuntimeEnabledFeatures::slimmingPaintV2Enabled()) {
+    if (RuntimeEnabledFeatures::slimmingPaintV2Enabled() && object.HasLayer()) {

[wkorman, 2017/05/18 00:45:58]

This means we will stop setting compositor element ids on transform/effect nodes
in SPv2 when the involved layout object has no PaintLayer, right?

I think it also means that the majority of element ids we see in SPv2 will now
be paint layer ids rather than dom node ids. I only mention this because it
means we'll stop pounding the WeakIdentifierMap for DOM node ids.

Both seem fine, just noting while reading. Suggest noting both in CL
description. It currently only mentions SPv1 logic changes. Changes here still
have no ramification for SPv1 despite spinvalidation, right?

Do currently-passing animation layout tests in SPv2 still pass with the change?

[chrishtr, 2017/05/18 01:25:47]

No, because transforms and effects all induce PaintLayers. See:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La...
Yes. This is the reason I made the WeakIdentifierMap at the same time as
setting it unconditionally in SPv1 mode - to avoid a perf regression.
Haven't checked. Let's see what CQ dry run says.

       paint_properties.SetCompositorElementId(
           CreateDomNodeBasedCompositorElementId(object));
     }
   } else if (!needs_paint_properties && had_paint_properties) {
     object.GetMutableForPainting().ClearPaintProperties();
     full_context.force_subtree_update = true;
   }
 }
 
 static inline bool ObjectTypeMightNeedPaintProperties(
@@ skipping 71 matching lines @@
                            context.force_subtree_update);
 
     context.force_subtree_update |= object.SubtreeNeedsPaintPropertyUpdate();
   }
 
   if (object.CanContainAbsolutePositionObjects())
     context.container_for_absolute_position = &object;
 }
 
 }  // namespace blink