Clear previous visual rects when composited PaintLayer goes away.

This avoids spurious layout shift reports when a static-positioned
element loses its PaintLayer and cc::Layer at the same time (a common
scenario at the end of a transition animation).

Bug: 960614
Change-Id: I0ab8b693dbb764880090d1b078cb138f4ca1ddb7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1636403
Commit-Queue: Steve Kobes <skobes@chromium.org>
Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#665588}

third_party/blink/renderer/core/layout/jank_tracker_test.cc

@@ -331,4 +331,55 @@ TEST_F(JankTrackerSimTest, SubframeWeighting) {
   EXPECT_FLOAT_EQ(0.15, jank_tracker.WeightedScore());
 }
 
+TEST_F(JankTrackerTest, StableCompositingChanges) {
+  SetBodyInnerHTML(R"HTML(
+    <style>
+      body { margin: 0; }
+      #outer {
+        margin-left: 50px;
+        margin-top: 50px;
+        width: 200px;
+        height: 200px;
+        background: #dde;
+      }
+      .tr {
+        will-change: transform;
+      }
+      .pl {
+        position: relative;
+        z-index: 0;
+        left: 0;
+        top: 0;
+      }
+      #inner {
+        display: inline-block;
+        width: 100px;
+        height: 100px;
+        background: #666;
+        margin-left: 50px;
+        margin-top: 50px;
+      }
+    </style>
+    <div id=outer><div id=inner></div></div>
+  )HTML");
+
+  Element* element = GetDocument().getElementById("outer");
+  size_t state = 0;
+  auto advance = [this, element, &state]() -> bool {
+    //
+    // Test each of the following transitions:
+    // - add/remove a PaintLayer
+    // - add/remove a cc::Layer when there is already a PaintLayer
+    // - add/remove a cc::Layer and a PaintLayer together
+
+    static const char* states[] = {"", "pl", "pl tr", "pl", "", "tr", ""};
+    element->setAttribute(html_names::kClassAttr, AtomicString(states[state]));
+    UpdateAllLifecyclePhases();
+    return ++state < sizeof states / sizeof *states;
+  };
+  while (advance()) {
+  }
+  EXPECT_FLOAT_EQ(0, GetJankTracker().Score());
+}
+
 }  // namespace blink

third_party/blink/renderer/core/paint/compositing/paint_layer_compositor.cc

@@ -381,8 +381,9 @@ void PaintLayerCompositor::UpdateWithoutAcceleratedCompositing(
 #endif
 }
 
-static void ForceRecomputeVisualRectsIncludingNonCompositingDescendants(
-    LayoutObject& layout_object) {
+void PaintLayerCompositor::
+    ForceRecomputeVisualRectsIncludingNonCompositingDescendants(
+        LayoutObject& layout_object) {
   // We clear the previous visual rect as it's wrong (paint invalidation
   // container changed, ...). Forcing a full invalidation will make us recompute
   // it. Also we are not changing the previous position from our paint

third_party/blink/renderer/core/paint/compositing/paint_layer_compositor.h

@@ -167,6 +167,9 @@ class CORE_EXPORT PaintLayerCompositor {
     compositing_inputs_root_.Update(layer);
   }
 
+  void ForceRecomputeVisualRectsIncludingNonCompositingDescendants(
+      LayoutObject&);
+
  private:
 #if DCHECK_IS_ON()
   void AssertNoUnresolvedDirtyBits();

third_party/blink/renderer/core/paint/paint_layer.cc

@@ -214,8 +214,10 @@ PaintLayer::~PaintLayer() {
 
   // Child layers will be deleted by their corresponding layout objects, so
   // we don't need to delete them ourselves.
-
-  ClearCompositedLayerMapping(true);
+  {
+    DisableCompositingQueryAsserts disabler;
+    ClearCompositedLayerMapping(true);
+  }
 
   if (scrollable_area_)
     scrollable_area_->Dispose();
@@ -2793,7 +2795,7 @@ GraphicsLayer* PaintLayer::GraphicsLayerBacking(const LayoutObject* obj) const {
 }
 
 void PaintLayer::EnsureCompositedLayerMapping() {
-  if (rare_data_ && rare_data_->composited_layer_mapping)
+  if (HasCompositedLayerMapping())
     return;
 
   EnsureRareData().composited_layer_mapping =
@@ -2803,7 +2805,25 @@ void PaintLayer::EnsureCompositedLayerMapping() {
 }
 
 void PaintLayer::ClearCompositedLayerMapping(bool layer_being_destroyed) {
-  if (!layer_being_destroyed) {
+  if (!HasCompositedLayerMapping())
+    return;
+
+  if (layer_being_destroyed) {
+    if (!RuntimeEnabledFeatures::CompositeAfterPaintEnabled()) {
+      // The visual rects will be in a different coordinate space after losing
+      // their compositing container. Clear them before prepaint to avoid
+      // spurious layout shift reports from JankTracker.
+      // If the PaintLayer were not being destroyed, this would happen during
+      // the compositing update (PaintLayerCompositor::UpdateIfNeeded).
+      // TODO: JankTracker's reliance on having visual rects cleared before
+      // prepaint in the case of compositing changes is not ideal, and will not
+      // work with CompositeAfterPaint. Some transform tree changes may still
+      // produce incorrect behavior from JankTracker (see discussion on review
+      // thread of http://crrev.com/c/1636403).
+      Compositor()->ForceRecomputeVisualRectsIncludingNonCompositingDescendants(
+          layout_object_);
+    }
+  } else {
     // We need to make sure our decendants get a geometry update. In principle,
     // we could call setNeedsGraphicsLayerUpdate on our children, but that would
     // require walking the z-order lists to find them. Instead, we
@@ -2813,9 +2833,8 @@ void PaintLayer::ClearCompositedLayerMapping(bool layer_being_destroyed) {
       compositing_parent->GetCompositedLayerMapping()
           ->SetNeedsGraphicsLayerUpdate(kGraphicsLayerUpdateSubtree);
   }
-
-  if (rare_data_)
-    rare_data_->composited_layer_mapping.reset();
+  DCHECK(rare_data_);
+  rare_data_->composited_layer_mapping.reset();
 }
 
 void PaintLayer::SetGroupedMapping(CompositedLayerMapping* grouped_mapping,