[LCP] Update LCP when both text and image are ready

Largest Contentful Paint compares the text and the image candidate
to decide which is the largest candidate. Currently, LCP generates
a result entry between updating the text candidate and the image
candidate, which produces an intermediate result. It causes
the issue in crbug.com/988115, #c4 explains the mechanism of the bug.

To remove the intermediate state, this CL changes the way
LCP-calculator takes the text candidate and image candidate. Instead
of notifying LCP-calculator of the text candidate's update right after
the update, we wait until both text candidate and image candidate have
been updated to notify LCP-calculator.

Also, LCP-calculator adds the logic of checking whether candidate
has changed, from ImagePaintTimingDetector and TextPaintTimingDetector.
Although it's redundant with |UpdateCandidate| in text and image,
these are necessary because going forwards, the "has_changed" logic in
both detectors would have to be removed along with the LIP and LTP
logic.

This CL also considers the chances where either detector is destroyed,
where it would no longer be able to find the largest candidate. In
this case, perf API would use the last reported candidate as the
candidate to compare with the candidate from another detector, in
order to decide the largest between text and image.

Bug: 982307, 988115

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

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

@@ -107,7 +107,7 @@ void ImagePaintTimingDetector::ReportNoCandidateToTrace() {
                ToTraceValue(&frame_view_->GetFrame()));
 }
 
-void ImagePaintTimingDetector::UpdateCandidate() {
+ImageRecord* ImagePaintTimingDetector::UpdateCandidate() {
   ImageRecord* largest_image_record =
       records_manager_.FindLargestPaintCandidate();
   const base::TimeTicks time = largest_image_record
@@ -116,27 +116,26 @@ void ImagePaintTimingDetector::UpdateCandidate() {
   const uint64_t size =
       largest_image_record ? largest_image_record->first_size : 0;
   PaintTimingDetector& detector = frame_view_->GetPaintTimingDetector();
+  // Two different candidates are rare to have the same time and size.
+  // So when they are unchanged, the candidate is considered unchanged.
   bool changed = detector.NotifyIfChangedLargestImagePaint(time, size);
-  if (!changed)
-    return;
-  if (!time.is_null()) {
-    if (auto* lcp_calculator = detector.GetLargestContentfulPaintCalculator())
-      lcp_calculator->OnLargestImageUpdated(largest_image_record);
-    // If an image has paint time, it must have been loaded.
-    DCHECK(largest_image_record->loaded);
-    ReportCandidateToTrace(*largest_image_record);
-  } else {
-    if (auto* lcp_calculator = detector.GetLargestContentfulPaintCalculator())
-      lcp_calculator->OnLargestImageUpdated(nullptr);
-    ReportNoCandidateToTrace();
+  if (changed) {
+    if (!time.is_null()) {
+      DCHECK(largest_image_record->loaded);
+      ReportCandidateToTrace(*largest_image_record);
+    } else {
+      ReportNoCandidateToTrace();
+    }
   }
+  return largest_image_record;
 }
 
 void ImagePaintTimingDetector::OnPaintFinished() {
   frame_index_++;
   if (need_update_timing_at_frame_end_) {
     need_update_timing_at_frame_end_ = false;
-    UpdateCandidate();
+    frame_view_->GetPaintTimingDetector()
+        .UpdateLargestContentfulPaintCandidate();
   }
 
   if (!records_manager_.HasUnregisteredRecordsInQueued(
@@ -187,7 +186,6 @@ void ImagePaintTimingDetector::ReportSwapTime(
   DCHECK(ThreadState::Current()->IsMainThread());
   records_manager_.AssignPaintTimeToRegisteredQueuedRecords(
       timestamp, last_queued_frame_index);
-  UpdateCandidate();
   num_pending_swap_callbacks_--;
   DCHECK_GE(num_pending_swap_callbacks_, 0);
 }

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

@@ -227,6 +227,9 @@ class CORE_EXPORT ImagePaintTimingDetector final
   }
   void ReportSwapTime(unsigned last_queued_frame_index, base::TimeTicks);
 
+  // Return the candidate.
+  ImageRecord* UpdateCandidate();
+
   void Trace(blink::Visitor*);
 
  private:
@@ -240,8 +243,6 @@ class CORE_EXPORT ImagePaintTimingDetector final
   void ReportNoCandidateToTrace();
   void Deactivate();
 
-  void UpdateCandidate();
-
   // Used to find the last candidate.
   unsigned count_candidates_ = 0;
 

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

@@ -140,9 +140,7 @@ class ImagePaintTimingDetectorTest
   }
 
   void UpdateCandidate() {
-    return GetPaintTimingDetector()
-        .GetImagePaintTimingDetector()
-        ->UpdateCandidate();
+    GetPaintTimingDetector().GetImagePaintTimingDetector()->UpdateCandidate();
   }
 
   base::TimeTicks LargestPaintStoredResult() {
@@ -206,6 +204,7 @@ class ImagePaintTimingDetectorTest
       MockPaintTimingCallbackManager* image_callback_manager) {
     image_callback_manager->InvokeSwapTimeCallback(
         test_task_runner_->NowTicks());
+    UpdateCandidate();
   }
 
   void SetImageAndPaint(AtomicString id, int width, int height) {

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

@@ -8,6 +8,32 @@
 
 namespace blink {
 
+namespace {
+bool HasLargestTextChanged(const std::unique_ptr<TextRecord>& a,
+                           const base::WeakPtr<TextRecord> b) {
+  if (!a && !b)
+    return false;
+  if (!a && b)
+    return true;
+  if (a && !b)
+    return true;
+  return a->node_id != b->node_id || a->first_size != b->first_size ||
+         a->paint_time != b->paint_time;
+}
+
+bool HasLargestImageChanged(const std::unique_ptr<ImageRecord>& a,
+                            const ImageRecord* b) {
+  if (!a && !b)
+    return false;
+  if (!a && b)
+    return true;
+  if (a && !b)
+    return true;
+  return a->node_id != b->node_id || a->first_size != b->first_size ||
+         a->paint_time != b->paint_time || a->load_time != b->load_time;
+}
+}  // namespace
+
 LargestContentfulPaintCalculator::LargestContentfulPaintCalculator(
     WindowPerformance* window_performance)
     : window_performance_(window_performance) {}
@@ -23,16 +49,6 @@ void LargestContentfulPaintCalculator::OnLargestImageUpdated(
     largest_image_->cached_image = largest_image->cached_image;
     largest_image_->load_time = largest_image->load_time;
   }
-
-  if (LargestImageSize() > LargestTextSize()) {
-    // The new largest image is the largest content, so report it as the LCP.
-    OnLargestContentfulPaintUpdated(LargestContentType::kImage);
-  } else if (largest_text_ && last_type_ == LargestContentType::kImage) {
-    // The text is at least as large as the new image. Because the last reported
-    // content type was image, this means that the largest image is now smaller
-    // and the largest text now needs to be reported as the LCP.
-    OnLargestContentfulPaintUpdated(LargestContentType::kText);
-  }
 }
 
 void LargestContentfulPaintCalculator::OnLargestTextUpdated(
@@ -43,24 +59,46 @@ void LargestContentfulPaintCalculator::OnLargestTextUpdated(
         largest_text->node_id, largest_text->first_size, FloatRect());
     largest_text_->paint_time = largest_text->paint_time;
   }
+}
 
+void LargestContentfulPaintCalculator::UpdateLargestContentPaintIfNeeded(
+    base::Optional<base::WeakPtr<TextRecord>> largest_text,
+    base::Optional<const ImageRecord*> largest_image) {
+  bool image_has_changed = false;
+  bool text_has_changed = false;
+  if (largest_image.has_value()) {
+    image_has_changed = HasLargestImageChanged(largest_image_, *largest_image);
+    OnLargestImageUpdated(*largest_image);
+  }
+  if (largest_text.has_value()) {
+    text_has_changed = HasLargestTextChanged(largest_text_, *largest_text);
+    OnLargestTextUpdated(*largest_text);
+  }
+  // If |largest_image| does not have value, the detector may have been
+  // destroyed. In this case, keep using its last candidate for comparison with
+  // the text candidate. The same for |largest_text|.
+  if ((!largest_image.has_value() || !image_has_changed) &&
+      (!largest_text.has_value() || !text_has_changed))
+    return;
+
+  if (!largest_text_ && !largest_image_)
+    return;
   if (LargestTextSize() > LargestImageSize()) {
-    // The new largest text is the largest content, so report it as the LCP.
-    OnLargestContentfulPaintUpdated(LargestContentType::kText);
-  } else if (largest_image_ && last_type_ == LargestContentType::kText) {
-    // The image is at least as large as the new text. Because the last reported
-    // content type was text, this means that the largest text is now smaller
-    // and the largest image now needs to be reported as the LCP.
-    OnLargestContentfulPaintUpdated(LargestContentType::kImage);
+    if (largest_text_->paint_time > base::TimeTicks())
+      UpdateLargestContentfulPaint(LargestContentType::kText);
+  } else {
+    if (largest_image_->paint_time > base::TimeTicks())
+      UpdateLargestContentfulPaint(LargestContentType::kImage);
   }
 }
 
-void LargestContentfulPaintCalculator::OnLargestContentfulPaintUpdated(
+void LargestContentfulPaintCalculator::UpdateLargestContentfulPaint(
     LargestContentType type) {
   DCHECK(window_performance_);
   DCHECK(type != LargestContentType::kUnknown);
   last_type_ = type;
   if (type == LargestContentType::kImage) {
+    DCHECK(largest_image_);
     const ImageResourceContent* cached_image = largest_image_->cached_image;
     Node* image_node = DOMNodeIds::NodeForId(largest_image_->node_id);
 
@@ -77,13 +115,12 @@ void LargestContentfulPaintCalculator::OnLargestContentfulPaintUpdated(
 
     const KURL& url = cached_image->Url();
     auto* document = window_performance_->GetExecutionContext();
+    bool expose_paint_time_to_api = true;
     if (!url.ProtocolIsData() &&
         (!document || !Performance::PassesTimingAllowCheck(
                           cached_image->GetResponse(),
                           *document->GetSecurityOrigin(), document))) {
-      // Reset the paint time of this image. It cannot be exposed to the
-      // webexposed API.
-      largest_image_->paint_time = base::TimeTicks();
+      expose_paint_time_to_api = false;
     }
     const String& image_url =
         url.ProtocolIsData()
@@ -95,9 +132,12 @@ void LargestContentfulPaintCalculator::OnLargestContentfulPaintUpdated(
     const AtomicString& image_id =
         image_element ? image_element->GetIdAttribute() : AtomicString();
     window_performance_->OnLargestContentfulPaintUpdated(
-        largest_image_->paint_time, largest_image_->first_size,
-        largest_image_->load_time, image_id, image_url, image_element);
+        expose_paint_time_to_api ? largest_image_->paint_time
+                                 : base::TimeTicks(),
+        largest_image_->first_size, largest_image_->load_time, image_id,
+        image_url, image_element);
   } else {
+    DCHECK(largest_text_);
     Node* text_node = DOMNodeIds::NodeForId(largest_text_->node_id);
     // |text_node| could be null and |largest_text_| should be ignored in this
     // case.

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

@@ -19,9 +19,9 @@ class CORE_EXPORT LargestContentfulPaintCalculator final
  public:
   explicit LargestContentfulPaintCalculator(WindowPerformance*);
 
-  void OnLargestImageUpdated(const ImageRecord* largest_image);
-
-  void OnLargestTextUpdated(base::WeakPtr<TextRecord> largest_text);
+  void UpdateLargestContentPaintIfNeeded(
+      base::Optional<base::WeakPtr<TextRecord>> largest_text,
+      base::Optional<const ImageRecord*> largest_image);
 
   void Trace(blink::Visitor* visitor);
 
@@ -33,7 +33,9 @@ class CORE_EXPORT LargestContentfulPaintCalculator final
     kImage,
     kText,
   };
-  void OnLargestContentfulPaintUpdated(LargestContentType type);
+  void OnLargestImageUpdated(const ImageRecord* largest_image);
+  void OnLargestTextUpdated(base::WeakPtr<TextRecord> largest_text);
+  void UpdateLargestContentfulPaint(LargestContentType type);
 
   uint64_t LargestTextSize() {
     return largest_text_ ? largest_text_->first_size : 0u;

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

@@ -18,6 +18,8 @@ namespace blink {
 
 class LargestContentfulPaintCalculatorTest : public RenderingTest {
  public:
+  using LargestContentType =
+      LargestContentfulPaintCalculator::LargestContentType;
   void SetUp() override {
     // Advance the clock so we do not assign null TimeTicks.
     simulated_clock_.Advance(base::TimeDelta::FromMilliseconds(100));
@@ -64,14 +66,8 @@ class LargestContentfulPaintCalculatorTest : public RenderingTest {
     return original_image_resource;
   }
 
-  bool IsLastReportedImage() {
-    return GetLargestContentfulPaintCalculator()->last_type_ ==
-           LargestContentfulPaintCalculator::LargestContentType::kImage;
-  }
-
-  bool IsLastReportedText() {
-    return GetLargestContentfulPaintCalculator()->last_type_ ==
-           LargestContentfulPaintCalculator::LargestContentType::kText;
+  LargestContentType LastReportedType() {
+    return GetLargestContentfulPaintCalculator()->last_type_;
   }
 
   uint64_t LargestImageSize() {
@@ -82,14 +78,41 @@ class LargestContentfulPaintCalculatorTest : public RenderingTest {
     return GetLargestContentfulPaintCalculator()->LargestTextSize();
   }
 
+  void UpdateLargestContentfulPaintCandidate() {
+    GetFrame()
+        .View()
+        ->GetPaintTimingDetector()
+        .UpdateLargestContentfulPaintCandidate();
+  }
+
+  void SimulateContentSwapPromise() {
+    mock_text_callback_manager_->InvokeSwapTimeCallback(
+        simulated_clock_.NowTicks());
+    mock_image_callback_manager_->InvokeSwapTimeCallback(
+        simulated_clock_.NowTicks());
+    // Outside the tests, this is invoked by
+    // |PaintTimingCallbackManagerImpl::ReportPaintTime|.
+    UpdateLargestContentfulPaintCandidate();
+  }
+
+  // Outside the tests, the text callback and the image callback are run
+  // together, as in |SimulateContentSwapPromise|.
   void SimulateImageSwapPromise() {
     mock_image_callback_manager_->InvokeSwapTimeCallback(
         simulated_clock_.NowTicks());
+    // Outside the tests, this is invoked by
+    // |PaintTimingCallbackManagerImpl::ReportPaintTime|.
+    UpdateLargestContentfulPaintCandidate();
   }
 
+  // Outside the tests, the text callback and the image callback are run
+  // together, as in |SimulateContentSwapPromise|.
   void SimulateTextSwapPromise() {
     mock_text_callback_manager_->InvokeSwapTimeCallback(
         simulated_clock_.NowTicks());
+    // Outside the tests, this is invoked by
+    // |PaintTimingCallbackManagerImpl::ReportPaintTime|.
+    UpdateLargestContentfulPaintCandidate();
   }
 
  private:
@@ -114,7 +137,7 @@ TEST_F(LargestContentfulPaintCalculatorTest, SingleImage) {
   UpdateAllLifecyclePhasesForTest();
   SimulateImageSwapPromise();
 
-  EXPECT_TRUE(IsLastReportedImage());
+  EXPECT_EQ(LastReportedType(), LargestContentType::kImage);
   EXPECT_EQ(LargestImageSize(), 15000u);
   EXPECT_EQ(LargestTextSize(), 0u);
 }
@@ -126,7 +149,7 @@ TEST_F(LargestContentfulPaintCalculatorTest, SingleText) {
   )HTML");
   UpdateAllLifecyclePhasesForTest();
   SimulateTextSwapPromise();
-  EXPECT_TRUE(IsLastReportedText());
+  EXPECT_EQ(LastReportedType(), LargestContentType::kText);
 }
 
 TEST_F(LargestContentfulPaintCalculatorTest, ImageLargerText) {
@@ -138,10 +161,10 @@ TEST_F(LargestContentfulPaintCalculatorTest, ImageLargerText) {
   SetImage("target", 3, 3);
   UpdateAllLifecyclePhasesForTest();
   SimulateImageSwapPromise();
-  EXPECT_TRUE(IsLastReportedImage());
+  EXPECT_EQ(LastReportedType(), LargestContentType::kImage);
   SimulateTextSwapPromise();
 
-  EXPECT_TRUE(IsLastReportedText());
+  EXPECT_EQ(LastReportedType(), LargestContentType::kText);
   EXPECT_EQ(LargestImageSize(), 9u);
   EXPECT_GT(LargestTextSize(), 9u);
 }
@@ -155,11 +178,11 @@ TEST_F(LargestContentfulPaintCalculatorTest, ImageSmallerText) {
   SetImage("target", 100, 200);
   UpdateAllLifecyclePhasesForTest();
   SimulateImageSwapPromise();
-  EXPECT_TRUE(IsLastReportedImage());
+  EXPECT_EQ(LastReportedType(), LargestContentType::kImage);
   SimulateTextSwapPromise();
 
   // Text should not be reported, since it is smaller than the image.
-  EXPECT_TRUE(IsLastReportedImage());
+  EXPECT_EQ(LastReportedType(), LargestContentType::kImage);
   EXPECT_EQ(LargestImageSize(), 20000u);
   EXPECT_GT(LargestTextSize(), 0u);
 }
@@ -172,11 +195,9 @@ TEST_F(LargestContentfulPaintCalculatorTest, TextLargerImage) {
   )HTML");
   SetImage("target", 100, 200);
   UpdateAllLifecyclePhasesForTest();
-  SimulateTextSwapPromise();
-  EXPECT_TRUE(IsLastReportedText());
-  SimulateImageSwapPromise();
+  SimulateContentSwapPromise();
 
-  EXPECT_TRUE(IsLastReportedImage());
+  EXPECT_EQ(LastReportedType(), LargestContentType::kImage);
   EXPECT_EQ(LargestImageSize(), 20000u);
   EXPECT_GT(LargestTextSize(), 0u);
 }
@@ -189,12 +210,10 @@ TEST_F(LargestContentfulPaintCalculatorTest, TextSmallerImage) {
   )HTML");
   SetImage("target", 3, 3);
   UpdateAllLifecyclePhasesForTest();
-  SimulateTextSwapPromise();
-  EXPECT_TRUE(IsLastReportedText());
-  SimulateImageSwapPromise();
+  SimulateContentSwapPromise();
 
   // Image should not be reported, since it is smaller than the text.
-  EXPECT_TRUE(IsLastReportedText());
+  EXPECT_EQ(LastReportedType(), LargestContentType::kText);
   EXPECT_EQ(LargestImageSize(), 9u);
   EXPECT_GT(LargestTextSize(), 9u);
 }
@@ -212,7 +231,7 @@ TEST_F(LargestContentfulPaintCalculatorTest, LargestImageRemoved) {
   SimulateImageSwapPromise();
   SimulateTextSwapPromise();
   // Image is larger than the text.
-  EXPECT_TRUE(IsLastReportedImage());
+  EXPECT_EQ(LastReportedType(), LargestContentType::kImage);
   EXPECT_EQ(LargestImageSize(), 20000u);
   EXPECT_GT(LargestTextSize(), 9u);
 
@@ -220,7 +239,7 @@ TEST_F(LargestContentfulPaintCalculatorTest, LargestImageRemoved) {
   UpdateAllLifecyclePhasesForTest();
   // The LCP should now be the text because it is larger than the remaining
   // image.
-  EXPECT_TRUE(IsLastReportedText());
+  EXPECT_EQ(LastReportedType(), LargestContentType::kText);
   EXPECT_EQ(LargestImageSize(), 9u);
   EXPECT_GT(LargestTextSize(), 9u);
 }
@@ -241,7 +260,7 @@ TEST_F(LargestContentfulPaintCalculatorTest, LargestTextRemoved) {
   SimulateImageSwapPromise();
   SimulateTextSwapPromise();
   // Test is larger than the image.
-  EXPECT_TRUE(IsLastReportedText());
+  EXPECT_EQ(LastReportedType(), LargestContentType::kText);
   EXPECT_EQ(LargestImageSize(), 50u);
   EXPECT_GT(LargestTextSize(), 50u);
 
@@ -249,7 +268,7 @@ TEST_F(LargestContentfulPaintCalculatorTest, LargestTextRemoved) {
   UpdateAllLifecyclePhasesForTest();
   // The LCP should now be the image because it is larger than the remaining
   // text.
-  EXPECT_TRUE(IsLastReportedImage());
+  EXPECT_EQ(LastReportedType(), LargestContentType::kImage);
   EXPECT_EQ(LargestImageSize(), 50u);
   EXPECT_LT(LargestTextSize(), 50u);
 }

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

@@ -177,6 +177,7 @@ void PaintTimingDetector::StopRecordingIfNeeded() {
   }
   if (image_paint_timing_detector_)
     image_paint_timing_detector_->StopRecordEntries();
+  largest_contentful_paint_calculator_ = nullptr;
 }
 
 void PaintTimingDetector::NotifyInputEvent(WebInputEvent::Type type) {
@@ -306,6 +307,33 @@ FloatRect PaintTimingDetector::CalculateVisualRect(
   return float_rect;
 }
 
+void PaintTimingDetector::UpdateLargestContentfulPaintCandidate() {
+  auto* lcp_calculator = GetLargestContentfulPaintCalculator();
+  if (!lcp_calculator)
+    return;
+
+  // Optional, WeakPtr, Record have different roles:
+  // * !Optional means |UpdateCandidate() is not reachable, e.g., user input
+  // has been given to stop LCP. In this case, we still use the last recorded
+  // result.
+  // * !Weak means there is no candidate, e.g., no content show up on the page.
+  // * Record.paint_time == 0 means there is an image but the image is still
+  // loading. The perf API should wait until the paint-time is available.
+  base::Optional<base::WeakPtr<TextRecord>> largest_text_record;
+  base::Optional<const ImageRecord*> largest_image_record;
+  if (auto* text_timing_detector = GetTextPaintTimingDetector()) {
+    if (text_timing_detector->IsRecordingLargestTextPaint()) {
+      largest_text_record.emplace(text_timing_detector->UpdateCandidate());
+    }
+  }
+  if (auto* image_timing_detector = GetImagePaintTimingDetector()) {
+    largest_image_record.emplace(image_timing_detector->UpdateCandidate());
+  }
+
+  lcp_calculator->UpdateLargestContentPaintIfNeeded(largest_text_record,
+                                                    largest_image_record);
+}
+
 ScopedPaintTimingDetectorBlockPaintHook*
     ScopedPaintTimingDetectorBlockPaintHook::top_ = nullptr;
 
@@ -381,6 +409,7 @@ void PaintTimingCallbackManagerImpl::ReportPaintTime(
     std::move(frame_callbacks->front()).Run(paint_time);
     frame_callbacks->pop();
   }
+  frame_view_->GetPaintTimingDetector().UpdateLargestContentfulPaintCandidate();
 }
 
 void PaintTimingCallbackManagerImpl::Trace(Visitor* visitor) {

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

-// Copyright 2019 The Chromium Authors. All rights reserved.
+// Copyright 2018 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -169,6 +169,9 @@ class CORE_EXPORT PaintTimingDetector
   uint64_t LargestImagePaintSize() const { return largest_image_paint_size_; }
   base::TimeTicks LargestTextPaint() const { return largest_text_paint_time_; }
   uint64_t LargestTextPaintSize() const { return largest_text_paint_size_; }
+
+  void UpdateLargestContentfulPaintCandidate();
+
   base::Optional<PaintTimingVisualizer>& Visualizer() { return visualizer_; }
   void Trace(Visitor* visitor);
 

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

@@ -76,7 +76,7 @@ void LargestTextPaintManager::ReportNoCandidateToTrace() {
                ToTraceValue(&frame_view_->GetFrame()));
 }
 
-void LargestTextPaintManager::UpdateCandidate() {
+base::WeakPtr<TextRecord> LargestTextPaintManager::UpdateCandidate() {
   base::WeakPtr<TextRecord> largest_text_record = FindLargestPaintCandidate();
   const base::TimeTicks time =
       largest_text_record ? largest_text_record->paint_time : base::TimeTicks();
@@ -85,27 +85,20 @@ void LargestTextPaintManager::UpdateCandidate() {
   DCHECK(paint_timing_detector_);
   bool changed =
       paint_timing_detector_->NotifyIfChangedLargestTextPaint(time, size);
-  if (!changed)
-    return;
-
-  if (!time.is_null()) {
-    if (auto* lcp_calculator =
-            paint_timing_detector_->GetLargestContentfulPaintCalculator())
-      lcp_calculator->OnLargestTextUpdated(largest_text_record);
-    ReportCandidateToTrace(*largest_text_record);
-  } else {
-    if (auto* lcp_calculator =
-            paint_timing_detector_->GetLargestContentfulPaintCalculator())
-      lcp_calculator->OnLargestTextUpdated(nullptr);
-    ReportNoCandidateToTrace();
+  if (changed) {
+    if (!time.is_null())
+      ReportCandidateToTrace(*largest_text_record);
+    else
+      ReportNoCandidateToTrace();
   }
+  return largest_text_record;
 }
 
 void TextPaintTimingDetector::OnPaintFinished() {
   if (need_update_timing_at_frame_end_) {
     need_update_timing_at_frame_end_ = false;
-    if (records_manager_.GetLargestTextPaintManager())
-      records_manager_.GetLargestTextPaintManager()->UpdateCandidate();
+    frame_view_->GetPaintTimingDetector()
+        .UpdateLargestContentfulPaintCandidate();
   }
   if (records_manager_.NeedMeausuringPaintTime()) {
     if (!awaiting_swap_promise_) {
@@ -150,8 +143,8 @@ void TextPaintTimingDetector::ReportSwapTime(base::TimeTicks timestamp) {
     }
   }
   records_manager_.AssignPaintTimeToQueuedRecords(timestamp);
-  if (records_manager_.GetLargestTextPaintManager())
-    records_manager_.GetLargestTextPaintManager()->UpdateCandidate();
+  if (IsRecordingLargestTextPaint())
+    UpdateCandidate();
   awaiting_swap_promise_ = false;
 }
 

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

@@ -69,7 +69,7 @@ class CORE_EXPORT LargestTextPaintManager {
 
   void ReportCandidateToTrace(const TextRecord&);
   void ReportNoCandidateToTrace();
-  void UpdateCandidate();
+  base::WeakPtr<TextRecord> UpdateCandidate();
   void PopulateTraceValue(TracedValue&, const TextRecord& first_text_paint);
   inline void SetCachedResultInvalidated(bool value) {
     is_result_invalidated_ = value;
@@ -145,8 +145,9 @@ class CORE_EXPORT TextRecordsManager {
     text_element_timing_ = text_element_timing;
   }
 
-  inline base::Optional<LargestTextPaintManager>& GetLargestTextPaintManager() {
-    return ltp_manager_;
+  inline base::WeakPtr<TextRecord> UpdateCandidate() {
+    DCHECK(ltp_manager_);
+    return ltp_manager_->UpdateCandidate();
   }
 
   inline bool IsRecordingLargestTextPaint() const {
@@ -212,6 +213,12 @@ class CORE_EXPORT TextPaintTimingDetector final
   void ResetCallbackManager(PaintTimingCallbackManager* manager) {
     callback_manager_ = manager;
   }
+  inline bool IsRecordingLargestTextPaint() const {
+    return records_manager_.IsRecordingLargestTextPaint();
+  }
+  inline base::WeakPtr<TextRecord> UpdateCandidate() {
+    return records_manager_.UpdateCandidate();
+  }
   void ReportSwapTime(base::TimeTicks timestamp);
   void Trace(blink::Visitor*);
 

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

@@ -77,8 +77,7 @@ class TextPaintTimingDetectorTest
   }
 
   base::Optional<LargestTextPaintManager>& GetLargestTextPaintManager() {
-    return GetTextPaintTimingDetector()
-        ->records_manager_.GetLargestTextPaintManager();
+    return GetTextPaintTimingDetector()->records_manager_.ltp_manager_;
   }
 
   wtf_size_t CountVisibleTexts() {
@@ -89,9 +88,7 @@ class TextPaintTimingDetectorTest
 
   wtf_size_t CountRankingSetSize() {
     DCHECK(GetTextPaintTimingDetector());
-    return GetTextPaintTimingDetector()
-        ->records_manager_.GetLargestTextPaintManager()
-        ->size_ordered_set_.size();
+    return GetLargestTextPaintManager()->size_ordered_set_.size();
   }
 
   wtf_size_t CountInvisibleTexts() {
@@ -126,6 +123,9 @@ class TextPaintTimingDetectorTest
   void InvokeSwapTimeCallback(
       MockPaintTimingCallbackManager* callback_manager) {
     callback_manager->InvokeSwapTimeCallback(test_task_runner_->NowTicks());
+    // Outside the tests, this is invoked by
+    // |PaintTimingCallbackManagerImpl::ReportPaintTime|.
+    GetLargestTextPaintManager()->UpdateCandidate();
   }
 
   base::TimeTicks LargestPaintStoredResult() {
@@ -192,19 +192,14 @@ class TextPaintTimingDetectorTest
   }
 
   base::WeakPtr<TextRecord> TextRecordOfLargestTextPaint() {
-    return GetFrameView()
-        .GetPaintTimingDetector()
-        .GetTextPaintTimingDetector()
-        ->records_manager_.GetLargestTextPaintManager()
-        ->FindLargestPaintCandidate();
+    return GetLargestTextPaintManager()->FindLargestPaintCandidate();
   }
 
   base::WeakPtr<TextRecord> ChildFrameTextRecordOfLargestTextPaint() {
     return GetChildFrameView()
         .GetPaintTimingDetector()
         .GetTextPaintTimingDetector()
-        ->records_manager_.GetLargestTextPaintManager()
-        ->FindLargestPaintCandidate();
+        ->records_manager_.ltp_manager_->FindLargestPaintCandidate();
   }
 
   void SetFontSize(Element* font_element, uint16_t font_size) {