[LCP] Image: combine normal image and background image handling logic

LargestImagePaint handles both normal layout images and background
images. Currently these two kinds of images are handled by two
different functions. It poses a challenge for maintenance, as we need
double effort in making change the handling logic. This CL combines
the two.

The main difference for the two kinds of images in LCP is that
background image uses (node_id, cached_image) as the key for storage,
whereas the normal image uses only node_id.

It's based on the fact that we can use (node_id, cached_image) to
identify normal image as well as background image.

As a side effect, this change automatically solves crbug.com/965653,
where the original version didn't handle the case where a layout image
changed its src.

A follow-up patch will correct the variable names.

Bug: 965653, 972146
Change-Id: I33a7961c663b7a24b4116e23c4a2f1052c864ab6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1692494Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
Commit-Queue: Liquan (Max) Gu <maxlg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#675849}

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

@@ -155,16 +155,6 @@ void ImagePaintTimingDetector::OnPaintFinished() {
   RegisterNotifySwapTime();
 }
 
-void ImagePaintTimingDetector::LayoutObjectWillBeDestroyed(DOMNodeId node_id) {
-  if (!is_recording_)
-    return;
-  // Todo: check whether it is visible background image.
-  if (!records_manager_.IsRecordedVisibleNode(node_id))
-    return;
-  records_manager_.RemoveVisibleRecord(node_id);
-  need_update_timing_at_frame_end_ = true;
-}
-
 void ImagePaintTimingDetector::NotifyBackgroundImageRemoved(
     DOMNodeId node_id,
     const ImageResourceContent* cached_image) {
@@ -174,6 +164,7 @@ void ImagePaintTimingDetector::NotifyBackgroundImageRemoved(
   if (!records_manager_.IsRecordedVisibleNode(background_image_id))
     return;
   records_manager_.RemoveVisibleRecord(background_image_id);
+  need_update_timing_at_frame_end_ = true;
 }
 
 void ImagePaintTimingDetector::RegisterNotifySwapTime() {
@@ -286,63 +277,6 @@ void ImagePaintTimingDetector::RecordBackgroundImage(
     HandleTooManyNodes();
 }
 
-void ImagePaintTimingDetector::RecordImage(
-    const LayoutObject& object,
-    const IntSize& intrinsic_size,
-    const ImageResourceContent& cached_image,
-    const PropertyTreeState& current_paint_chunk_properties) {
-  // TODO(crbug.com/933479): Use LayoutObject::GeneratingNode() to include
-  // anonymous objects' rect.
-  Node* node = object.GetNode();
-  if (!node)
-    return;
-
-  DOMNodeId node_id = DOMNodeIds::IdForNode(node);
-  DCHECK_NE(node_id, kInvalidDOMNodeId);
-
-  if (records_manager_.IsRecordedInvisibleNode(node_id))
-    return;
-
-  bool is_loaded = cached_image.IsLoaded();
-  bool is_recored_visible_node =
-      records_manager_.IsRecordedVisibleNode(node_id);
-  if (is_recored_visible_node &&
-      !records_manager_.WasVisibleNodeLoaded(node_id) && is_loaded) {
-    records_manager_.OnImageLoaded(node_id, frame_index_);
-    need_update_timing_at_frame_end_ = true;
-    return;
-  }
-
-  if (is_recored_visible_node || !is_recording_)
-    return;
-  IntRect visual_rect = object.FragmentsVisualRectBoundingBox();
-  // Before the image resource starts loading, <img> has no size info. We wait
-  // until the size is known.
-  if (visual_rect.IsEmpty())
-    return;
-  uint64_t rect_size =
-      frame_view_->GetPaintTimingDetector()
-          .CalculateVisualRect(visual_rect, current_paint_chunk_properties)
-          .Size()
-          .Area();
-  rect_size = DownScaleIfIntrinsicSizeIsSmaller(
-      rect_size, intrinsic_size.Area(), visual_rect.Size().Area());
-  DVLOG(2) << "Node id (" << node_id << "): size=" << rect_size
-           << ", type=" << object.DebugName();
-  if (rect_size == 0) {
-    records_manager_.RecordInvisibleNode(node_id);
-  } else {
-    records_manager_.RecordVisibleNode(node_id, rect_size, cached_image);
-    if (is_loaded) {
-      records_manager_.OnImageLoaded(node_id, frame_index_);
-      need_update_timing_at_frame_end_ = true;
-    }
-  }
-
-  if (records_manager_.RecordedTooManyNodes())
-    HandleTooManyNodes();
-}
-
 void ImagePaintTimingDetector::HandleTooManyNodes() {
   TRACE_EVENT_INSTANT0("loading", "ImagePaintTimingDetector::OverNodeLimit",
                        TRACE_EVENT_SCOPE_THREAD);
@@ -352,12 +286,6 @@ void ImagePaintTimingDetector::HandleTooManyNodes() {
 ImageRecordsManager::ImageRecordsManager()
     : size_ordered_set_(&LargeImageFirst) {}
 
-void ImageRecordsManager::OnImageLoaded(const DOMNodeId& node_id,
-                                        unsigned current_frame_index) {
-  base::WeakPtr<ImageRecord> record = FindVisibleRecord(node_id);
-  OnImageLoadedInternal(record, current_frame_index);
-}
-
 void ImageRecordsManager::OnImageLoaded(
     const BackgroundImageId& background_image_id,
     unsigned current_frame_index) {
@@ -372,16 +300,6 @@ void ImageRecordsManager::OnImageLoadedInternal(
   QueueToMeasurePaintTime(record, current_frame_index);
 }
 
-void ImageRecordsManager::RecordVisibleNode(
-    const DOMNodeId& node_id,
-    const uint64_t& visual_size,
-    const ImageResourceContent& cached_image) {
-  std::unique_ptr<ImageRecord> record =
-      CreateImageRecord(node_id, &cached_image, visual_size);
-  size_ordered_set_.insert(record->AsWeakPtr());
-  visible_node_map_.insert(node_id, std::move(record));
-}
-
 void ImageRecordsManager::RecordVisibleNode(
     const BackgroundImageId& background_image_id,
     const uint64_t& visual_size) {
@@ -406,8 +324,7 @@ std::unique_ptr<ImageRecord> ImageRecordsManager::CreateImageRecord(
 }
 
 ImageRecord* ImageRecordsManager::FindLargestPaintCandidate() const {
-  DCHECK_EQ(visible_node_map_.size() + visible_background_image_map_.size(),
-            size_ordered_set_.size());
+  DCHECK_EQ(visible_background_image_map_.size(), size_ordered_set_.size());
   if (size_ordered_set_.size() == 0)
     return nullptr;
   return size_ordered_set_.begin()->get();

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

@@ -65,14 +65,7 @@ class CORE_EXPORT ImageRecordsManager {
   ImageRecord* FindLargestPaintCandidate() const;
 
   bool AreAllVisibleNodesDetached() const;
-  inline void RemoveVisibleRecord(const DOMNodeId& visible_node_id) {
-    base::WeakPtr<ImageRecord> record =
-        visible_node_map_.find(visible_node_id)->value->AsWeakPtr();
-    size_ordered_set_.erase(record);
-    visible_node_map_.erase(visible_node_id);
-    // Leave out |images_queued_for_paint_time_| intentionally because the null
-    // record will be removed in |AssignPaintTimeToRegisteredQueuedNodes|.
-  }
+
   inline void RemoveVisibleRecord(
       const BackgroundImageId& background_image_id) {
     base::WeakPtr<ImageRecord> record =
@@ -88,16 +81,9 @@ class CORE_EXPORT ImageRecordsManager {
     DCHECK(!RecordedTooManyNodes());
     invisible_node_ids_.insert(node_id);
   }
-  void RecordVisibleNode(const DOMNodeId&,
-                         const uint64_t& visual_size,
-                         const ImageResourceContent&);
   void RecordVisibleNode(const BackgroundImageId& background_image_id,
                          const uint64_t& visual_size);
-  size_t CountVisibleNodes() const { return visible_node_map_.size(); }
   size_t CountInvisibleNodes() const { return invisible_node_ids_.size(); }
-  bool IsRecordedVisibleNode(const DOMNodeId& node_id) const {
-    return visible_node_map_.Contains(node_id);
-  }
   bool IsRecordedVisibleNode(
       const BackgroundImageId& background_image_id) const {
     return visible_background_image_map_.Contains(background_image_id);
@@ -107,21 +93,15 @@ class CORE_EXPORT ImageRecordsManager {
   }
 
   inline bool RecordedTooManyNodes() const {
-    return visible_node_map_.size() + visible_background_image_map_.size() +
-               invisible_node_ids_.size() >
+    return visible_background_image_map_.size() + invisible_node_ids_.size() >
            kImageNodeNumberLimit;
   }
 
-  inline bool WasVisibleNodeLoaded(const DOMNodeId& node_id) const {
-    DCHECK(visible_node_map_.Contains(node_id));
-    return visible_node_map_.at(node_id)->loaded;
-  }
   inline bool WasVisibleNodeLoaded(
       const BackgroundImageId& background_image_id) const {
     DCHECK(visible_background_image_map_.Contains(background_image_id));
     return visible_background_image_map_.at(background_image_id)->loaded;
   }
-  void OnImageLoaded(const DOMNodeId&, unsigned current_frame_index);
   void OnImageLoaded(const BackgroundImageId&, unsigned current_frame_index);
   void OnImageLoadedInternal(base::WeakPtr<ImageRecord>&,
                              unsigned current_frame_index);
@@ -152,11 +132,6 @@ class CORE_EXPORT ImageRecordsManager {
 
  private:
   // Find the image record of an visible image.
-  inline base::WeakPtr<ImageRecord> FindVisibleRecord(
-      const DOMNodeId& node_id) const {
-    DCHECK(visible_node_map_.Contains(node_id));
-    return visible_node_map_.find(node_id)->value->AsWeakPtr();
-  }
   inline base::WeakPtr<ImageRecord> FindVisibleRecord(
       const BackgroundImageId& background_image_id) const {
     DCHECK(visible_background_image_map_.Contains(background_image_id));
@@ -177,9 +152,6 @@ class CORE_EXPORT ImageRecordsManager {
   }
 
   unsigned max_record_id_ = 0;
-  // We will never destroy the pointers within |visible_node_map_|. Once created
-  // they will exist for the whole life cycle of |visible_node_map_|.
-  HashMap<DOMNodeId, std::unique_ptr<ImageRecord>> visible_node_map_;
   HashMap<BackgroundImageId, std::unique_ptr<ImageRecord>>
       visible_background_image_map_;
   HashSet<DOMNodeId> invisible_node_ids_;

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

@@ -97,15 +97,6 @@ class ImagePaintTimingDetectorTest
   }
 
   size_t CountVisibleImageRecords() {
-    if (!GetPaintTimingDetector().GetImagePaintTimingDetector())
-      return 0;
-
-    return GetPaintTimingDetector()
-        .GetImagePaintTimingDetector()
-        ->records_manager_.visible_node_map_.size();
-  }
-
-  size_t CountVisibleBackgroundImageRecords() {
     return GetPaintTimingDetector()
         .GetImagePaintTimingDetector()
         ->records_manager_.visible_background_image_map_.size();
@@ -115,9 +106,6 @@ class ImagePaintTimingDetectorTest
     return GetPaintTimingDetector()
                .GetImagePaintTimingDetector()
                ->records_manager_.visible_background_image_map_.size() +
-           GetPaintTimingDetector()
-               .GetImagePaintTimingDetector()
-               ->records_manager_.visible_node_map_.size() +
            GetPaintTimingDetector()
                .GetImagePaintTimingDetector()
                ->records_manager_.size_ordered_set_.size() +
@@ -129,7 +117,7 @@ class ImagePaintTimingDetectorTest
   size_t CountChildFrameRecords() {
     return GetChildPaintTimingDetector()
         .GetImagePaintTimingDetector()
-        ->records_manager_.visible_node_map_.size();
+        ->records_manager_.visible_background_image_map_.size();
   }
 
   size_t CountRankingSetRecords() {
@@ -392,7 +380,6 @@ TEST_F(ImagePaintTimingDetectorTest, LargestImagePaint_OpacityZero) {
   SetImageAndPaint("target", 5, 5);
   UpdateAllLifecyclePhasesAndInvokeCallbackIfAny();
   EXPECT_EQ(CountVisibleImageRecords(), 0u);
-  EXPECT_EQ(CountVisibleBackgroundImageRecords(), 0u);
   ImageRecord* record = FindLargestPaintCandidate();
   EXPECT_FALSE(record);
 }
@@ -409,7 +396,6 @@ TEST_F(ImagePaintTimingDetectorTest, LargestImagePaint_VisibilityHidden) {
   SetImageAndPaint("target", 5, 5);
   UpdateAllLifecyclePhasesAndInvokeCallbackIfAny();
   EXPECT_EQ(CountVisibleImageRecords(), 0u);
-  EXPECT_EQ(CountVisibleBackgroundImageRecords(), 0u);
   ImageRecord* record = FindLargestPaintCandidate();
   EXPECT_FALSE(record);
 }
@@ -426,7 +412,6 @@ TEST_F(ImagePaintTimingDetectorTest, LargestImagePaint_DisplayNone) {
   SetImageAndPaint("target", 5, 5);
   UpdateAllLifecyclePhasesAndInvokeCallbackIfAny();
   EXPECT_EQ(CountVisibleImageRecords(), 0u);
-  EXPECT_EQ(CountVisibleBackgroundImageRecords(), 0u);
   ImageRecord* record = FindLargestPaintCandidate();
   EXPECT_FALSE(record);
 }
@@ -443,7 +428,6 @@ TEST_F(ImagePaintTimingDetectorTest, LargestImagePaint_OpacityNonZero) {
   SetImageAndPaint("target", 5, 5);
   UpdateAllLifecyclePhasesAndInvokeCallbackIfAny();
   EXPECT_EQ(CountVisibleImageRecords(), 1u);
-  EXPECT_EQ(CountVisibleBackgroundImageRecords(), 0u);
   ImageRecord* record = FindLargestPaintCandidate();
   EXPECT_TRUE(record);
 }
@@ -808,7 +792,7 @@ TEST_F(ImagePaintTimingDetectorTest, BackgroundImage) {
   )HTML");
   ImageRecord* record = FindLargestPaintCandidate();
   EXPECT_TRUE(record);
-  EXPECT_EQ(CountVisibleBackgroundImageRecords(), 1u);
+  EXPECT_EQ(CountVisibleImageRecords(), 1u);
 }
 
 TEST_F(ImagePaintTimingDetectorTest,
@@ -825,8 +809,7 @@ TEST_F(ImagePaintTimingDetectorTest,
   )HTML");
   SetImageAndPaint("target", 1, 1);
   UpdateAllLifecyclePhasesAndInvokeCallbackIfAny();
-  EXPECT_EQ(CountVisibleBackgroundImageRecords(), 1u);
-  EXPECT_EQ(CountVisibleImageRecords(), 1u);
+  EXPECT_EQ(CountVisibleImageRecords(), 2u);
   ImageRecord* record = FindLargestPaintCandidate();
   EXPECT_TRUE(record);
   EXPECT_EQ(record->first_size, 1u);
@@ -840,7 +823,7 @@ TEST_F(ImagePaintTimingDetectorTest, BackgroundImage_IgnoreBody) {
       }
     </style>
   )HTML");
-  EXPECT_EQ(CountVisibleBackgroundImageRecords(), 0u);
+  EXPECT_EQ(CountVisibleImageRecords(), 0u);
 }
 
 TEST_F(ImagePaintTimingDetectorTest, BackgroundImage_IgnoreHtml) {
@@ -853,7 +836,7 @@ TEST_F(ImagePaintTimingDetectorTest, BackgroundImage_IgnoreHtml) {
     </style>
     </html>
   )HTML");
-  EXPECT_EQ(CountVisibleBackgroundImageRecords(), 0u);
+  EXPECT_EQ(CountVisibleImageRecords(), 0u);
 }
 
 TEST_F(ImagePaintTimingDetectorTest, BackgroundImage_IgnoreGradient) {
@@ -867,7 +850,7 @@ TEST_F(ImagePaintTimingDetectorTest, BackgroundImage_IgnoreGradient) {
       place-holder
     </div>
   )HTML");
-  EXPECT_EQ(CountVisibleBackgroundImageRecords(), 0u);
+  EXPECT_EQ(CountVisibleImageRecords(), 0u);
 }
 
 // We put two background images in the same object, and test whether FCP++ can
@@ -885,7 +868,7 @@ TEST_F(ImagePaintTimingDetectorTest, BackgroundImageTrackedDifferently) {
     </style>
     <div id="d"></div>
   )HTML");
-  EXPECT_EQ(CountVisibleBackgroundImageRecords(), 2u);
+  EXPECT_EQ(CountVisibleImageRecords(), 2u);
 }
 
 TEST_F(ImagePaintTimingDetectorTest, DeactivateAfterUserInput) {
@@ -897,7 +880,7 @@ TEST_F(ImagePaintTimingDetectorTest, DeactivateAfterUserInput) {
   SimulateScroll();
   SetImageAndPaint("target", 5, 5);
   UpdateAllLifecyclePhasesAndInvokeCallbackIfAny();
-  EXPECT_EQ(CountVisibleImageRecords(), 0u);
+  EXPECT_FALSE(GetPaintTimingDetector().GetImagePaintTimingDetector());
 }
 
 TEST_F(ImagePaintTimingDetectorTest, NullTimeNoCrash) {

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

@@ -110,23 +110,16 @@ void PaintTimingDetector::NotifyImagePaint(
   if (!cached_image)
     return;
   PaintTimingDetector& detector = frame_view->GetPaintTimingDetector();
-  if (detector.GetImagePaintTimingDetector()) {
-    detector.GetImagePaintTimingDetector()->RecordImage(
-        object, intrinsic_size, *cached_image, current_paint_chunk_properties);
-  }
+  if (!detector.GetImagePaintTimingDetector())
+    return;
+  detector.GetImagePaintTimingDetector()->RecordBackgroundImage(
+      object, intrinsic_size, *cached_image, current_paint_chunk_properties);
 }
 
 void PaintTimingDetector::LayoutObjectWillBeDestroyed(
     const LayoutObject& object) {
   if (text_paint_timing_detector_)
     text_paint_timing_detector_->LayoutObjectWillBeDestroyed(object);
-
-  DOMNodeId node_id = DOMNodeIds::ExistingIdForNode(object.GetNode());
-  if (node_id == kInvalidDOMNodeId)
-    return;
-
-  if (image_paint_timing_detector_)
-    image_paint_timing_detector_->LayoutObjectWillBeDestroyed(node_id);
 }
 
 void PaintTimingDetector::NotifyBackgroundImageRemoved(