[FCP++] Image: ensure (time==0 && size>0) represent "still loading or not painted"

Now that we already have the image time and size in
PaintTimingDetector, we should deprecate largest_image_paint_ which
stores duplicate information.

Correct the order of DidChangePerformanceTiming() and
NotifyLargestImage(). NotifyLargestImage() is about changing the data
that PerformanceTiming should fetch. This is why
DidChangePerformanceTiming() should come after NotifyLargestImage().

Bug:949974

Change-Id: I2b0801ce56cf327a8187273ac537e71d0e352533
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1596622
Commit-Queue: Liquan (Max) Gu <maxlg@chromium.org>
Reviewed-by: Steve Kobes <skobes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#658288}

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

@@ -92,10 +92,9 @@ void ImagePaintTimingDetector::PopulateTraceValue(
                    !frame_view_->GetFrame().LocalFrameRoot().IsMainFrame());
 }
 
-void ImagePaintTimingDetector::OnLargestImagePaintDetected(
+void ImagePaintTimingDetector::ReportCandidateToTrace(
     ImageRecord& largest_image_record) {
   DCHECK(!largest_image_record.paint_time.is_null());
-  largest_image_paint_ = &largest_image_record;
   auto value = std::make_unique<TracedValue>();
   PopulateTraceValue(*value, largest_image_record, ++count_candidates_);
   TRACE_EVENT_MARK_WITH_TIMESTAMP2("loading", "LargestImagePaint::Candidate",
@@ -107,33 +106,27 @@ void ImagePaintTimingDetector::OnLargestImagePaintDetected(
 void ImagePaintTimingDetector::UpdateCandidate() {
   ImageRecord* largest_image_record =
       records_manager_.FindLargestPaintCandidate();
-  // These conditions represents the following scenarios:
-  // 1. candiate being nullptr: no image is found. We discard the candidate and
-  // wait for the next analysis.
-  // 2. candidate's first paint being null: largest image is still pending for
-  // timing. We discard the candidate and wait for the next analysis.
-  // 3. new candidate equals to old candidate: we don't need to update the
-  // result.
-  if (largest_image_record == largest_image_paint_)
+  const base::TimeTicks time = largest_image_record
+                                   ? largest_image_record->paint_time
+                                   : base::TimeTicks();
+  const uint64_t size =
+      largest_image_record ? largest_image_record->first_size : 0;
+  bool changed =
+      frame_view_->GetPaintTimingDetector().HasLargestImagePaintChanged(time,
+                                                                        size);
+  if (!changed)
     return;
-
-  if (!largest_image_record) {
-    largest_image_paint_ = nullptr;
-    frame_view_->GetPaintTimingDetector().DidChangePerformanceTiming();
-  } else if (largest_image_record->loaded &&
-             !largest_image_record->paint_time.is_null()) {
-    OnLargestImagePaintDetected(*largest_image_record);
-    frame_view_->GetPaintTimingDetector().DidChangePerformanceTiming();
-  }
-  if (largest_image_paint_) {
-    frame_view_->GetPaintTimingDetector().NotifyLargestImage(
-        largest_image_paint_->paint_time, largest_image_paint_->first_size);
-  } else {
-    frame_view_->GetPaintTimingDetector().NotifyLargestImage(base::TimeTicks(),
-                                                             0);
+  if (largest_image_record && !largest_image_record->paint_time.is_null()) {
+    // If an image has paint time, it must have been loaded.
+    DCHECK(largest_image_record->loaded);
+    // TODO(crbug.com/960365): we need to figure out how to communicate to the
+    // trace (devtools/trace-viewer/benchmarks) that the largest image is still
+    // loading (when paint_time is null). Before we decide how to handle it, we
+    // do not notify the trace about this specific case for now.
+    ReportCandidateToTrace(*largest_image_record);
   }
-  // TODO(crbug/949974): when the largest image is still loading, we should
-  // update the result as the current time.
+  frame_view_->GetPaintTimingDetector().NotifyLargestImagePaintChange(time,
+                                                                      size);
 }
 
 void ImagePaintTimingDetector::OnPaintFinished() {
@@ -242,6 +235,7 @@ void ImagePaintTimingDetector::RecordBackgroundImage(
       !records_manager_.WasVisibleNodeLoaded(background_image_id) &&
       cached_image.IsLoaded()) {
     records_manager_.OnImageLoaded(background_image_id, frame_index_);
+    need_update_timing_at_frame_end_ = true;
     return;
   }
 
@@ -268,8 +262,10 @@ void ImagePaintTimingDetector::RecordBackgroundImage(
     records_manager_.RecordInvisibleNode(node_id);
   } else {
     records_manager_.RecordVisibleNode(background_image_id, rect_size);
-    if (cached_image.IsLoaded())
+    if (cached_image.IsLoaded()) {
       records_manager_.OnImageLoaded(background_image_id, frame_index_);
+      need_update_timing_at_frame_end_ = true;
+    }
   }
 
   if (records_manager_.RecordedTooManyNodes())
@@ -301,6 +297,7 @@ void ImagePaintTimingDetector::RecordImage(
   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;
   }
 
@@ -325,8 +322,10 @@ void ImagePaintTimingDetector::RecordImage(
     records_manager_.RecordInvisibleNode(node_id);
   } else {
     records_manager_.RecordVisibleNode(node_id, rect_size);
-    if (is_loaded)
+    if (is_loaded) {
       records_manager_.OnImageLoaded(node_id, frame_index_);
+      need_update_timing_at_frame_end_ = true;
+    }
   }
 
   if (records_manager_.RecordedTooManyNodes())
@@ -464,7 +463,7 @@ bool ImagePaintTimingDetector::FinishedReportingImages() const {
   return !is_recording_ && num_pending_swap_callbacks_ == 0;
 }
 
-ImageRecord* ImageRecordsManager::FindLargestPaintCandidate() {
+ImageRecord* ImageRecordsManager::FindLargestPaintCandidate() const {
   DCHECK_EQ(visible_node_map_.size() + visible_background_image_map_.size(),
             size_ordered_set_.size());
   for (auto it = size_ordered_set_.begin(); it != size_ordered_set_.end();

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

@@ -33,7 +33,7 @@ class ImageRecord : public base::SupportsWeakPtr<ImageRecord> {
   // |size_ordered_set_| since it's the sorting key.
   uint64_t first_size = 0;
   unsigned frame_index = 0;
-  // The time of the first paint after fully loaded.
+  // The time of the first paint after fully loaded. 0 means not painted yet.
   base::TimeTicks paint_time = base::TimeTicks();
   WeakPersistent<const ImageResourceContent> cached_image;
   bool loaded = false;
@@ -56,7 +56,7 @@ class CORE_EXPORT ImageRecordsManager {
 
  public:
   ImageRecordsManager();
-  ImageRecord* FindLargestPaintCandidate();
+  ImageRecord* FindLargestPaintCandidate() const;
 
   bool AreAllVisibleNodesDetached() const;
   void SetNodeDetached(const DOMNodeId& visible_node_id);
@@ -182,7 +182,7 @@ class CORE_EXPORT ImagePaintTimingDetector final
   void Trace(blink::Visitor*);
 
  private:
-  ImageRecord* FindLargestPaintCandidate();
+  ImageRecord* FindLargestPaintCandidate() const;
 
   void PopulateTraceValue(TracedValue&,
                           const ImageRecord& first_image_paint,
@@ -192,7 +192,7 @@ class CORE_EXPORT ImagePaintTimingDetector final
                       WebWidgetClient::SwapResult,
                       base::TimeTicks);
   void RegisterNotifySwapTime();
-  void OnLargestImagePaintDetected(ImageRecord&);
+  void ReportCandidateToTrace(ImageRecord&);
   void Deactivate();
   void HandleTooManyNodes();
 
@@ -216,9 +216,10 @@ class CORE_EXPORT ImagePaintTimingDetector final
   // |is_recording|, helps determine whether this detector can be destroyed.
   int num_pending_swap_callbacks_ = 0;
 
+  // This need to be set whenever changes that can affect the output of
+  // |FindLargestPaintCandidate| occur during the paint tree walk.
   bool need_update_timing_at_frame_end_ = false;
 
-  ImageRecord* largest_image_paint_ = nullptr;
   ImageRecordsManager records_manager_;
   Member<LocalFrameView> frame_view_;
 };

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

@@ -7,6 +7,7 @@
 #include "base/bind.h"
 #include "build/build_config.h"
 #include "third_party/blink/public/platform/web_url_loader_mock_factory.h"
+#include "third_party/blink/public/web/web_performance.h"
 #include "third_party/blink/public/web/web_widget_client.h"
 #include "third_party/blink/renderer/core/html/html_image_element.h"
 #include "third_party/blink/renderer/core/html/media/html_video_element.h"
@@ -14,6 +15,9 @@
 #include "third_party/blink/renderer/core/scroll/scroll_types.h"
 #include "third_party/blink/renderer/core/svg/svg_image_element.h"
 #include "third_party/blink/renderer/core/testing/core_unit_test_helper.h"
+#include "third_party/blink/renderer/core/timing/dom_window_performance.h"
+#include "third_party/blink/renderer/core/timing/performance_timing.h"
+#include "third_party/blink/renderer/core/timing/window_performance.h"
 #include "third_party/blink/renderer/platform/graphics/static_bitmap_image.h"
 #include "third_party/blink/renderer/platform/heap/heap.h"
 #include "third_party/blink/renderer/platform/runtime_enabled_features.h"
@@ -57,6 +61,12 @@ class ImagePaintTimingDetectorTest
     return GetChildFrameView().GetPaintTimingDetector();
   }
 
+  const PerformanceTiming& GetPerformanceTiming() {
+    PerformanceTiming* performance =
+        DOMWindowPerformance::performance(*GetFrame().DomWindow())->timing();
+    return *performance;
+  }
+
   IntRect GetViewportRect(LocalFrameView& view) {
     ScrollableArea* scrollable_area = view.GetScrollableArea();
     DCHECK(scrollable_area);
@@ -118,17 +128,13 @@ class ImagePaintTimingDetectorTest
   }
 
   TimeTicks LargestPaintStoredResult() {
-    ImageRecord* record = GetPaintTimingDetector()
-                              .GetImagePaintTimingDetector()
-                              ->largest_image_paint_;
-    return !record ? base::TimeTicks() : record->paint_time;
+    return GetPaintTimingDetector().largest_image_paint_time_;
   }
 
   void UpdateAllLifecyclePhasesAndInvokeCallbackIfAny() {
     UpdateAllLifecyclePhasesForTest();
-    if (!callback_queue_.empty()) {
+    if (!callback_queue_.empty())
       InvokeCallback();
-    }
   }
 
   void InvokeCallback() {
@@ -211,6 +217,48 @@ TEST_F(ImagePaintTimingDetectorTest, LargestImagePaint_OneImage) {
   EXPECT_TRUE(record->loaded);
 }
 
+TEST_F(ImagePaintTimingDetectorTest, UpdatePerformanceTiming) {
+  const PerformanceTiming& performance_timing = GetPerformanceTiming();
+  EXPECT_EQ(performance_timing.LargestImagePaintSize(), 0u);
+  EXPECT_EQ(performance_timing.LargestImagePaint(), 0u);
+  SetBodyInnerHTML(R"HTML(
+    <img id="target"></img>
+  )HTML");
+  SetImageAndPaint("target", 5, 5);
+  UpdateAllLifecyclePhasesAndInvokeCallbackIfAny();
+  EXPECT_EQ(performance_timing.LargestImagePaintSize(), 25u);
+  EXPECT_GT(performance_timing.LargestImagePaint(), 0u);
+}
+
+TEST_F(ImagePaintTimingDetectorTest,
+       PerformanceTimingHasZeroTimeNonZeroSizeWhenTheLargestIsNotPainted) {
+  const PerformanceTiming& performance_timing = GetPerformanceTiming();
+  EXPECT_EQ(performance_timing.LargestImagePaintSize(), 0u);
+  EXPECT_EQ(performance_timing.LargestImagePaint(), 0u);
+  SetBodyInnerHTML(R"HTML(
+    <img id="target"></img>
+  )HTML");
+  SetImageAndPaint("target", 5, 5);
+  UpdateAllLifecyclePhasesForTest();
+  EXPECT_EQ(performance_timing.LargestImagePaintSize(), 25u);
+  EXPECT_EQ(performance_timing.LargestImagePaint(), 0u);
+}
+
+TEST_F(ImagePaintTimingDetectorTest, UpdatePerformanceTimingToZero) {
+  SetBodyInnerHTML(R"HTML(
+    <img id="target"></img>
+  )HTML");
+  SetImageAndPaint("target", 5, 5);
+  UpdateAllLifecyclePhasesAndInvokeCallbackIfAny();
+  const PerformanceTiming& performance_timing = GetPerformanceTiming();
+  EXPECT_EQ(performance_timing.LargestImagePaintSize(), 25u);
+  EXPECT_GT(performance_timing.LargestImagePaint(), 0u);
+  GetDocument().body()->RemoveChild(GetDocument().getElementById("target"));
+  UpdateAllLifecyclePhasesAndInvokeCallbackIfAny();
+  EXPECT_EQ(performance_timing.LargestImagePaintSize(), 0u);
+  EXPECT_EQ(performance_timing.LargestImagePaint(), 0u);
+}
+
 TEST_F(ImagePaintTimingDetectorTest, LargestImagePaint_OpacityZero) {
   SetBodyInnerHTML(R"HTML(
     <style>

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

@@ -181,10 +181,20 @@ bool PaintTimingDetector::NeedToNotifyInputOrScroll() {
           image_paint_timing_detector_->IsRecording());
 }
 
-void PaintTimingDetector::NotifyLargestImage(base::TimeTicks image_paint_time,
-                                             uint64_t image_paint_size) {
+void PaintTimingDetector::NotifyLargestImagePaintChange(
+    base::TimeTicks image_paint_time,
+    uint64_t image_paint_size) {
+  DCHECK(HasLargestImagePaintChanged(image_paint_time, image_paint_size));
   largest_image_paint_time_ = image_paint_time;
   largest_image_paint_size_ = image_paint_size;
+  DidChangePerformanceTiming();
+}
+
+bool PaintTimingDetector::HasLargestImagePaintChanged(
+    base::TimeTicks largest_image_paint_time,
+    uint64_t largest_image_paint_size) {
+  return largest_image_paint_time != largest_image_paint_time_ ||
+         largest_image_paint_size != largest_image_paint_size_;
 }
 
 void PaintTimingDetector::NotifyLargestText(base::TimeTicks text_paint_time,

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

@@ -30,6 +30,8 @@ class TextPaintTimingDetector;
 // https://docs.google.com/document/d/1DRVd4a2VU8-yyWftgOparZF-sf16daf0vfbsHuz2rws/edit
 class CORE_EXPORT PaintTimingDetector
     : public GarbageCollected<PaintTimingDetector> {
+  friend class ImagePaintTimingDetectorTest;
+
  public:
   PaintTimingDetector(LocalFrameView*);
 
@@ -52,8 +54,9 @@ class CORE_EXPORT PaintTimingDetector
   void NotifyInputEvent(WebInputEvent::Type);
   bool NeedToNotifyInputOrScroll();
   void NotifyScroll(ScrollType);
-  void NotifyLargestImage(base::TimeTicks, uint64_t size);
+  void NotifyLargestImagePaintChange(base::TimeTicks, uint64_t size);
   void NotifyLargestText(base::TimeTicks, uint64_t size);
+  bool HasLargestImagePaintChanged(base::TimeTicks, uint64_t size);
   void DidChangePerformanceTiming();
 
   FloatRect CalculateVisualRect(const IntRect& visual_rect,