[LCP] Combine text and image callbacks

Currently, ImagePaintTimingDetector and TextPaintTimingDetector each
has their own paint-time callback. Two callbacks are unnecessary since
if they are registered from the same (animated) frame, they would get
the same paint-time. Therefore, they should be combined into one
callback.

Combining the callbacks also creates a chance for LCPCalculator to
hook to "either text records or image records have been changed",
i.e., the end of the combined callback. This hook would enable us to
remove the intermediate state between two callbacks, and thus fixed
the out-of-sync issue in crbug.com/982307.

Bug: 982307

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

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

@@ -60,10 +60,10 @@ static bool LargeImageFirst(const base::WeakPtr<ImageRecord>& a,
   return a->insertion_index < b->insertion_index;
 }
 
-ImagePaintTimingDetector::ImagePaintTimingDetector(LocalFrameView* frame_view)
-    : frame_view_(frame_view),
-      callback_manager_(
-          MakeGarbageCollected<PaintTimingCallbackManagerImpl>()) {}
+ImagePaintTimingDetector::ImagePaintTimingDetector(
+    LocalFrameView* frame_view,
+    PaintTimingCallbackManager* callback_manager)
+    : frame_view_(frame_view), callback_manager_(callback_manager) {}
 
 void ImagePaintTimingDetector::PopulateTraceValue(
     TracedValue& value,
@@ -171,22 +171,15 @@ void ImagePaintTimingDetector::NotifyImageRemoved(
 }
 
 void ImagePaintTimingDetector::RegisterNotifySwapTime() {
-  auto callback = CrossThreadBindOnce(&ImagePaintTimingDetector::ReportSwapTime,
-                                      WrapCrossThreadWeakPersistent(this),
-                                      last_registered_frame_index_);
-  // ReportSwapTime on layerTreeView will queue a swap-promise, the callback is
-  // called when the swap for current render frame completes or fails to happen.
-  LocalFrame& frame = frame_view_->GetFrame();
-  if (!frame.GetPage())
-    return;
-
-  callback_manager_->RegisterCallback(frame, std::move(callback));
+  auto callback = WTF::Bind(&ImagePaintTimingDetector::ReportSwapTime,
+                            WrapCrossThreadWeakPersistent(this),
+                            last_registered_frame_index_);
+  callback_manager_->RegisterCallback(std::move(callback));
   num_pending_swap_callbacks_++;
 }
 
 void ImagePaintTimingDetector::ReportSwapTime(
     unsigned last_queued_frame_index,
-    WebWidgetClient::SwapResult result,
     base::TimeTicks timestamp) {
   if (!is_recording_)
     return;

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

@@ -203,7 +203,7 @@ class CORE_EXPORT ImagePaintTimingDetector final
   friend class ImagePaintTimingDetectorTest;
 
  public:
-  ImagePaintTimingDetector(LocalFrameView*);
+  ImagePaintTimingDetector(LocalFrameView*, PaintTimingCallbackManager*);
   void RecordImage(const LayoutObject&,
                    const IntSize& intrinsic_size,
                    const ImageResourceContent&,
@@ -222,6 +222,10 @@ class CORE_EXPORT ImagePaintTimingDetector final
   inline bool FinishedReportingImages() const {
     return !is_recording_ && num_pending_swap_callbacks_ == 0;
   }
+  void ResetCallbackManager(PaintTimingCallbackManager* manager) {
+    callback_manager_ = manager;
+  }
+  void ReportSwapTime(unsigned last_queued_frame_index, base::TimeTicks);
 
   void Trace(blink::Visitor*);
 
@@ -231,13 +235,6 @@ class CORE_EXPORT ImagePaintTimingDetector final
   ImageRecord* FindLargestPaintCandidate() const;
 
   void PopulateTraceValue(TracedValue&, const ImageRecord& first_image_paint);
-  // This is provided for unit test to force invoking swap promise callback.
-  void ReportSwapTime(unsigned last_queued_frame_index,
-                      WebWidgetClient::SwapResult,
-                      base::TimeTicks);
-  void ResetCallbackManagerForTesting(PaintTimingCallbackManager* manager) {
-    callback_manager_ = manager;
-  }
   void RegisterNotifySwapTime();
   void ReportCandidateToTrace(ImageRecord&);
   void ReportNoCandidateToTrace();

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

@@ -176,7 +176,7 @@ class ImagePaintTimingDetectorTest
         MakeGarbageCollected<MockPaintTimingCallbackManager>();
     GetPaintTimingDetector()
         .GetImagePaintTimingDetector()
-        ->ResetCallbackManagerForTesting(mock_callback_manager_);
+        ->ResetCallbackManager(mock_callback_manager_);
     UpdateAllLifecyclePhases();
   }
 
@@ -188,7 +188,7 @@ class ImagePaintTimingDetectorTest
         MakeGarbageCollected<MockPaintTimingCallbackManager>();
     GetChildPaintTimingDetector()
         .GetImagePaintTimingDetector()
-        ->ResetCallbackManagerForTesting(child_mock_callback_manager_);
+        ->ResetCallbackManager(child_mock_callback_manager_);
     UpdateAllLifecyclePhases();
   }
 
@@ -263,7 +263,7 @@ class ImagePaintTimingDetectorTest
     return original_image_resource;
   }
 
-  CallbackQueue callback_queue_;
+  PaintTimingCallbackManager::CallbackQueue callback_queue_;
   Persistent<MockPaintTimingCallbackManager> mock_callback_manager_;
   Persistent<MockPaintTimingCallbackManager> child_mock_callback_manager_;
 };

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

@@ -26,11 +26,11 @@ class LargestContentfulPaintCalculatorTest : public RenderingTest {
 
     mock_text_callback_manager_ =
         MakeGarbageCollected<MockPaintTimingCallbackManager>();
-    GetTextPaintTimingDetector()->ResetCallbackManagerForTesting(
+    GetTextPaintTimingDetector()->ResetCallbackManager(
         mock_text_callback_manager_);
     mock_image_callback_manager_ =
         MakeGarbageCollected<MockPaintTimingCallbackManager>();
-    GetImagePaintTimingDetector()->ResetCallbackManagerForTesting(
+    GetImagePaintTimingDetector()->ResetCallbackManager(
         mock_image_callback_manager_);
   }
 

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

@@ -28,6 +28,7 @@
 #include "third_party/blink/renderer/platform/graphics/paint/property_tree_state.h"
 #include "third_party/blink/renderer/platform/graphics/paint/scoped_paint_chunk_properties.h"
 #include "third_party/blink/renderer/platform/wtf/cross_thread_functional.h"
+#include "third_party/blink/renderer/platform/wtf/functional.h"
 
 namespace blink {
 
@@ -59,9 +60,18 @@ bool IsBackgroundImageContentful(const LayoutObject& object,
 PaintTimingDetector::PaintTimingDetector(LocalFrameView* frame_view)
     : frame_view_(frame_view),
       text_paint_timing_detector_(
-          MakeGarbageCollected<TextPaintTimingDetector>(frame_view, this)),
+          MakeGarbageCollected<TextPaintTimingDetector>(frame_view,
+                                                        this,
+                                                        nullptr /*set later*/)),
       image_paint_timing_detector_(
-          MakeGarbageCollected<ImagePaintTimingDetector>(frame_view)) {}
+          MakeGarbageCollected<ImagePaintTimingDetector>(
+              frame_view,
+              nullptr /*set later*/)),
+      callback_manager_(
+          MakeGarbageCollected<PaintTimingCallbackManagerImpl>(frame_view)) {
+  text_paint_timing_detector_->ResetCallbackManager(callback_manager_.Get());
+  image_paint_timing_detector_->ResetCallbackManager(callback_manager_.Get());
+}
 
 void PaintTimingDetector::NotifyPaintFinished() {
   if (text_paint_timing_detector_) {
@@ -76,6 +86,8 @@ void PaintTimingDetector::NotifyPaintFinished() {
     if (image_paint_timing_detector_->FinishedReportingImages())
       image_paint_timing_detector_ = nullptr;
   }
+  if (callback_manager_->CountCallbacks() > 0)
+    callback_manager_->RegisterPaintTimeCallbackForCombinedCallbacks();
 }
 
 // static
@@ -328,12 +340,42 @@ void PaintTimingDetector::Trace(Visitor* visitor) {
   visitor->Trace(image_paint_timing_detector_);
   visitor->Trace(frame_view_);
   visitor->Trace(largest_contentful_paint_calculator_);
+  visitor->Trace(callback_manager_);
+}
+
+void PaintTimingCallbackManagerImpl::
+    RegisterPaintTimeCallbackForCombinedCallbacks() {
+  DCHECK(!frame_callbacks_->empty());
+  LocalFrame& frame = frame_view_->GetFrame();
+  if (!frame.GetPage())
+    return;
+
+  auto combined_callback = CrossThreadBindOnce(
+      &PaintTimingCallbackManagerImpl::ReportPaintTime,
+      WrapCrossThreadWeakPersistent(this), std::move(frame_callbacks_));
+  frame_callbacks_ =
+      std::make_unique<PaintTimingCallbackManager::CallbackQueue>();
+
+  // |ReportPaintTime| on |layerTreeView| will queue a swap-promise, the
+  // callback is called when the swap for current render frame completes or
+  // fails to happen.
+  frame.GetPage()->GetChromeClient().NotifySwapTime(
+      frame, std::move(combined_callback));
 }
 
-void PaintTimingCallbackManagerImpl::RegisterCallback(
-    LocalFrame& frame,
-    ReportTimeCallback callback) {
-  frame.GetPage()->GetChromeClient().NotifySwapTime(frame, std::move(callback));
+void PaintTimingCallbackManagerImpl::ReportPaintTime(
+    std::unique_ptr<PaintTimingCallbackManager::CallbackQueue> frame_callbacks,
+    WebWidgetClient::SwapResult result,
+    base::TimeTicks paint_time) {
+  while (!frame_callbacks->empty()) {
+    std::move(frame_callbacks->front()).Run(paint_time);
+    frame_callbacks->pop();
+  }
+}
+
+void PaintTimingCallbackManagerImpl::Trace(Visitor* visitor) {
+  visitor->Trace(frame_view_);
+  PaintTimingCallbackManager::Trace(visitor);
 }
 
 }  // namespace blink

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

@@ -25,9 +25,6 @@ class PropertyTreeState;
 class TextPaintTimingDetector;
 struct WebFloatRect;
 
-using ReportTimeCallback =
-    WTF::CrossThreadOnceFunction<void(WebWidgetClient::SwapResult,
-                                      base::TimeTicks)>;
 // |PaintTimingCallbackManager| is an interface between
 // |ImagePaintTimingDetector|/|TextPaintTimingDetector| and |ChromeClient|.
 // As |ChromeClient| is shared among the paint-timing-detecters, it
@@ -39,21 +36,71 @@ using ReportTimeCallback =
 // without having to popping the |TextPaintTimingDetector|'s.
 class PaintTimingCallbackManager : public GarbageCollectedMixin {
  public:
-  virtual void RegisterCallback(LocalFrame&, ReportTimeCallback) = 0;
-};
+  using LocalThreadCallback = base::OnceCallback<void(base::TimeTicks)>;
+  using CallbackQueue = std::queue<LocalThreadCallback>;
 
-using ReportTimeCallback =
-    WTF::CrossThreadOnceFunction<void(WebWidgetClient::SwapResult,
-                                      base::TimeTicks)>;
+  virtual void RegisterCallback(
+      PaintTimingCallbackManager::LocalThreadCallback) = 0;
+};
 
-class PaintTimingCallbackManagerImpl
-    : public GarbageCollected<PaintTimingCallbackManagerImpl>,
+// This class is responsible for managing the swap-time callback for Largest
+// Image Paint and Largest Text Paint. In frames where both text and image are
+// painted, Largest Image Paint and Largest Text Paint need to assign the same
+// paint-time for their records. In this case, |PaintTimeCallbackManager|
+// requests a swap-time callback and share the swap-time with LIP and LTP.
+// Otherwise LIP and LTP would have to request their own swap-time callbacks.
+// An extra benefit of this design is that |LargestContentfulPaintCalculator|
+// can thus hook to the end of the LIP and LTP's record assignments.
+//
+// |GarbageCollectedFinalized| inheritance is required by the swap-time callback
+// registration.
+class PaintTimingCallbackManagerImpl final
+    : public GarbageCollectedFinalized<PaintTimingCallbackManagerImpl>,
       public PaintTimingCallbackManager {
   USING_GARBAGE_COLLECTED_MIXIN(PaintTimingCallbackManagerImpl);
 
  public:
-  void RegisterCallback(LocalFrame&, ReportTimeCallback) override;
-  void Trace(Visitor* visitor) override {}
+  PaintTimingCallbackManagerImpl(LocalFrameView* frame_view)
+      : frame_view_(frame_view),
+        frame_callbacks_(
+            std::make_unique<std::queue<
+                PaintTimingCallbackManager::LocalThreadCallback>>()) {}
+  ~PaintTimingCallbackManagerImpl() { frame_callbacks_.reset(); }
+
+  // Instead of registering the callback right away, this impl of the interface
+  // combine the callback into |frame_callbacks_| before registering a separate
+  // swap-time callback for the combined callbacks. When the swap-time callback
+  // is invoked, the swap-time is then assigned to each callback of
+  // |frame_callbacks_|.
+  void RegisterCallback(
+      PaintTimingCallbackManager::LocalThreadCallback callback) override {
+    frame_callbacks_->push(std::move(callback));
+  }
+
+  void RegisterPaintTimeCallbackForCombinedCallbacks();
+
+  inline size_t CountCallbacks() { return frame_callbacks_->size(); }
+
+  void ReportPaintTime(
+      std::unique_ptr<std::queue<
+          PaintTimingCallbackManager::LocalThreadCallback>> frame_callbacks,
+      WebWidgetClient::SwapResult,
+      base::TimeTicks paint_time);
+
+  void Trace(Visitor* visitor) override;
+
+ private:
+  Member<LocalFrameView> frame_view_;
+  // |frame_callbacks_| stores the callbacks of |TextPaintTimingDetector| and
+  // |ImagePaintTimingDetector| in an (animated) frame. It is passed as an
+  // argument of a swap-time callback which once is invoked, invokes every
+  // callback in |frame_callbacks_|. This hierarchical callback design is to
+  // reduce the need of calling ChromeClient to register swap-time callbacks for
+  // both detectos.
+  // Although |frame_callbacks_| intends to store callbacks
+  // of a frame, it occasionally has to do that for more than one frame, when it
+  // fails to register a swap-time callback.
+  std::unique_ptr<PaintTimingCallbackManager::CallbackQueue> frame_callbacks_;
 };
 
 // PaintTimingDetector contains some of paint metric detectors,
@@ -137,6 +184,8 @@ class CORE_EXPORT PaintTimingDetector
 
   Member<LargestContentfulPaintCalculator> largest_contentful_paint_calculator_;
 
+  Member<PaintTimingCallbackManagerImpl> callback_manager_;
+
   // Largest image information.
   base::TimeTicks largest_image_paint_time_;
   uint64_t largest_image_paint_size_ = 0;

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

@@ -4,7 +4,6 @@
 #include "third_party/blink/renderer/core/paint/paint_timing_detector.h"
 namespace blink {
 
-using CallbackQueue = std::queue<WebWidgetClient::ReportTimeCallback>;
 // |MockPaintTimingCallbackManager| is used to mock
 // |ChromeClient::NotifySwapTime()|'s swap-time queueing and invoking for
 // unit-tests. Find more details in |PaintTimingCallbackManager|.
@@ -15,14 +14,13 @@ class MockPaintTimingCallbackManager final
 
  public:
   ~MockPaintTimingCallbackManager() {}
-  void RegisterCallback(LocalFrame& frame,
-                        ReportTimeCallback callback) override {
-    callback_queue_.push(ConvertToBaseOnceCallback(std::move(callback)));
+  void RegisterCallback(
+      PaintTimingCallbackManager::LocalThreadCallback callback) override {
+    callback_queue_.push(std::move(callback));
   }
   void InvokeSwapTimeCallback(base::TimeTicks swap_time) {
     DCHECK_GT(callback_queue_.size(), 0UL);
-    std::move(callback_queue_.front())
-        .Run(WebWidgetClient::SwapResult::kDidSwap, swap_time);
+    std::move(callback_queue_.front()).Run(swap_time);
     callback_queue_.pop();
   }
 
@@ -31,7 +29,7 @@ class MockPaintTimingCallbackManager final
   void Trace(Visitor* visitor) override {}
 
  private:
-  CallbackQueue callback_queue_;
+  PaintTimingCallbackManager::CallbackQueue callback_queue_;
 };
 }  // namespace blink
 

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

@@ -33,9 +33,10 @@ bool LargeTextFirst(const base::WeakPtr<TextRecord>& a,
 
 TextPaintTimingDetector::TextPaintTimingDetector(
     LocalFrameView* frame_view,
-    PaintTimingDetector* paint_timing_detector)
+    PaintTimingDetector* paint_timing_detector,
+    PaintTimingCallbackManager* callback_manager)
     : records_manager_(frame_view, paint_timing_detector),
-      callback_manager_(MakeGarbageCollected<PaintTimingCallbackManagerImpl>()),
+      callback_manager_(callback_manager),
       frame_view_(frame_view) {}
 
 void LargestTextPaintManager::PopulateTraceValue(
@@ -110,9 +111,8 @@ void TextPaintTimingDetector::OnPaintFinished() {
     if (!awaiting_swap_promise_) {
       // |WrapCrossThreadWeakPersistent| guarantees that when |this| is killed,
       // the callback function will not be invoked.
-      RegisterNotifySwapTime(
-          CrossThreadBindOnce(&TextPaintTimingDetector::ReportSwapTime,
-                              WrapCrossThreadWeakPersistent(this)));
+      RegisterNotifySwapTime(WTF::Bind(&TextPaintTimingDetector::ReportSwapTime,
+                                       WrapCrossThreadWeakPersistent(this)));
     }
   }
 }
@@ -131,18 +131,12 @@ void TextPaintTimingDetector::LayoutObjectWillBeDestroyed(
 }
 
 void TextPaintTimingDetector::RegisterNotifySwapTime(
-    ReportTimeCallback callback) {
-  // ReportSwapTime on layerTreeView will queue a swap-promise, the callback is
-  // called when the swap for current render frame completes or fails to happen.
-  LocalFrame& frame = frame_view_->GetFrame();
-  if (!frame.GetPage())
-    return;
-  callback_manager_->RegisterCallback(frame, std::move(callback));
+    PaintTimingCallbackManager::LocalThreadCallback callback) {
+  callback_manager_->RegisterCallback(std::move(callback));
   awaiting_swap_promise_ = true;
 }
 
-void TextPaintTimingDetector::ReportSwapTime(WebWidgetClient::SwapResult result,
-                                             base::TimeTicks timestamp) {
+void TextPaintTimingDetector::ReportSwapTime(base::TimeTicks timestamp) {
   if (!is_recording_)
     return;
   if (!records_manager_.HasTextElementTiming()) {

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

@@ -193,13 +193,12 @@ class CORE_EXPORT TextRecordsManager {
 // https://docs.google.com/document/d/1DRVd4a2VU8-yyWftgOparZF-sf16daf0vfbsHuz2rws/edit#heading=h.lvno2v283uls
 class CORE_EXPORT TextPaintTimingDetector final
     : public GarbageCollectedFinalized<TextPaintTimingDetector> {
-  using ReportTimeCallback =
-      WTF::CrossThreadOnceFunction<void(WebWidgetClient::SwapResult,
-                                        base::TimeTicks)>;
   friend class TextPaintTimingDetectorTest;
 
  public:
-  explicit TextPaintTimingDetector(LocalFrameView*, PaintTimingDetector*);
+  explicit TextPaintTimingDetector(LocalFrameView*,
+                                   PaintTimingDetector*,
+                                   PaintTimingCallbackManager*);
   bool ShouldWalkObject(const LayoutBoxModelObject&) const;
   void RecordAggregatedText(const LayoutBoxModelObject& aggregator,
                             const IntRect& aggregated_visual_rect,
@@ -210,17 +209,17 @@ class CORE_EXPORT TextPaintTimingDetector final
   void StopRecordingLargestTextPaint();
   bool IsRecording() const { return is_recording_; }
   inline bool FinishedReportingText() const { return !is_recording_; }
+  void ResetCallbackManager(PaintTimingCallbackManager* manager) {
+    callback_manager_ = manager;
+  }
+  void ReportSwapTime(base::TimeTicks timestamp);
   void Trace(blink::Visitor*);
 
  private:
   friend class LargestContentfulPaintCalculatorTest;
 
-  void ReportSwapTime(WebWidgetClient::SwapResult result,
-                      base::TimeTicks timestamp);
-  void RegisterNotifySwapTime(ReportTimeCallback callback);
-  void ResetCallbackManagerForTesting(PaintTimingCallbackManager* manager) {
-    callback_manager_ = manager;
-  }
+  void RegisterNotifySwapTime(
+      PaintTimingCallbackManager::LocalThreadCallback callback);
 
   TextRecordsManager records_manager_;
 

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

@@ -138,8 +138,7 @@ class TextPaintTimingDetectorTest
         KURL("http://test.com"));
     mock_callback_manager_ =
         MakeGarbageCollected<MockPaintTimingCallbackManager>();
-    GetTextPaintTimingDetector()->ResetCallbackManagerForTesting(
-        mock_callback_manager_);
+    GetTextPaintTimingDetector()->ResetCallbackManager(mock_callback_manager_);
     UpdateAllLifecyclePhases();
   }
 
@@ -149,7 +148,7 @@ class TextPaintTimingDetectorTest
                                                        ASSERT_NO_EXCEPTION);
     child_frame_mock_callback_manager_ =
         MakeGarbageCollected<MockPaintTimingCallbackManager>();
-    GetChildFrameTextPaintTimingDetector()->ResetCallbackManagerForTesting(
+    GetChildFrameTextPaintTimingDetector()->ResetCallbackManager(
         child_frame_mock_callback_manager_);
     UpdateAllLifecyclePhases();
   }