[M77][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
(cherry picked from commit 4e8c79343bb9f35d6c9358f3f7714b75288adc49)
Change-Id: Ic9bc3076d163b471d91e6414da70bd6d7af0fd6d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1730517
Commit-Queue: Liquan (Max) Gu <[email protected]>
Reviewed-by: Steve Kobes <[email protected]>
Cr-Original-Commit-Position: refs/heads/master@{#683470}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1748605
Reviewed-by: Nicolás Peña Moreno <[email protected]>
Cr-Commit-Position: refs/branch-heads/3865@{#332}
Cr-Branched-From: 0cdcc6158160790658d1f033d3db873603250124-refs/heads/master@{#681094}
diff --git a/third_party/blink/renderer/core/paint/image_paint_timing_detector.cc b/third_party/blink/renderer/core/paint/image_paint_timing_detector.cc
index af391d77..a94e6abe 100644
--- a/third_party/blink/renderer/core/paint/image_paint_timing_detector.cc
+++ b/third_party/blink/renderer/core/paint/image_paint_timing_detector.cc
@@ -60,10 +60,10 @@
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::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;
diff --git a/third_party/blink/renderer/core/paint/image_paint_timing_detector.h b/third_party/blink/renderer/core/paint/image_paint_timing_detector.h
index a9c9e2b..b458d31 100644
--- a/third_party/blink/renderer/core/paint/image_paint_timing_detector.h
+++ b/third_party/blink/renderer/core/paint/image_paint_timing_detector.h
@@ -204,7 +204,7 @@
friend class ImagePaintTimingDetectorTest;
public:
- ImagePaintTimingDetector(LocalFrameView*);
+ ImagePaintTimingDetector(LocalFrameView*, PaintTimingCallbackManager*);
void RecordImage(const LayoutObject&,
const IntSize& intrinsic_size,
const ImageResourceContent&,
@@ -223,6 +223,10 @@
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*);
@@ -232,13 +236,6 @@
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();
diff --git a/third_party/blink/renderer/core/paint/image_paint_timing_detector_test.cc b/third_party/blink/renderer/core/paint/image_paint_timing_detector_test.cc
index f923dab..99c2aab 100644
--- a/third_party/blink/renderer/core/paint/image_paint_timing_detector_test.cc
+++ b/third_party/blink/renderer/core/paint/image_paint_timing_detector_test.cc
@@ -176,7 +176,7 @@
MakeGarbageCollected<MockPaintTimingCallbackManager>();
GetPaintTimingDetector()
.GetImagePaintTimingDetector()
- ->ResetCallbackManagerForTesting(mock_callback_manager_);
+ ->ResetCallbackManager(mock_callback_manager_);
UpdateAllLifecyclePhases();
}
@@ -188,7 +188,7 @@
MakeGarbageCollected<MockPaintTimingCallbackManager>();
GetChildPaintTimingDetector()
.GetImagePaintTimingDetector()
- ->ResetCallbackManagerForTesting(child_mock_callback_manager_);
+ ->ResetCallbackManager(child_mock_callback_manager_);
UpdateAllLifecyclePhases();
}
@@ -263,7 +263,7 @@
return original_image_resource;
}
- CallbackQueue callback_queue_;
+ PaintTimingCallbackManager::CallbackQueue callback_queue_;
Persistent<MockPaintTimingCallbackManager> mock_callback_manager_;
Persistent<MockPaintTimingCallbackManager> child_mock_callback_manager_;
};
diff --git a/third_party/blink/renderer/core/paint/largest_contentful_paint_calculator_test.cc b/third_party/blink/renderer/core/paint/largest_contentful_paint_calculator_test.cc
index 6210886..f43b595 100644
--- a/third_party/blink/renderer/core/paint/largest_contentful_paint_calculator_test.cc
+++ b/third_party/blink/renderer/core/paint/largest_contentful_paint_calculator_test.cc
@@ -26,11 +26,11 @@
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_);
}
diff --git a/third_party/blink/renderer/core/paint/paint_timing_detector.cc b/third_party/blink/renderer/core/paint/paint_timing_detector.cc
index 6ccfd00..9385ee0 100644
--- a/third_party/blink/renderer/core/paint/paint_timing_detector.cc
+++ b/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 @@
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 @@
if (image_paint_timing_detector_->FinishedReportingImages())
image_paint_timing_detector_ = nullptr;
}
+ if (callback_manager_->CountCallbacks() > 0)
+ callback_manager_->RegisterPaintTimeCallbackForCombinedCallbacks();
}
// static
@@ -328,12 +340,42 @@
visitor->Trace(image_paint_timing_detector_);
visitor->Trace(frame_view_);
visitor->Trace(largest_contentful_paint_calculator_);
+ visitor->Trace(callback_manager_);
}
-void PaintTimingCallbackManagerImpl::RegisterCallback(
- LocalFrame& frame,
- ReportTimeCallback callback) {
- frame.GetPage()->GetChromeClient().NotifySwapTime(frame, std::move(callback));
+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::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
diff --git a/third_party/blink/renderer/core/paint/paint_timing_detector.h b/third_party/blink/renderer/core/paint/paint_timing_detector.h
index 3cd50468..269d629 100644
--- a/third_party/blink/renderer/core/paint/paint_timing_detector.h
+++ b/third_party/blink/renderer/core/paint/paint_timing_detector.h
@@ -25,9 +25,6 @@
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 @@
// 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>;
+
+ virtual void RegisterCallback(
+ PaintTimingCallbackManager::LocalThreadCallback) = 0;
};
-using ReportTimeCallback =
- WTF::CrossThreadOnceFunction<void(WebWidgetClient::SwapResult,
- base::TimeTicks)>;
-
-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 @@
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;
diff --git a/third_party/blink/renderer/core/paint/paint_timing_test_helper.h b/third_party/blink/renderer/core/paint/paint_timing_test_helper.h
index 7817bca..8d1dfb2 100644
--- a/third_party/blink/renderer/core/paint/paint_timing_test_helper.h
+++ b/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 @@
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 @@
void Trace(Visitor* visitor) override {}
private:
- CallbackQueue callback_queue_;
+ PaintTimingCallbackManager::CallbackQueue callback_queue_;
};
} // namespace blink
diff --git a/third_party/blink/renderer/core/paint/text_paint_timing_detector.cc b/third_party/blink/renderer/core/paint/text_paint_timing_detector.cc
index dc996c3..390ea30 100644
--- a/third_party/blink/renderer/core/paint/text_paint_timing_detector.cc
+++ b/third_party/blink/renderer/core/paint/text_paint_timing_detector.cc
@@ -33,9 +33,10 @@
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 @@
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::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()) {
diff --git a/third_party/blink/renderer/core/paint/text_paint_timing_detector.h b/third_party/blink/renderer/core/paint/text_paint_timing_detector.h
index f441637..149c826 100644
--- a/third_party/blink/renderer/core/paint/text_paint_timing_detector.h
+++ b/third_party/blink/renderer/core/paint/text_paint_timing_detector.h
@@ -193,13 +193,12 @@
// 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 @@
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_;
diff --git a/third_party/blink/renderer/core/paint/text_paint_timing_detector_test.cc b/third_party/blink/renderer/core/paint/text_paint_timing_detector_test.cc
index 0219171..b3171ed 100644
--- a/third_party/blink/renderer/core/paint/text_paint_timing_detector_test.cc
+++ b/third_party/blink/renderer/core/paint/text_paint_timing_detector_test.cc
@@ -138,8 +138,7 @@
KURL("http://test.com"));
mock_callback_manager_ =
MakeGarbageCollected<MockPaintTimingCallbackManager>();
- GetTextPaintTimingDetector()->ResetCallbackManagerForTesting(
- mock_callback_manager_);
+ GetTextPaintTimingDetector()->ResetCallbackManager(mock_callback_manager_);
UpdateAllLifecyclePhases();
}
@@ -149,7 +148,7 @@
ASSERT_NO_EXCEPTION);
child_frame_mock_callback_manager_ =
MakeGarbageCollected<MockPaintTimingCallbackManager>();
- GetChildFrameTextPaintTimingDetector()->ResetCallbackManagerForTesting(
+ GetChildFrameTextPaintTimingDetector()->ResetCallbackManager(
child_frame_mock_callback_manager_);
UpdateAllLifecyclePhases();
}
