Unify graphics layer traversal functions

This makes them consistent to use the same skipping conditions as
what we do during PrePaint, to avoid access of null or stale layer
states.

Bug: 1110232
Change-Id: I50e3d997476e8fc8532c2b3cb931c5639c71044d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2342468Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#796152}

third_party/blink/renderer/core/frame/frame_overlay_test.cc

@@ -109,7 +109,7 @@ TEST_P(FrameOverlayTest, AcceleratedCompositing) {
     builder.EndRecording()->Playback(&canvas);
   } else {
     auto* graphics_layer = frame_overlay->GetGraphicsLayer();
-    EXPECT_FALSE(graphics_layer->GetHitTestable());
+    EXPECT_FALSE(graphics_layer->IsHitTestable());
     EXPECT_EQ(PropertyTreeState::Root(),
               graphics_layer->GetPropertyTreeState());
     graphics_layer->Paint();
@@ -162,7 +162,7 @@ TEST_P(FrameOverlayTest, DeviceEmulationScale) {
     check_paint_results(*paint_controller);
   } else {
     auto* graphics_layer = frame_overlay->GetGraphicsLayer();
-    EXPECT_FALSE(graphics_layer->GetHitTestable());
+    EXPECT_FALSE(graphics_layer->IsHitTestable());
     EXPECT_EQ(state, graphics_layer->GetPropertyTreeState());
     graphics_layer->Paint();
     check_paint_results(graphics_layer->GetPaintController());

third_party/blink/renderer/core/frame/local_frame_view.cc

@@ -2657,14 +2657,6 @@ bool LocalFrameView::RunPrePaintLifecyclePhase(
   return target_state > DocumentLifecycle::kPrePaintClean;
 }
 
-template <typename Function>
-static void ForAllGraphicsLayers(GraphicsLayer& layer,
-                                 const Function& function) {
-  function(layer);
-  for (auto* child : layer.Children())
-    ForAllGraphicsLayers(*child, function);
-}
-
 void LocalFrameView::RunPaintLifecyclePhase() {
   TRACE_EVENT0("blink,benchmark", "LocalFrameView::RunPaintLifecyclePhase");
   // While printing or capturing a paint preview of a document, the paint walk
@@ -2722,14 +2714,12 @@ void LocalFrameView::RunPaintLifecyclePhase() {
     if (!RuntimeEnabledFeatures::CompositeAfterPaintEnabled()) {
       auto* root = GetLayoutView()->Compositor()->PaintRootGraphicsLayer();
       if (root) {
-        ForAllGraphicsLayers(*root, [](GraphicsLayer& layer) {
-          if (layer.PaintsContentOrHitTest() && layer.HasLayerState()) {
-            // Notify the paint controller that the artifact has been pushed and
-            // some lifecycle state can be freed (such as raster invalidations).
-            layer.GetPaintController().FinishCycle();
-            layer.GetPaintController().ClearPropertyTreeChangedStateTo(
-                layer.GetPropertyTreeState());
-          }
+        ForAllPaintingGraphicsLayers(*root, [](GraphicsLayer& layer) {
+          // Notify the paint controller that the artifact has been pushed and
+          // some lifecycle state can be freed (such as raster invalidations).
+          layer.GetPaintController().FinishCycle();
+          layer.GetPaintController().ClearPropertyTreeChangedStateTo(
+              layer.GetPropertyTreeState());
         });
       }
     }
@@ -2794,69 +2784,35 @@ void LocalFrameView::EnqueueScrollEvents() {
   });
 }
 
-template <typename MainLayerFunction, typename ContentsLayerFunction>
-static void ForAllDrawableGraphicsLayers(
-    const GraphicsLayer* layer,
-    const MainLayerFunction& main_layer_function,
-    const ContentsLayerFunction& contents_layer_function) {
-  if (!layer || layer->Client().ShouldThrottleRendering() ||
-      layer->Client().IsUnderSVGHiddenContainer()) {
-    return;
-  }
-
-  if (layer->Client().PaintBlockedByDisplayLockIncludingAncestors(
-          DisplayLockContextLifecycleTarget::kSelf)) {
-    // If we skip the layer, then we need to ensure to notify the
-    // display-lock, since we need to force recollect the layers when we commit.
-    layer->Client().NotifyDisplayLockNeedsGraphicsLayerCollection();
-    return;
-  }
-
-  // We need to collect all layers that draw content, as well as some layers
-  // that don't for the purposes of hit testing. For example, an empty div
-  // will not draw content but needs to create a layer to ensure scroll events
-  // do not pass through it.
-  if (layer->PaintsContentOrHitTest() || layer->GetHitTestable())
-    main_layer_function(layer);
-
-  if (auto* contents_layer = layer->ContentsLayer())
-    contents_layer_function(layer, contents_layer);
-
-  for (const auto* child : layer->Children()) {
-    ForAllDrawableGraphicsLayers(child, main_layer_function,
-                                 contents_layer_function);
-  }
-}
-
-static void CollectDrawableLayersForLayerListRecursively(
+static void CollectGraphicsLayersForLayerListRecursively(
     GraphicsContext& context,
-    const GraphicsLayer* root) {
-  ForAllDrawableGraphicsLayers(
+    const GraphicsLayer& root) {
+  ForAllActiveGraphicsLayers(
       root,
-      [&](const GraphicsLayer* layer) { RecordGraphicsLayer(context, *layer); },
-      [&](const GraphicsLayer* layer, cc::Layer* contents_layer) {
+      [&](const GraphicsLayer& layer) { RecordGraphicsLayer(context, layer); },
+      [&](const GraphicsLayer& layer, cc::Layer& contents_layer) {
         RecordForeignLayer(
-            context, *layer, DisplayItem::kForeignLayerContentsWrapper,
-            contents_layer, layer->GetContentsOffsetFromTransformNode(),
-            &layer->GetContentsPropertyTreeState());
+            context, layer, DisplayItem::kForeignLayerContentsWrapper,
+            &contents_layer, layer.GetContentsOffsetFromTransformNode(),
+            &layer.GetContentsPropertyTreeState());
       });
 }
 
-static void UpdateLayerDebugInfoRecursively(const GraphicsLayer* root) {
-  ForAllDrawableGraphicsLayers(
+static void UpdateLayerDebugInfoRecursively(const GraphicsLayer& root) {
+  ForAllActiveGraphicsLayers(
       root,
-      [](const GraphicsLayer* layer) {
+      [](const GraphicsLayer& layer) {
         PaintArtifactCompositor::UpdateLayerDebugInfo(
-            layer->CcLayer(),
-            PaintChunk::Id(*layer, DisplayItem::kGraphicsLayerWrapper),
-            layer->GetCompositingReasons(),
-            layer->GetRasterInvalidationTracking());
+            *layer.CcLayer(),
+            PaintChunk::Id(layer, DisplayItem::kGraphicsLayerWrapper),
+            layer.GetCompositingReasons(),
+            layer.GetRasterInvalidationTracking());
       },
-      [](const GraphicsLayer* layer, cc::Layer* contents_layer) {
+      [](const GraphicsLayer& layer, cc::Layer& contents_layer) {
         PaintArtifactCompositor::UpdateLayerDebugInfo(
             contents_layer,
-            PaintChunk::Id(*layer, DisplayItem::kForeignLayerContentsWrapper),
-            layer->GetCompositingReasons(), nullptr);
+            PaintChunk::Id(layer, DisplayItem::kForeignLayerContentsWrapper),
+            layer.GetCompositingReasons(), nullptr);
       });
 }
 
@@ -3005,8 +2961,8 @@ void LocalFrameView::PushPaintArtifactToCompositor() {
 
   if (!RuntimeEnabledFeatures::CompositeAfterPaintEnabled() &&
       layer_debug_info_enabled_) {
-    UpdateLayerDebugInfoRecursively(
-        GetLayoutView()->Compositor()->PaintRootGraphicsLayer());
+    if (auto* root = GetLayoutView()->Compositor()->PaintRootGraphicsLayer())
+      UpdateLayerDebugInfoRecursively(*root);
   }
 
   // Skip updating property trees, pushing cc::Layers, and issuing raster
@@ -3055,7 +3011,7 @@ void LocalFrameView::PushPaintArtifactToCompositor() {
 
     GraphicsContext context(*paint_controller_);
     auto* root = GetLayoutView()->Compositor()->PaintRootGraphicsLayer();
-    CollectDrawableLayersForLayerListRecursively(context, root);
+    CollectGraphicsLayersForLayerListRecursively(context, *root);
 
     if (frame_->IsMainFrame()) {
       if (root == GetLayoutView()->Compositor()->RootGraphicsLayer())

third_party/blink/renderer/core/page/scrolling/scrolling_coordinator.cc

@@ -158,22 +158,6 @@ void ScrollingCoordinator::UpdateAfterPaint(LocalFrameView* frame_view) {
   }
 }
 
-template <typename Function>
-static void ForAllPaintingGraphicsLayers(GraphicsLayer& layer,
-                                         const Function& function) {
-  // Don't recurse into display-locked elements.
-  if (layer.Client().PaintBlockedByDisplayLockIncludingAncestors(
-          DisplayLockContextLifecycleTarget::kSelf)) {
-    return;
-  }
-
-  if (layer.PaintsContentOrHitTest() && layer.HasLayerState())
-    function(layer);
-
-  for (auto* child : layer.Children())
-    ForAllPaintingGraphicsLayers(*child, function);
-}
-
 // Set the non-fast scrollable regions on |layer|'s cc layer.
 static void UpdateLayerNonFastScrollableRegions(GraphicsLayer& layer) {
   // CompositeAfterPaint does this update in PaintArtifactCompositor.
@@ -181,11 +165,6 @@ static void UpdateLayerNonFastScrollableRegions(GraphicsLayer& layer) {
 
   DCHECK(layer.PaintsContentOrHitTest());
 
-  if (layer.Client().ShouldThrottleRendering()) {
-    layer.CcLayer()->SetNonFastScrollableRegion(cc::Region());
-    return;
-  }
-
   auto offset = layer.GetOffsetFromTransformNode();
   gfx::Vector2dF layer_offset = gfx::Vector2dF(offset.X(), offset.Y());
   PaintChunkSubset paint_chunks =
@@ -212,10 +191,10 @@ void ScrollingCoordinator::UpdateNonFastScrollableRegions(LocalFrame* frame) {
 // Set the touch action rects on the cc layer from the touch action data stored
 // on the GraphicsLayer's paint chunks.
 static void UpdateLayerTouchActionRects(GraphicsLayer& layer) {
-  if (layer.Client().ShouldThrottleRendering()) {
-    layer.CcLayer()->SetTouchActionRegion(cc::TouchActionRegion());
-    return;
-  }
+  // CompositeAfterPaint does this update in PaintArtifactCompositor.
+  DCHECK(!RuntimeEnabledFeatures::CompositeAfterPaintEnabled());
+
+  DCHECK(layer.PaintsContentOrHitTest());
 
   auto offset = layer.GetOffsetFromTransformNode();
   gfx::Vector2dF layer_offset = gfx::Vector2dF(offset.X(), offset.Y());

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

@@ -1059,7 +1059,7 @@ TEST_F(CompositedLayerMappingTest,
       mapping->ForegroundLayer()->PaintingPhase());
   // Regression test for crbug.com/767908: a foreground layer should also
   // participates hit testing.
-  EXPECT_TRUE(mapping->ForegroundLayer()->GetHitTestable());
+  EXPECT_TRUE(mapping->ForegroundLayer()->IsHitTestable());
 
   Element* negative_composited_child =
       GetDocument().getElementById("negative-composited-child");

third_party/blink/renderer/platform/graphics/compositing/paint_artifact_compositor.cc

@@ -1376,7 +1376,7 @@ void PaintArtifactCompositor::Update(
                        .GetTracking();
       }
       UpdateLayerDebugInfo(
-          layer.get(), pending_layer.FirstPaintChunk().id,
+          *layer, pending_layer.FirstPaintChunk().id,
           GetCompositingReasons(pending_layer, previous_pending_layer),
           tracking);
     }
@@ -1593,11 +1593,11 @@ void PaintArtifactCompositor::SetLayerDebugInfoEnabled(bool enabled) {
 }
 
 void PaintArtifactCompositor::UpdateLayerDebugInfo(
-    cc::Layer* layer,
+    cc::Layer& layer,
     const PaintChunk::Id& id,
     CompositingReasons compositing_reasons,
     RasterInvalidationTracking* raster_invalidation_tracking) {
-  cc::LayerDebugInfo& debug_info = layer->EnsureDebugInfo();
+  cc::LayerDebugInfo& debug_info = layer.EnsureDebugInfo();
 
   debug_info.name = id.client.DebugName().Utf8();
   if (id.type == DisplayItem::kForeignLayerContentsWrapper) {

third_party/blink/renderer/platform/graphics/compositing/paint_artifact_compositor.h

@@ -210,7 +210,7 @@ class PLATFORM_EXPORT PaintArtifactCompositor final
 
   // TODO(wangxianzhu): Make this private and refactor when removing
   // pre-CompositeAfterPaint.
-  static void UpdateLayerDebugInfo(cc::Layer* layer,
+  static void UpdateLayerDebugInfo(cc::Layer& layer,
                                    const PaintChunk::Id&,
                                    CompositingReasons,
                                    RasterInvalidationTracking*);

third_party/blink/renderer/platform/graphics/graphics_layer.cc

@@ -261,29 +261,13 @@ IntRect GraphicsLayer::InterestRect() {
 }
 
 bool GraphicsLayer::PaintRecursively() {
-  // TODO(crbug.com/1033240): Debugging information for the referenced bug.
-  // Remove when it is fixed.
-  CHECK(&client_);
-  if (client_.PaintBlockedByDisplayLockIncludingAncestors(
-          DisplayLockContextLifecycleTarget::kSelf)) {
-    return false;
-  }
-
   bool painted = false;
-  if (PaintsContentOrHitTest()) {
-    if (Paint())
-      painted = true;
-  }
-
-  for (auto* child : Children())
-    painted |= child->PaintRecursively();
+  ForAllPaintingGraphicsLayers(
+      *this, [&painted](GraphicsLayer& layer) { painted |= layer.Paint(); });
   return painted;
 }
 
 bool GraphicsLayer::Paint() {
-  if (client_.ShouldThrottleRendering())
-    return false;
-
 #if !DCHECK_IS_ON()
   // TODO(crbug.com/853096): Investigate why we can ever reach here without
   // a valid layer state. Seems to only happen on Android builds.

third_party/blink/renderer/platform/graphics/graphics_layer.h

@@ -149,7 +149,7 @@ class PLATFORM_EXPORT GraphicsLayer : public DisplayItemClient,
   void SetContentsOpaqueForText(bool);
 
   void SetHitTestable(bool);
-  bool GetHitTestable() const { return hit_testable_; }
+  bool IsHitTestable() const { return hit_testable_; }
 
   // Some GraphicsLayers paint only the foreground or the background content
   GraphicsLayerPaintingPhase PaintingPhase() const { return painting_phase_; }
@@ -326,6 +326,59 @@ class PLATFORM_EXPORT GraphicsLayer : public DisplayItemClient,
   DISALLOW_COPY_AND_ASSIGN(GraphicsLayer);
 };
 
+// Iterates all graphics layers that should be seen by the compositor in
+// pre-order. |GraphicsLayerType| matches |GraphicsLayer&| or
+// |const GraphicsLayer&|.
+template <typename GraphicsLayerType,
+          typename GraphicsLayerFunction,
+          typename ContentsLayerFunction>
+void ForAllActiveGraphicsLayers(
+    GraphicsLayerType& layer,
+    const GraphicsLayerFunction& graphics_layer_function,
+    const ContentsLayerFunction& contents_layer_function) {
+  if (layer.Client().ShouldThrottleRendering() ||
+      layer.Client().IsUnderSVGHiddenContainer()) {
+    return;
+  }
+
+  if (layer.Client().PaintBlockedByDisplayLockIncludingAncestors(
+          DisplayLockContextLifecycleTarget::kSelf)) {
+    // If we skip the layer, then we need to ensure to notify the
+    // display-lock, since we need to force recollect the layers when we commit.
+    layer.Client().NotifyDisplayLockNeedsGraphicsLayerCollection();
+    return;
+  }
+
+  DCHECK(layer.HasLayerState());
+
+  if (layer.PaintsContentOrHitTest() || layer.IsHitTestable())
+    graphics_layer_function(layer);
+
+  if (auto* contents_layer = layer.ContentsLayer())
+    contents_layer_function(layer, *contents_layer);
+
+  for (auto* child : layer.Children()) {
+    ForAllActiveGraphicsLayers(*child, graphics_layer_function,
+                               contents_layer_function);
+  }
+}
+
+template <typename GraphicsLayerType, typename Function>
+void ForAllActiveGraphicsLayers(GraphicsLayerType& layer,
+                                const Function& function) {
+  ForAllActiveGraphicsLayers(layer, function,
+                             [](GraphicsLayerType&, const cc::Layer&) {});
+}
+
+template <typename GraphicsLayerType, typename Function>
+void ForAllPaintingGraphicsLayers(GraphicsLayerType& layer,
+                                  const Function& function) {
+  ForAllActiveGraphicsLayers(layer, [&function](GraphicsLayerType& layer) {
+    if (layer.PaintsContentOrHitTest())
+      function(layer);
+  });
+}
+
 }  // namespace blink
 
 #endif  // THIRD_PARTY_BLINK_RENDERER_PLATFORM_GRAPHICS_GRAPHICS_LAYER_H_