[FCP++] Image: move node checking from pre-paint to paint

Currently FCP++ is hooking to pre-paint tree walk for the image nodes,
Instead, it should hook to the paint stage. Text nodes have finished
this change in
https://chromium-review.googlesource.com/c/chromium/src/+/1470732. Now
we should do the same for the image nodes.

Specifically, the painters that the ImagePaintDetector hooks to include:
* BoxPainter
* SVGImagePainter
* ImagePainter

Bug: 924301
Change-Id: Id1c05bc38b7967908b78b26b74d1e8f2e8376aac
Reviewed-on: https://chromium-review.googlesource.com/c/1477348
Commit-Queue: Liquan (Max) Gu <maxlg@chromium.org>
Reviewed-by: Liquan (Max) Gu <maxlg@chromium.org>
Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#635756}

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

@@ -13,12 +13,14 @@
 #include "third_party/blink/renderer/core/paint/nine_piece_image_painter.h"
 #include "third_party/blink/renderer/core/paint/paint_info.h"
 #include "third_party/blink/renderer/core/paint/paint_layer.h"
+#include "third_party/blink/renderer/core/paint/paint_timing_detector.h"
 #include "third_party/blink/renderer/core/paint/rounded_inner_rect_clipper.h"
 #include "third_party/blink/renderer/core/style/border_edge.h"
 #include "third_party/blink/renderer/core/style/computed_style.h"
 #include "third_party/blink/renderer/core/style/shadow_list.h"
 #include "third_party/blink/renderer/platform/geometry/layout_rect.h"
 #include "third_party/blink/renderer/platform/graphics/graphics_context_state_saver.h"
+#include "third_party/blink/renderer/platform/graphics/paint/paint_controller.h"
 #include "third_party/blink/renderer/platform/graphics/scoped_interpolation_quality.h"
 
 namespace blink {
@@ -542,6 +544,14 @@ inline bool PaintFastBottomLayer(Node* node,
   context.DrawImageRRect(image, Image::kSyncDecode, image_border, src_rect,
                          composite_op);
 
+  if (RuntimeEnabledFeatures::FirstContentfulPaintPlusPlusEnabled()) {
+    if (info.image && info.image->IsImageResource()) {
+      PaintTimingDetector::NotifyBackgroundImagePaint(
+          node, image,
+          paint_info.context.GetPaintController()
+              .CurrentPaintChunkProperties());
+    }
+  }
   return true;
 }
 
@@ -658,6 +668,13 @@ void PaintFillLayerBackground(GraphicsContext& context,
                         FloatRect(geometry.SnappedDestRect()), geometry.Phase(),
                         FloatSize(geometry.TileSize()), composite_op,
                         FloatSize(geometry.SpaceSize()));
+    if (RuntimeEnabledFeatures::FirstContentfulPaintPlusPlusEnabled()) {
+      if (info.image && info.image->IsImageResource()) {
+        PaintTimingDetector::NotifyBackgroundImagePaint(
+            node, image,
+            context.GetPaintController().CurrentPaintChunkProperties());
+      }
+    }
   }
 }
 

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

@@ -17,6 +17,7 @@
 #include "third_party/blink/renderer/core/paint/paint_timing_detector.h"
 #include "third_party/blink/renderer/core/style/style_fetched_image.h"
 #include "third_party/blink/renderer/platform/geometry/layout_rect.h"
+#include "third_party/blink/renderer/platform/graphics/image.h"
 #include "third_party/blink/renderer/platform/graphics/paint/geometry_mapper.h"
 #include "third_party/blink/renderer/platform/instrumentation/tracing/trace_event.h"
 #include "third_party/blink/renderer/platform/instrumentation/tracing/traced_value.h"
@@ -42,7 +43,7 @@ String GetImageUrl(const LayoutObject& object) {
     const ImageResourceContent* cached_image = image_resource->CachedImage();
     return cached_image ? cached_image->Url().StrippedForUseAsReferrer() : "";
   }
-  DCHECK(ImagePaintTimingDetector::HasContentfulBackgroundImage(object));
+  DCHECK(ImagePaintTimingDetector::HasBackgroundImage(object));
   const ComputedStyle* style = object.Style();
   StringBuilder concatenated_result;
   for (const FillLayer* bg_layer = &style->BackgroundLayers(); bg_layer;
@@ -59,7 +60,7 @@ String GetImageUrl(const LayoutObject& object) {
 #endif
 
 bool AttachedBackgroundImagesAllLoaded(const LayoutObject& object) {
-  DCHECK(ImagePaintTimingDetector::HasContentfulBackgroundImage(object));
+  DCHECK(ImagePaintTimingDetector::HasBackgroundImage(object));
   const ComputedStyle* style = object.Style();
   DCHECK(style);
   for (const FillLayer* bg_layer = &style->BackgroundLayers(); bg_layer;
@@ -92,7 +93,7 @@ bool IsLoaded(const LayoutObject& object) {
     const ImageResourceContent* cached_image = image_resource->CachedImage();
     return cached_image ? cached_image->IsLoaded() : false;
   }
-  DCHECK(ImagePaintTimingDetector::HasContentfulBackgroundImage(object));
+  DCHECK(ImagePaintTimingDetector::HasBackgroundImage(object));
   return AttachedBackgroundImagesAllLoaded(object);
 }
 }  // namespace
@@ -190,7 +191,7 @@ void ImagePaintTimingDetector::Analyze() {
   }
 }
 
-void ImagePaintTimingDetector::OnPrePaintFinished() {
+void ImagePaintTimingDetector::OnPaintFinished() {
   frame_index_++;
   if (records_pending_timing_.size() <= 0)
     return;
@@ -276,37 +277,14 @@ void ImagePaintTimingDetector::ReportSwapTime(
   Analyze();
 }
 
-// In the context of FCP++, we define contentful background image as one that
-// satisfies all of the following conditions:
-// * has image reources attached to style of the object, i.e.,
-//  { background-image: url('example.gif') }
-// * not attached to <body> or <html>
-//
 // static
-bool ImagePaintTimingDetector::HasContentfulBackgroundImage(
-    const LayoutObject& object) {
+bool ImagePaintTimingDetector::HasBackgroundImage(const LayoutObject& object) {
   Node* node = object.GetNode();
   if (!node)
     return false;
-  // Background images attached to <body> or <html> are likely for background
-  // purpose, so we rule them out, according to the following rules:
-  // * Box model objects includes objects of box model, such as <div>, <body>,
-  //  LayoutView, but not includes LayoutText.
-  // * BackgroundTransfersToView is true for the <body>, <html>, e.g., that
-  //  have transferred their background to LayoutView.
-  // * LayoutView has the background transfered by <html> if <html> has
-  //  background.
-  if (!object.IsBoxModelObject() ||
-      ToLayoutBoxModelObject(object).BackgroundTransfersToView())
-    return false;
-  if (object.IsLayoutView())
-    return false;
   const ComputedStyle* style = object.Style();
   if (!style)
     return false;
-  if (!style->HasBackgroundImage())
-    return false;
-
   for (const FillLayer* bg_layer = &style->BackgroundLayers(); bg_layer;
        bg_layer = bg_layer->Next()) {
     StyleImage* bg_image = bg_layer->GetImage();
@@ -318,8 +296,9 @@ bool ImagePaintTimingDetector::HasContentfulBackgroundImage(
   return false;
 }
 
-void ImagePaintTimingDetector::RecordImage(const LayoutObject& object,
-                                           const PaintLayer& painting_layer) {
+void ImagePaintTimingDetector::RecordImage(
+    const LayoutObject& object,
+    const PropertyTreeState& current_paint_chunk_properties) {
   // TODO(crbug.com/933479): Use LayoutObject::GeneratingNode() to include
   // anonymous objects' rect.
   Node* node = object.GetNode();
@@ -342,7 +321,9 @@ void ImagePaintTimingDetector::RecordImage(const LayoutObject& object,
       return;
     uint64_t rect_size =
         frame_view_->GetPaintTimingDetector().CalculateVisualSize(
-            visual_rect, painting_layer);
+            visual_rect, current_paint_chunk_properties);
+    DVLOG(2) << "Node id (" << node_id << "): size=" << rect_size
+             << ", type=" << object.DebugName();
     if (rect_size == 0) {
       // When rect_size == 0, it either means the image is size 0 or the image
       // is out of viewport. Either way, we don't track this image anymore, to
@@ -388,6 +369,30 @@ void ImagePaintTimingDetector::RecordImage(const LayoutObject& object,
   }
 }
 
+// In the context of FCP++, we define contentful background image as one that
+// satisfies all of the following conditions:
+// * has image reources attached to style of the object, i.e.,
+//  { background-image: url('example.gif') }
+// * not attached to <body> or <html>
+// This function contains the above heuristics.
+//
+// static
+bool ImagePaintTimingDetector::IsBackgroundImageContentful(
+    const LayoutObject& object,
+    const Image& image) {
+  // Background images attached to <body> or <html> are likely for background
+  // purpose, so we rule them out.
+  if (object.IsLayoutView()) {
+    return false;
+  }
+  // Generated images are excluded here, as they are likely to serve for
+  // background purpose.
+  if (!image.IsBitmapImage() && !image.IsStaticBitmapImage() &&
+      !image.IsSVGImage() && !image.IsPlaceholderImage())
+    return false;
+  return true;
+}
+
 void ImagePaintTimingDetector::StopRecordEntries() {
   is_recording_ = false;
 }

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

@@ -16,10 +16,12 @@
 #include "third_party/blink/renderer/platform/wtf/time.h"
 
 namespace blink {
-class PaintLayer;
+
 class LayoutObject;
-class TracedValue;
 class LocalFrameView;
+class PropertyTreeState;
+class TracedValue;
+class Image;
 
 class ImageRecord : public base::SupportsWeakPtr<ImageRecord> {
  public:
@@ -44,7 +46,9 @@ class ImageRecord : public base::SupportsWeakPtr<ImageRecord> {
 // 1. Tracks all images' first invalidation, recording their visual size, if
 // this image is within viewport.
 // 2. When an image finishes loading, record its paint time.
-// 3. At the end of each prepaint tree walk, the algorithm starts an analysis.
+// 3. At the end of each frame, if new images are added and loaded, the
+// algorithm will start an analysis.
+//
 // In the analysis:
 // 3.1 Largest Image Paint finds the largest image by the first visual size. If
 // it has finished loading, reports a candidate result as its first paint time
@@ -69,9 +73,11 @@ class CORE_EXPORT ImagePaintTimingDetector final
 
  public:
   ImagePaintTimingDetector(LocalFrameView*);
-  void RecordImage(const LayoutObject&, const PaintLayer&);
-  static bool HasContentfulBackgroundImage(const LayoutObject& object);
-  void OnPrePaintFinished();
+  void RecordImage(const LayoutObject&,
+                   const PropertyTreeState& current_paint_chunk_properties);
+  static bool IsBackgroundImageContentful(const LayoutObject&, const Image&);
+  static bool HasBackgroundImage(const LayoutObject& object);
+  void OnPaintFinished();
   void NotifyNodeRemoved(DOMNodeId);
   base::TimeTicks LargestImagePaint() const {
     return !largest_image_paint_

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

@@ -23,7 +23,7 @@
 namespace blink {
 
 class ImagePaintTimingDetectorTest
-    : public PageTestBase,
+    : public RenderingTest,
       private ScopedFirstContentfulPaintPlusPlusForTest {
   using CallbackQueue = std::queue<WebLayerTreeView::ReportTimeCallback>;
 
@@ -39,7 +39,8 @@ class ImagePaintTimingDetectorTest
   }
 
   void SetUp() override {
-    PageTestBase::SetUp();
+    RenderingTest::SetUp();
+    RenderingTest::EnableCompositing();
     GetPaintTimingDetector()
         .GetImagePaintTimingDetector()
         .notify_swap_time_override_for_testing_ =
@@ -301,35 +302,6 @@ TEST_F(ImagePaintTimingDetectorTest,
             base::TimeTicks() + TimeDelta::FromSecondsD(1));
 }
 
-// This test dipicts a situation when a smaller image has loaded, but a larger
-// image is loading. When we call analyze, the result will be empty because
-// we don't know when the largest image will finish loading. We wait until
-// next analysis to make the judgement again.
-// This bahavior is the same with Last Image Paint as well.
-TEST_F(ImagePaintTimingDetectorTest, DiscardAnalysisWhenLargestIsLoading) {
-  SetBodyInnerHTML(R"HTML(
-    <style>img { display:block }</style>
-    <div id="parent">
-      <img height="5" width="5" id="1"></img>
-      <img height="9" width="9" id="2"></img>
-    </div>
-  )HTML");
-  SetImageAndPaint("1", 5, 5);
-  UpdateAllLifecyclePhasesForTest();
-  ImageRecord* record;
-  InvokeCallback();
-  record = FindLargestPaintCandidate();
-  EXPECT_FALSE(record);
-
-  SetImageAndPaint("2", 9, 9);
-  UpdateAllLifecyclePhasesForTest();
-  InvokeCallback();
-  record = FindLargestPaintCandidate();
-  EXPECT_TRUE(record);
-  EXPECT_EQ(record->first_size, 81ul);
-  EXPECT_FALSE(record->first_paint_time_after_loaded.is_null());
-}
-
 // This is to prove that a swap time is assigned only to nodes of the frame who
 // register the swap time. In other words, swap time A should match frame A;
 // swap time B should match frame B.
@@ -632,8 +604,6 @@ TEST_F(ImagePaintTimingDetectorTest, BackgroundImage_IgnoreBody) {
         background-image: url(data:image/gif;base64,R0lGODlhAQABAAAAACH5BAEKAAEALAAAAAABAAEAAAICTAEAOw==);
       }
     </style>
-    <body>
-    </body>
   )HTML");
   EXPECT_EQ(CountRecords(), 0u);
 }

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

@@ -18,6 +18,7 @@
 #include "third_party/blink/renderer/core/page/page.h"
 #include "third_party/blink/renderer/core/paint/image_element_timing.h"
 #include "third_party/blink/renderer/core/paint/paint_info.h"
+#include "third_party/blink/renderer/core/paint/paint_timing_detector.h"
 #include "third_party/blink/renderer/core/paint/scoped_paint_state.h"
 #include "third_party/blink/renderer/platform/geometry/layout_point.h"
 #include "third_party/blink/renderer/platform/graphics/paint/display_item_cache_skipper.h"
@@ -216,6 +217,12 @@ void ImagePainter::PaintIntoRect(GraphicsContext& context,
     ImageElementTiming::From(*window).NotifyImagePainted(
         &layout_image_, layout_image_.CachedImage(), painting_layer);
   }
+
+  if (RuntimeEnabledFeatures::FirstContentfulPaintPlusPlusEnabled()) {
+    PaintTimingDetector::NotifyImagePaint(
+        layout_image_,
+        context.GetPaintController().CurrentPaintChunkProperties());
+  }
 }
 
 }  // namespace blink

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

@@ -13,6 +13,7 @@
 #include "third_party/blink/renderer/core/paint/paint_layer.h"
 #include "third_party/blink/renderer/core/paint/text_paint_timing_detector.h"
 #include "third_party/blink/renderer/platform/geometry/int_rect.h"
+#include "third_party/blink/renderer/platform/graphics/image.h"
 #include "third_party/blink/renderer/platform/graphics/paint/float_clip_rect.h"
 #include "third_party/blink/renderer/platform/graphics/paint/geometry_mapper.h"
 #include "third_party/blink/renderer/platform/graphics/paint/property_tree_state.h"
@@ -29,21 +30,58 @@ PaintTimingDetector::PaintTimingDetector(LocalFrameView* frame_view)
 
 void PaintTimingDetector::NotifyPaintFinished() {
   text_paint_timing_detector_->OnPaintFinished();
-  image_paint_timing_detector_->OnPrePaintFinished();
+  image_paint_timing_detector_->OnPaintFinished();
 }
 
-void PaintTimingDetector::NotifyObjectPrePaint(
-    const LayoutObject& object,
-    const PaintLayer& painting_layer) {
-  // Todo(maxlg): incoperate iframe's statistics
-  if (!frame_view_->GetFrame().IsMainFrame())
+// static
+void PaintTimingDetector::NotifyBackgroundImagePaint(
+    const Node* node,
+    Image* image,
+    const PropertyTreeState& current_paint_chunk_properties) {
+  DCHECK(image);
+  if (!node)
+    return;
+  LayoutObject* object = node->GetLayoutObject();
+  if (!object)
+    return;
+  if (!ImagePaintTimingDetector::IsBackgroundImageContentful(*object, *image))
     return;
+  // TODO(crbug/936149): This check is needed because the |image| and the
+  // background images in node could have inconsistent state. This can be
+  // resolved by tracking each background image separately. We will no longer
+  // need to find background images from a node's layers.
+  if (!ImagePaintTimingDetector::HasBackgroundImage(*object))
+    return;
+  LocalFrameView* frame_view = object->GetFrameView();
+  if (!frame_view || !frame_view->GetFrame().IsMainFrame())
+    return;
+  PaintTimingDetector& detector = frame_view->GetPaintTimingDetector();
+  detector.GetImagePaintTimingDetector().RecordImage(
+      *object, current_paint_chunk_properties);
+}
 
-  if (object.IsLayoutImage() || object.IsVideo() || object.IsSVGImage() ||
-      ImagePaintTimingDetector::HasContentfulBackgroundImage(object)) {
-    image_paint_timing_detector_->RecordImage(object, painting_layer);
-  }
-  // Todo(maxlg): add other detectors here.
+// static
+void PaintTimingDetector::NotifyImagePaint(
+    const Node* node,
+    const PropertyTreeState& current_paint_chunk_properties) {
+  if (!node)
+    return;
+  LayoutObject* object = node->GetLayoutObject();
+  if (!object)
+    return;
+  NotifyImagePaint(*object, current_paint_chunk_properties);
+}
+
+// static
+void PaintTimingDetector::NotifyImagePaint(
+    const LayoutObject& object,
+    const PropertyTreeState& current_paint_chunk_properties) {
+  LocalFrameView* frame_view = object.GetFrameView();
+  if (!frame_view || !frame_view->GetFrame().IsMainFrame())
+    return;
+  PaintTimingDetector& detector = frame_view->GetPaintTimingDetector();
+  detector.GetImagePaintTimingDetector().RecordImage(
+      object, current_paint_chunk_properties);
 }
 
 // static

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

@@ -19,6 +19,7 @@ class LayoutRect;
 class TextPaintTimingDetector;
 class ImagePaintTimingDetector;
 class PropertyTreeState;
+class Image;
 
 // PaintTimingDetector contains some of paint metric detectors,
 // providing common infrastructure for these detectors.
@@ -31,8 +32,21 @@ class CORE_EXPORT PaintTimingDetector
     : public GarbageCollected<PaintTimingDetector> {
  public:
   PaintTimingDetector(LocalFrameView*);
-  void NotifyObjectPrePaint(const LayoutObject& object,
-                            const PaintLayer& painting_layer);
+
+  // TODO(crbug/936124): the detector no longer need to look for background
+  // images in each layer.
+  // Notify the paint of background image.
+  static void NotifyBackgroundImagePaint(
+      const Node* node,
+      Image* image,
+      const PropertyTreeState& current_paint_chunk_properties);
+  static void NotifyImagePaint(
+      const Node* node,
+      const PropertyTreeState& current_paint_chunk_properties);
+  static void NotifyImagePaint(
+      const LayoutObject& object,
+      const PropertyTreeState& current_paint_chunk_properties);
+
   static void NotifyTextPaint(const Node* node, const PropertyTreeState&);
   static void NotifyTextPaint(const LayoutObject& object,
                               const PropertyTreeState&);

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

@@ -24,7 +24,6 @@
 #include "third_party/blink/renderer/core/paint/ng/ng_paint_fragment.h"
 #include "third_party/blink/renderer/core/paint/paint_layer.h"
 #include "third_party/blink/renderer/core/paint/paint_property_tree_printer.h"
-#include "third_party/blink/renderer/core/paint/paint_timing_detector.h"
 #include "third_party/blink/renderer/platform/graphics/paint/geometry_mapper.h"
 
 namespace blink {
@@ -380,10 +379,6 @@ void PrePaintTreeWalk::WalkInternal(const LayoutObject& object,
         object, paint_invalidator_context.old_visual_rect,
         *paint_invalidator_context.painting_layer);
   }
-  if (RuntimeEnabledFeatures::FirstContentfulPaintPlusPlusEnabled()) {
-    object.GetFrameView()->GetPaintTimingDetector().NotifyObjectPrePaint(
-        object, *paint_invalidator_context.painting_layer);
-  }
 }
 
 void PrePaintTreeWalk::Walk(const LayoutObject& object) {

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

@@ -10,6 +10,7 @@
 #include "third_party/blink/renderer/core/origin_trials/origin_trials.h"
 #include "third_party/blink/renderer/core/paint/image_element_timing.h"
 #include "third_party/blink/renderer/core/paint/paint_info.h"
+#include "third_party/blink/renderer/core/paint/paint_timing_detector.h"
 #include "third_party/blink/renderer/core/paint/scoped_svg_paint_state.h"
 #include "third_party/blink/renderer/core/paint/svg_model_object_painter.h"
 #include "third_party/blink/renderer/core/svg/graphics/svg_image.h"
@@ -87,6 +88,12 @@ void SVGImagePainter::PaintForeground(const PaintInfo& paint_info) {
         &layout_svg_image_, image_resource->CachedImage(),
         paint_info.PaintContainer()->Layer());
   }
+
+  if (RuntimeEnabledFeatures::FirstContentfulPaintPlusPlusEnabled()) {
+    PaintTimingDetector::NotifyImagePaint(
+        layout_svg_image_,
+        paint_info.context.GetPaintController().CurrentPaintChunkProperties());
+  }
 }
 
 FloatSize SVGImagePainter::ComputeImageViewportSize() const {

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

@@ -212,6 +212,8 @@ void TextPaintTimingDetector::RecordText(
     rect_size = frame_view_->GetPaintTimingDetector().CalculateVisualSize(
         visual_rect, current_paint_chunk_properties);
   }
+  DVLOG(2) << "Node id (" << node_id << "): size=" << rect_size
+           << ", type=" << object.DebugName();
 
   // When rect_size == 0, it either means the text size is 0 or the text is out
   // of viewport. In either case, we don't record their time for efficiency.