Don't detect paint timing for opacity:0

Previously PaintTimingDetector depended the painter not to paint
opacity:0 contents to avoid detect timing of them, which didn't work
for composited layers and CompositeAfterPaint where we always paint
opacity:0.

Bug: 957674
Change-Id: Ie5530f5a4c00d5e350b6dc2d1922f58e3953e7f0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2106341
Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Reviewed-by: Nicolás Peña Moreno <npm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#751105}

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

@@ -10,6 +10,7 @@
 #include "third_party/blink/renderer/core/layout/svg/layout_svg_image.h"
 #include "third_party/blink/renderer/core/loader/resource/image_resource_content.h"
 #include "third_party/blink/renderer/platform/graphics/unaccelerated_static_bitmap_image.h"
+#include "third_party/blink/renderer/platform/testing/paint_test_configurations.h"
 #include "third_party/blink/renderer/platform/testing/url_test_helpers.h"
 #include "third_party/skia/include/core/SkImage.h"
 #include "third_party/skia/include/core/SkSurface.h"
@@ -22,7 +23,8 @@ extern bool IsExplicitlyRegisteredForTiming(const LayoutObject* layout_object);
 
 }
 
-class ImageElementTimingTest : public testing::Test {
+class ImageElementTimingTest : public testing::Test,
+                               public PaintTestConfigurations {
  protected:
   void SetUp() override {
     web_view_helper_.Initialize();
@@ -107,13 +109,18 @@ class ImageElementTimingTest : public testing::Test {
   }
 };
 
-TEST_F(ImageElementTimingTest, TestIsExplicitlyRegisteredForTiming) {
+INSTANTIATE_PAINT_TEST_SUITE_P(ImageElementTimingTest);
+
+TEST_P(ImageElementTimingTest, TestIsExplicitlyRegisteredForTiming) {
   frame_test_helpers::LoadHTMLString(
       web_view_helper_.GetWebView()->MainFrameImpl(), R"HTML(
     <img id="missing-attribute" style='width: 100px; height: 100px;'/>
-    <img id="unset-attribute" elementtiming style='width: 100px; height: 100px;'/>
-    <img id="empty-attribute" elementtiming="" style='width: 100px; height: 100px;'/>
-    <img id="valid-attribute" elementtiming="valid-id" style='width: 100px; height: 100px;'/>
+    <img id="unset-attribute" elementtiming
+         style='width: 100px; height: 100px;'/>
+    <img id="empty-attribute" elementtiming=""
+         style='width: 100px; height: 100px;'/>
+    <img id="valid-attribute" elementtiming="valid-id"
+         style='width: 100px; height: 100px;'/>
   )HTML",
       base_url_);
 
@@ -141,7 +148,7 @@ TEST_F(ImageElementTimingTest, TestIsExplicitlyRegisteredForTiming) {
                          "should be explicitly registered.";
 }
 
-TEST_F(ImageElementTimingTest, IgnoresUnmarkedElement) {
+TEST_P(ImageElementTimingTest, IgnoresUnmarkedElement) {
   // Tests that, if the 'elementtiming' attribute is missing, the element isn't
   // considered by ImageElementTiming.
   frame_test_helpers::LoadHTMLString(
@@ -156,12 +163,13 @@ TEST_F(ImageElementTimingTest, IgnoresUnmarkedElement) {
       std::make_pair(layout_image, layout_image->CachedImage())));
 }
 
-TEST_F(ImageElementTimingTest, ImageInsideSVG) {
+TEST_P(ImageElementTimingTest, ImageInsideSVG) {
   frame_test_helpers::LoadHTMLString(
       web_view_helper_.GetWebView()->MainFrameImpl(), R"HTML(
     <svg>
       <foreignObject width="100" height="100">
-        <img elementtiming="image-inside-svg" id="target" style='width: 100px; height: 100px;'/>
+        <img elementtiming="image-inside-svg" id="target"
+             style='width: 100px; height: 100px;'/>
       </foreignObject>
     </svg>
   )HTML",
@@ -175,13 +183,14 @@ TEST_F(ImageElementTimingTest, ImageInsideSVG) {
       std::make_pair(layout_image, layout_image->CachedImage())));
 }
 
-TEST_F(ImageElementTimingTest, ImageInsideNonRenderedSVG) {
+TEST_P(ImageElementTimingTest, ImageInsideNonRenderedSVG) {
   frame_test_helpers::LoadHTMLString(
       web_view_helper_.GetWebView()->MainFrameImpl(), R"HTML(
     <svg mask="url(#mask)">
       <mask id="mask">
         <foreignObject width="100" height="100">
-          <img elementtiming="image-inside-svg" id="target" style='width: 100px; height: 100px;'/>
+          <img elementtiming="image-inside-svg" id="target"
+               style='width: 100px; height: 100px;'/>
         </foreignObject>
       </mask>
       <rect width="100" height="100" fill="green"/>
@@ -195,10 +204,11 @@ TEST_F(ImageElementTimingTest, ImageInsideNonRenderedSVG) {
   EXPECT_FALSE(GetLayoutObjectById("target"));
 }
 
-TEST_F(ImageElementTimingTest, ImageRemoved) {
+TEST_P(ImageElementTimingTest, ImageRemoved) {
   frame_test_helpers::LoadHTMLString(
       web_view_helper_.GetWebView()->MainFrameImpl(), R"HTML(
-    <img elementtiming="will-be-removed" id="target" style='width: 100px; height: 100px;'/>
+    <img elementtiming="will-be-removed" id="target"
+         style='width: 100px; height: 100px;'/>
   )HTML",
       base_url_);
   LayoutImage* layout_image = SetImageResource("target", 5, 5);
@@ -213,11 +223,12 @@ TEST_F(ImageElementTimingTest, ImageRemoved) {
   EXPECT_EQ(ImagesNotifiedSize(), 0u);
 }
 
-TEST_F(ImageElementTimingTest, SVGImageRemoved) {
+TEST_P(ImageElementTimingTest, SVGImageRemoved) {
   frame_test_helpers::LoadHTMLString(
       web_view_helper_.GetWebView()->MainFrameImpl(), R"HTML(
     <svg>
-      <image elementtiming="svg-will-be-removed" id="target" style='width: 100px; height: 100px;'/>
+      <image elementtiming="svg-will-be-removed" id="target"
+             style='width: 100px; height: 100px;'/>
     </svg>
   )HTML",
       base_url_);
@@ -233,7 +244,7 @@ TEST_F(ImageElementTimingTest, SVGImageRemoved) {
   EXPECT_EQ(ImagesNotifiedSize(), 0u);
 }
 
-TEST_F(ImageElementTimingTest, BackgroundImageRemoved) {
+TEST_P(ImageElementTimingTest, BackgroundImageRemoved) {
   frame_test_helpers::LoadHTMLString(
       web_view_helper_.GetWebView()->MainFrameImpl(), R"HTML(
     <style>

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

@@ -17,6 +17,7 @@
 #include "third_party/blink/renderer/core/paint/paint_layer.h"
 #include "third_party/blink/renderer/core/paint/paint_layer_paint_order_iterator.h"
 #include "third_party/blink/renderer/core/paint/paint_layer_scrollable_area.h"
+#include "third_party/blink/renderer/core/paint/paint_timing_detector.h"
 #include "third_party/blink/renderer/core/paint/scrollable_area_painter.h"
 #include "third_party/blink/renderer/platform/geometry/float_point_3d.h"
 #include "third_party/blink/renderer/platform/graphics/graphics_layer.h"
@@ -48,8 +49,6 @@ static ShouldRespectOverflowClipType ShouldRespectOverflowClip(
 }
 
 bool PaintLayerPainter::PaintedOutputInvisible(const ComputedStyle& style) {
-  DCHECK(!RuntimeEnabledFeatures::CompositeAfterPaintEnabled());
-
   if (style.HasBackdropFilter())
     return false;
 
@@ -86,10 +85,10 @@ PaintResult PaintLayerPainter::Paint(
       !paint_layer_.HasSelfPaintingLayerDescendant())
     return kFullyPainted;
 
-  // If this layer is totally invisible then there is nothing to paint. In CAP
-  // we simplify this optimization by painting even when effectively invisible
-  // but skipping the painted content during layerization in
-  // PaintArtifactCompositor.
+  // If this layer is totally invisible then there is nothing to paint.
+  // In CompositeAfterPaint we simplify this optimization by painting even when
+  // effectively invisible but skipping the painted content during layerization
+  // in PaintArtifactCompositor.
   if (paint_layer_.PaintsWithTransparency(
           painting_info.GetGlobalPaintFlags())) {
     if (!RuntimeEnabledFeatures::CompositeAfterPaintEnabled() &&
@@ -325,6 +324,10 @@ PaintResult PaintLayerPainter::PaintLayerContents(
     return kMayBeClippedByCullRect;
   }
 
+  base::Optional<IgnorePaintTimingScope> ignore_paint_timing;
+  if (PaintedOutputInvisible(paint_layer_.GetLayoutObject().StyleRef()))
+    ignore_paint_timing.emplace();
+
   PaintLayerFlags paint_flags = paint_flags_arg;
   PaintLayerPaintingInfo painting_info = painting_info_arg;
   AdjustForPaintProperties(context, painting_info, paint_flags);

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

@@ -63,6 +63,8 @@ bool IsBackgroundImageContentful(const LayoutObject& object,
 
 }  // namespace
 
+bool IgnorePaintTimingScope::should_ignore_ = false;
+
 PaintTimingDetector::PaintTimingDetector(LocalFrameView* frame_view)
     : frame_view_(frame_view),
       text_paint_timing_detector_(
@@ -106,6 +108,9 @@ void PaintTimingDetector::NotifyBackgroundImagePaint(
     const StyleFetchedImage* style_image,
     const PropertyTreeState& current_paint_chunk_properties,
     const IntRect& image_border) {
+  if (IgnorePaintTimingScope::ShouldIgnore())
+    return;
+
   DCHECK(image);
   DCHECK(style_image->CachedImage());
   if (!node)
@@ -132,6 +137,9 @@ void PaintTimingDetector::NotifyImagePaint(
     const IntSize& intrinsic_size,
     const ImageResourceContent* cached_image,
     const PropertyTreeState& current_paint_chunk_properties) {
+  if (IgnorePaintTimingScope::ShouldIgnore())
+    return;
+
   LocalFrameView* frame_view = object.GetFrameView();
   if (!frame_view)
     return;
@@ -349,6 +357,9 @@ ScopedPaintTimingDetectorBlockPaintHook*
 void ScopedPaintTimingDetectorBlockPaintHook::EmplaceIfNeeded(
     const LayoutBoxModelObject& aggregator,
     const PropertyTreeState& property_tree_state) {
+  if (IgnorePaintTimingScope::ShouldIgnore())
+    return;
+
   // |reset_top_| is unset when |aggregator| is anonymous so that each
   // aggregation corresponds to an element. See crbug.com/988593. When set,
   // |top_| becomes |this|, and |top_| is restored to the previous value when

third_party/blink/renderer/core/paint/paint_timing_detector.h

@@ -273,9 +273,27 @@ class ScopedPaintTimingDetectorBlockPaintHook {
   DISALLOW_COPY_AND_ASSIGN(ScopedPaintTimingDetectorBlockPaintHook);
 };
 
+// Creates a scope to ignore paint timing, e.g. when we are painting contents
+// under opacity:0.
+class IgnorePaintTimingScope {
+  STACK_ALLOCATED();
+
+ public:
+  IgnorePaintTimingScope() : auto_reset_(&should_ignore_, true) {}
+  ~IgnorePaintTimingScope() = default;
+
+  static bool ShouldIgnore() { return should_ignore_; }
+
+ private:
+  base::AutoReset<bool> auto_reset_;
+  static bool should_ignore_;
+};
+
 // static
 inline void PaintTimingDetector::NotifyTextPaint(
     const IntRect& text_visual_rect) {
+  if (IgnorePaintTimingScope::ShouldIgnore())
+    return;
   ScopedPaintTimingDetectorBlockPaintHook::AggregateTextPaint(text_visual_rect);
 }
 

third_party/blink/web_tests/FlagExpectations/composite-after-paint

@@ -45,11 +45,6 @@ virtual/android/url-bar/bottom-fixed-adjusted-when-showing-url-bar.html [ Crash
 # Wrong clipping of the right side of nested composited reflection with non-zero filter origin.
 compositing/reflections/deeply-nested-reflections.html [ Failure ]
 
-# We always paint opacity:0 which breaks LCP's expectation.
-crbug.com/957674 cexternal/wpt/largest-contentful-paint/invisible-images.html [ Failure ]
-crbug.com/957674 virtual/scalefactor200/external/wpt/largest-contentful-paint/invisible-images.html [ Skip ]
-crbug.com/957674 virtual/scalefactor200withoutzoom/external/wpt/largest-contentful-paint/invisible-images.html [ Skip ]
-
 # A fieldset paints its background twice.
 external/wpt/html/rendering/non-replaced-elements/the-fieldset-and-legend-elements/fieldset-overflow.html [ Crash ]
 

third_party/blink/web_tests/external/wpt/largest-contentful-paint/invisible-images-composited.html

+<!DOCTYPE HTML>
+<meta charset=utf-8>
+<head>
+<title>Largest Contentful Paint: invisible images are not observable</title>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<style>
+  .opacity0 {
+    opacity: 0;
+  }
+  .visibilityHidden {
+    visibility: hidden;
+  }
+  .displayNone {
+    display: none;
+  }
+  .composited {
+    will-change: transform;
+  }
+</style>
+</head>
+<body>
+<script>
+  async_test(t => {
+    assert_precondition(window.LargestContentfulPaint, "LargestContentfulPaint is not implemented");
+    const observer = new PerformanceObserver(
+      t.step_func(entryList => {
+        entryList.getEntries().forEach(entry => {
+          // May receive a text entry. Ignore that entry.
+          if (!entry.url) {
+            return;
+          }
+          // The images should not have caused an entry, so fail test.
+          assert_unreached('Should not have received an entry! Received one with id '
+              + entryList.getEntries()[0].id);
+        });
+      })
+    );
+    observer.observe({type: 'largest-contentful-paint', buffered: true});
+    // Images have been added but should not cause entries to be dispatched.
+    // Wait for 500ms and end test, ensuring no entry was created.
+    t.step_timeout(() => {
+      t.done();
+    }, 500);
+  }, 'Images with opacity: 0, visibility: hidden, or display: none are not observable by LargestContentfulPaint.');
+</script>
+<img src='/images/blue.png' class='opacity0 composited' id='opacity0'/>
+<img src='/images/green.png' class='visibilityHidden composited' id='visibilityHidden'/>
+<img src='/images/red.png' class='displayNone composited' id='displayNone'/>
+<div class='opacity0 composited'><img class='composited' src='/images/yellow.png' id='divOpacity0'/></div>
+<div class='visibilityHidden composited'><img class='composited' src='/images/yellow.png' id='divVisibilityHidden'/></div>
+<div class='displayNone composited'><img src='/images/yellow.png' id='divDisplayNone'/></div>
+<div class='opacity0 composited'><img src='/images/yellow.png' id='divOpacity0Composited'/></div>
+<div class='visibilityHidden composited'><img src='/images/yellow.png' id='divVisibilityHiddenComposited'/></div>
+<div class='displayNone composited'><img src='/images/yellow.png' id='divDisplayNoneComposited'/></div>
+</body>