[FCP++] Allow time==0 to be propagated to UKM/UMA

Currently, LargestImagePaint, LargestTextPaint with time==0 are treated as
null and are not propagated to UMA/UKM. As we are changing to use time==0 &&
size>0 to represent that the largest content is still loading, we should
allow time==0 to be propagated to UMA/UKM.

Bug: 949974
Change-Id: I55c6d148d4a91b366fbf519a551a15f197d47b79
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1606800
Commit-Queue: Liquan (Max) Gu <maxlg@chromium.org>
Reviewed-by: Bryan McQuade <bmcquade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#663180}

chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc

@@ -820,7 +820,7 @@ void CorePageLoadMetricsObserver::RecordTimingHistograms(
         largest_content_type);
   }
 
-  const page_load_metrics::TimingInfo& paint =
+  const page_load_metrics::ContentfulPaintTimingInfo& paint =
       largest_contentful_paint_handler_.MergeMainFrameAndSubframes();
   if (!paint.IsEmpty() &&
       WasStartedInForegroundOptionalEventInForeground(paint.Time(), info)) {

chrome/browser/page_load_metrics/observers/largest_contentful_paint_handler.cc

@@ -16,8 +16,9 @@ static bool g_disable_subframe_navigation_start_offset = false;
 
 namespace {
 
-const TimingInfo& MergeTimingsBySizeAndTime(const TimingInfo& timing1,
-                                            const TimingInfo& timing2) {
+const ContentfulPaintTimingInfo& MergeTimingsBySizeAndTime(
+    const ContentfulPaintTimingInfo& timing1,
+    const ContentfulPaintTimingInfo& timing2) {
   // When both are empty, just return either.
   if (timing1.IsEmpty() && timing2.IsEmpty())
     return timing1;
@@ -39,19 +40,19 @@ const TimingInfo& MergeTimingsBySizeAndTime(const TimingInfo& timing1,
 }
 
 void MergeForSubframesWithAdjustedTime(
-    TimingInfo* inout_timing,
+    ContentfulPaintTimingInfo* inout_timing,
     const base::Optional<base::TimeDelta>& candidate_new_time,
     const uint64_t& candidate_new_size) {
   DCHECK(inout_timing);
-  const TimingInfo new_candidate(candidate_new_time, candidate_new_size,
-                                 inout_timing->Type());
-  const TimingInfo& merged_candidate =
+  const ContentfulPaintTimingInfo new_candidate(
+      candidate_new_time, candidate_new_size, inout_timing->Type());
+  const ContentfulPaintTimingInfo& merged_candidate =
       MergeTimingsBySizeAndTime(new_candidate, *inout_timing);
   inout_timing->Reset(merged_candidate.Time(), merged_candidate.Size());
 }
 
 void MergeForSubframes(
-    TimingInfo* inout_timing,
+    ContentfulPaintTimingInfo* inout_timing,
     const base::Optional<base::TimeDelta>& candidate_new_time,
     const uint64_t& candidate_new_size,
     base::TimeDelta navigation_start_offset) {
@@ -68,18 +69,20 @@ bool IsSubframe(content::RenderFrameHost* subframe_rfh) {
 
 }  // namespace
 
-TimingInfo::TimingInfo(PageLoadMetricsObserver::LargestContentType type)
+ContentfulPaintTimingInfo::ContentfulPaintTimingInfo(
+    PageLoadMetricsObserver::LargestContentType type)
     : time_(base::Optional<base::TimeDelta>()), size_(0), type_(type) {}
-TimingInfo::TimingInfo(
+ContentfulPaintTimingInfo::ContentfulPaintTimingInfo(
     const base::Optional<base::TimeDelta>& time,
     const uint64_t& size,
     const page_load_metrics::PageLoadMetricsObserver::LargestContentType type)
     : time_(time), size_(size), type_(type) {}
 
-TimingInfo::TimingInfo(const TimingInfo& other) = default;
+ContentfulPaintTimingInfo::ContentfulPaintTimingInfo(
+    const ContentfulPaintTimingInfo& other) = default;
 
-std::unique_ptr<base::trace_event::TracedValue> TimingInfo::DataAsTraceValue()
-    const {
+std::unique_ptr<base::trace_event::TracedValue>
+ContentfulPaintTimingInfo::DataAsTraceValue() const {
   std::unique_ptr<base::trace_event::TracedValue> data =
       std::make_unique<base::trace_event::TracedValue>();
   data->SetInteger("durationInMilliseconds", time_.value().InMilliseconds());
@@ -88,7 +91,7 @@ std::unique_ptr<base::trace_event::TracedValue> TimingInfo::DataAsTraceValue()
   return data;
 }
 
-std::string TimingInfo::TypeInString() const {
+std::string ContentfulPaintTimingInfo::TypeInString() const {
   switch (Type()) {
     case page_load_metrics::PageLoadMetricsObserver::LargestContentType::kText:
       return "text";
@@ -105,18 +108,18 @@ void LargestContentfulPaintHandler::SetTestMode(bool enabled) {
   g_disable_subframe_navigation_start_offset = enabled;
 }
 
-void TimingInfo::Reset(const base::Optional<base::TimeDelta>& time,
+void ContentfulPaintTimingInfo::Reset(
+    const base::Optional<base::TimeDelta>& time,
     const uint64_t& size) {
   size_ = size;
   time_ = time;
-  DCHECK(HasConsistentTimeAndSize());
 }
 
 ContentfulPaint::ContentfulPaint()
     : text_(PageLoadMetricsObserver::LargestContentType::kText),
       image_(PageLoadMetricsObserver::LargestContentType::kImage) {}
 
-const TimingInfo& ContentfulPaint::MergeTextAndImageTiming() {
+const ContentfulPaintTimingInfo& ContentfulPaint::MergeTextAndImageTiming() {
   return MergeTimingsBySizeAndTime(text_, image_);
 }
 
@@ -144,10 +147,11 @@ void LargestContentfulPaintHandler::RecordTiming(
   RecordSubframeTiming(timing, navigation_start_offset);
 }
 
-const TimingInfo& LargestContentfulPaintHandler::MergeMainFrameAndSubframes() {
-  const TimingInfo& main_frame_timing =
+const ContentfulPaintTimingInfo&
+LargestContentfulPaintHandler::MergeMainFrameAndSubframes() {
+  const ContentfulPaintTimingInfo& main_frame_timing =
       main_frame_contentful_paint_.MergeTextAndImageTiming();
-  const TimingInfo& subframe_timing =
+  const ContentfulPaintTimingInfo& subframe_timing =
       subframe_contentful_paint_.MergeTextAndImageTiming();
   return MergeTimingsBySizeAndTime(main_frame_timing, subframe_timing);
 }

chrome/browser/page_load_metrics/observers/largest_contentful_paint_handler.h

@@ -13,44 +13,34 @@
 
 namespace page_load_metrics {
 
-class TimingInfo {
+class ContentfulPaintTimingInfo {
  public:
-  explicit TimingInfo(
+  explicit ContentfulPaintTimingInfo(
       page_load_metrics::PageLoadMetricsObserver::LargestContentType);
-  explicit TimingInfo(
+  explicit ContentfulPaintTimingInfo(
       const base::Optional<base::TimeDelta>&,
       const uint64_t& size,
       const page_load_metrics::PageLoadMetricsObserver::LargestContentType);
-  explicit TimingInfo(const TimingInfo& other);
+  explicit ContentfulPaintTimingInfo(const ContentfulPaintTimingInfo& other);
   void Reset(const base::Optional<base::TimeDelta>&, const uint64_t& size);
   base::Optional<base::TimeDelta> Time() const {
-    DCHECK(HasConsistentTimeAndSize());
     return time_;
   }
-  uint64_t Size() const {
-    DCHECK(HasConsistentTimeAndSize());
-    return size_;
-  }
+  uint64_t Size() const { return size_; }
   page_load_metrics::PageLoadMetricsObserver::LargestContentType Type() const {
-    DCHECK(HasConsistentTimeAndSize());
     return type_;
   }
   bool IsEmpty() const {
-    DCHECK(HasConsistentTimeAndSize());
-    // |size_| will be 0 as well, as checked by the DCHECK.
+    // |size_| is not necessarily 0, for example, when the largest image is
+    // still loading.
     return !time_;
   }
 
   std::unique_ptr<base::trace_event::TracedValue> DataAsTraceValue() const;
 
  private:
-  TimingInfo() = delete;
+  ContentfulPaintTimingInfo() = delete;
   std::string TypeInString() const;
-  // This is only for DCHECK. We will never need the inconsistent state.
-  bool HasConsistentTimeAndSize() const {
-    return (time_ && size_) || (!time_ && !size_);
-  }
-  // This uses mainthread navigation start as origin.
   base::Optional<base::TimeDelta> time_;
   uint64_t size_;
   page_load_metrics::PageLoadMetricsObserver::LargestContentType type_;
@@ -59,13 +49,13 @@ class TimingInfo {
 class ContentfulPaint {
  public:
   ContentfulPaint();
-  TimingInfo& Text() { return text_; }
-  TimingInfo& Image() { return image_; }
-  const TimingInfo& MergeTextAndImageTiming();
+  ContentfulPaintTimingInfo& Text() { return text_; }
+  ContentfulPaintTimingInfo& Image() { return image_; }
+  const ContentfulPaintTimingInfo& MergeTextAndImageTiming();
 
  private:
-  TimingInfo text_;
-  TimingInfo image_;
+  ContentfulPaintTimingInfo text_;
+  ContentfulPaintTimingInfo image_;
 };
 
 class LargestContentfulPaintHandler {
@@ -79,7 +69,7 @@ class LargestContentfulPaintHandler {
                     content::RenderFrameHost* subframe_rfh);
   // We merge the candidates from main frame and subframe to get the largest
   // candidate across all frames.
-  const TimingInfo& MergeMainFrameAndSubframes();
+  const ContentfulPaintTimingInfo& MergeMainFrameAndSubframes();
   void OnDidFinishSubFrameNavigation(
       content::NavigationHandle* navigation_handle,
       const page_load_metrics::PageLoadExtraInfo& extra_info);

chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.cc

@@ -303,7 +303,7 @@ void UkmPageLoadMetricsObserver::RecordTimingMetrics(
     builder.SetExperimental_PaintTiming_NavigationToLargestContentPaint(
         largest_content_paint_time.value().InMilliseconds());
   }
-  const page_load_metrics::TimingInfo& paint =
+  const page_load_metrics::ContentfulPaintTimingInfo& paint =
       largest_contentful_paint_handler_.MergeMainFrameAndSubframes();
   if (!paint.IsEmpty() &&
       WasStartedInForegroundOptionalEventInForeground(paint.Time(), info)) {

chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc

@@ -337,17 +337,19 @@ mojom::PageLoadTimingPtr MetricsRenderFrameObserver::GetTiming() const {
     timing->paint_timing->first_meaningful_paint =
         ClampDelta(perf.FirstMeaningfulPaint(), start);
   }
-  if (perf.LargestImagePaint() > 0.0) {
+  if (perf.LargestImagePaintSize() > 0) {
     timing->paint_timing->largest_image_paint =
-        ClampDelta(perf.LargestImagePaint(), start);
-    DCHECK(perf.LargestImagePaintSize() > 0);
+        perf.LargestImagePaint() == 0.0
+            ? base::TimeDelta()
+            : ClampDelta(perf.LargestImagePaint(), start);
     timing->paint_timing->largest_image_paint_size =
         perf.LargestImagePaintSize();
   }
-  if (perf.LargestTextPaint() > 0.0) {
+  if (perf.LargestTextPaintSize() > 0) {
     timing->paint_timing->largest_text_paint =
-        ClampDelta(perf.LargestTextPaint(), start);
-    DCHECK(perf.LargestTextPaintSize() > 0);
+        perf.LargestTextPaint() == 0.0
+            ? base::TimeDelta()
+            : ClampDelta(perf.LargestTextPaint(), start);
     timing->paint_timing->largest_text_paint_size = perf.LargestTextPaintSize();
   }
   if (perf.ParseStart() > 0.0)