[FCP++] Update candidate when node is removed

The text side of FCP++ should update the candidate when any node is removed in
the same frame, in the same style as the image side.

Before, we didn't update candidate on node removal unless the node is the last
detached node. We shouldn't special case this. Instead, everytime a node is
removed we should update the candidate.

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

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

@@ -103,7 +103,7 @@ void ImagePaintTimingDetector::OnLargestImagePaintDetected(
       largest_image_record->paint_time, "data", std::move(value));
 }
 
-void ImagePaintTimingDetector::Analyze() {
+void ImagePaintTimingDetector::UpdateCandidate() {
   ImageRecord* largest_image_record =
       records_manager_.FindLargestPaintCandidate();
   // These conditions represents the following scenarios:
@@ -132,7 +132,7 @@ void ImagePaintTimingDetector::OnPaintFinished() {
   frame_index_++;
   if (need_update_timing_at_frame_end_) {
     need_update_timing_at_frame_end_ = false;
-    Analyze();
+    UpdateCandidate();
   }
   if (!records_manager_.NeedMeausuringPaintTime())
     return;
@@ -191,7 +191,7 @@ void ImagePaintTimingDetector::ReportSwapTime(
   DCHECK(ThreadState::Current()->IsMainThread());
   records_manager_.AssignPaintTimeToRegisteredQueuedNodes(
       timestamp, last_queued_frame_index);
-  Analyze();
+  UpdateCandidate();
 }
 
 void ImageRecordsManager::AssignPaintTimeToRegisteredQueuedNodes(

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

@@ -208,7 +208,7 @@ class CORE_EXPORT ImagePaintTimingDetector final
   void Deactivate();
   void HandleTooManyNodes();
 
-  void Analyze();
+  void UpdateCandidate();
 
   base::RepeatingCallback<void(WebWidgetClient::ReportTimeCallback)>
       notify_swap_time_override_for_testing_;

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

@@ -108,8 +108,10 @@ class ImagePaintTimingDetectorTest
         .records_manager_.size_ordered_set_.size();
   }
 
-  void Analyze() {
-    return GetPaintTimingDetector().GetImagePaintTimingDetector().Analyze();
+  void UpdateCandidate() {
+    return GetPaintTimingDetector()
+        .GetImagePaintTimingDetector()
+        .UpdateCandidate();
   }
 
   TimeTicks LargestPaintStoredResult() {
@@ -596,7 +598,7 @@ TEST_F(ImagePaintTimingDetectorTest, NullTimeNoCrash) {
   )HTML");
   SetImageAndPaint("target", 5, 5);
   UpdateAllLifecyclePhasesForTest();
-  Analyze();
+  UpdateCandidate();
 }
 
 TEST_F(ImagePaintTimingDetectorTest, Iframe) {

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

@@ -73,21 +73,28 @@ void TextPaintTimingDetector::OnLargestTextDetected(
 }
 
 void TextPaintTimingDetector::TimerFired(TimerBase* time) {
-  // Wrap Analyze method in TimerFired so that we can drop |time| for Analyze
-  // in testing.
-  Analyze();
+  // Wrap |UpdateCandidate| method in TimerFired so that we can drop |time| for
+  // |UpdateCandidate| in testing.
+  UpdateCandidate();
 }
 
-void TextPaintTimingDetector::Analyze() {
+void TextPaintTimingDetector::UpdateCandidate() {
   TextRecord* candidate = records_manager_.FindLargestPaintCandidate();
-  DCHECK(!candidate || !candidate->paint_time.is_null());
-  if (candidate && candidate->paint_time != largest_text_paint_) {
+  if (!candidate) {
+    largest_text_paint_ = base::TimeTicks();
+    largest_text_paint_size_ = 0;
+    frame_view_->GetPaintTimingDetector().DidChangePerformanceTiming();
+  } else if (candidate->paint_time != largest_text_paint_) {
     OnLargestTextDetected(*candidate);
     frame_view_->GetPaintTimingDetector().DidChangePerformanceTiming();
   }
 }
 
 void TextPaintTimingDetector::OnPaintFinished() {
+  if (need_update_timing_at_frame_end_) {
+    need_update_timing_at_frame_end_ = false;
+    UpdateCandidate();
+  }
   if (records_manager_.NeedMeausuringPaintTime()) {
     // Start repeating timer only once at the first text paint.
     if (!timer_.IsActive())
@@ -106,11 +113,7 @@ void TextPaintTimingDetector::NotifyNodeRemoved(DOMNodeId node_id) {
   if (!records_manager_.IsKnownVisibleNode(node_id))
     return;
   records_manager_.SetNodeDetachedIfNeeded(node_id);
-  if (records_manager_.AreAllVisibleNodesDetached() &&
-      largest_text_paint_ != base::TimeTicks()) {
-    largest_text_paint_ = base::TimeTicks();
-    frame_view_->GetPaintTimingDetector().DidChangePerformanceTiming();
-  }
+  need_update_timing_at_frame_end_ = true;
 }
 
 void TextPaintTimingDetector::RegisterNotifySwapTime(

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

@@ -130,7 +130,7 @@ class CORE_EXPORT TextPaintTimingDetector final
                           const TextRecord& first_text_paint,
                           unsigned candidate_index) const;
   void TimerFired(TimerBase*);
-  void Analyze();
+  void UpdateCandidate();
 
   void ReportSwapTime(WebWidgetClient::SwapResult result,
                       base::TimeTicks timestamp);
@@ -145,6 +145,7 @@ class CORE_EXPORT TextPaintTimingDetector final
   bool is_recording_ = true;
 
   bool has_records_changed_ = true;
+  bool need_update_timing_at_frame_end_ = false;
 
   base::TimeTicks largest_text_paint_;
   uint64_t largest_text_paint_size_ = 0;

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

@@ -96,8 +96,8 @@ class TextPaintTimingDetectorTest
                         CurrentTimeTicks());
   }
 
-  void SimulateAnalyze() {
-    GetPaintTimingDetector().GetTextPaintTimingDetector().Analyze();
+  void UpdateCandidate() {
+    GetPaintTimingDetector().GetTextPaintTimingDetector().UpdateCandidate();
   }
 
   Element* AppendFontElementToBody(String content) {
@@ -204,7 +204,7 @@ TEST_F(TextPaintTimingDetectorTest, UpdateResultWhenCandidateChanged) {
     <div>small text</div>
   )HTML");
   UpdateAllLifecyclePhasesAndSimulateSwapTime();
-  SimulateAnalyze();
+  UpdateCandidate();
   TimeTicks time2 = CurrentTimeTicks();
   TimeTicks first_largest = LargestPaintStoredResult();
   EXPECT_GE(first_largest, time1);
@@ -213,7 +213,7 @@ TEST_F(TextPaintTimingDetectorTest, UpdateResultWhenCandidateChanged) {
   Text* larger_text = GetDocument().createTextNode("a long-long-long text");
   GetDocument().body()->AppendChild(larger_text);
   UpdateAllLifecyclePhasesAndSimulateSwapTime();
-  SimulateAnalyze();
+  UpdateCandidate();
   TimeTicks time3 = CurrentTimeTicks();
   TimeTicks second_largest = LargestPaintStoredResult();
   EXPECT_GE(second_largest, time2);
@@ -307,13 +307,13 @@ TEST_F(TextPaintTimingDetectorTest, LargestTextPaint_ReportLastNullCandidate) {
   )HTML");
   Element* text = AppendDivElementToBody("text to remove");
   UpdateAllLifecyclePhasesAndSimulateSwapTime();
-  SimulateAnalyze();
+  UpdateCandidate();
   EXPECT_EQ(TextRecordOfLargestTextPaint()->node_id, NodeIdOfText(text));
   EXPECT_NE(LargestPaintStoredResult(), base::TimeTicks());
 
   RemoveElement(text);
   UpdateAllLifecyclePhasesAndSimulateSwapTime();
-  SimulateAnalyze();
+  UpdateCandidate();
   EXPECT_FALSE(TextRecordOfLargestTextPaint());
   EXPECT_EQ(LargestPaintStoredResult(), base::TimeTicks());
 }