FirstMeaningfulPaint UMA

chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc

Index: chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc
diff --git a/chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc b/chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc
index eb81d4042bdfd64439d6d3e0216e29b2518f2a35..958c65d3663a8520ccc55032b439045b8105b980 100644
--- a/chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc
+++ b/chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc
@@ -62,6 +62,13 @@ PageLoadType GetPageLoadType(ui::PageTransition transition) {
   return LOAD_TYPE_NONE;
 }
 
+enum FirstMeaningfulPaintStatus {
+  FMP_RECORDED,
+  FMP_DID_NOT_REACH_NETWORK_STABLE,
+  FMP_USER_INTERACTION_BEFORE_FMP,
+  FMP_ENUM_MAX
+};
+
 }  // namespace
 
 namespace internal {
@@ -154,6 +161,10 @@ const char kHistogramFirstContentfulPaintImmediate[] =
     "PageLoad.PaintTiming.NavigationToFirstContentfulPaint";
 const char kBackgroundHistogramFirstContentfulPaintImmediate[] =
     "PageLoad.PaintTiming.NavigationToFirstContentfulPaint.Background";
+const char kHistogramFirstMeaningfulPaintImmediate[] =
+    "PageLoad.PaintTiming.NavigationToFirstMeaningfulPaint";
+const char kBackgroundHistogramFirstMeaningfulPaintImmediate[] =
+    "PageLoad.PaintTiming.NavigationToFirstMeaningfulPaint.Background";
 const char kHistogramParseStartToFirstContentfulPaintImmediate[] =
     "PageLoad.PaintTiming.ParseStartToFirstContentfulPaint";
 const char kBackgroundHistogramParseStartToFirstContentfulPaintImmediate[] =
@@ -162,6 +173,10 @@ const char kHistogramParseStartImmediate[] =
     "PageLoad.ParseTiming.NavigationToParseStart";
 const char kBackgroundHistogramParseStartImmediate[] =
     "PageLoad.ParseTiming.NavigationToParseStart.Background";
+const char kHistogramFirstMeaningfulPaintToNetworkStable[] =
+    "PageLoad.PaintTiming.FirstMeaningfulPaintToNetworkStable";
+const char kBackgroundHistogramFirstMeaningfulPaintToNetworkStable[] =
+    "PageLoad.PaintTiming.FirstMeaningfulPaintToNetworkStable.Background";
 const char kHistogramParseDurationImmediate[] =
     "PageLoad.ParseTiming.ParseDuration";
 const char kBackgroundHistogramParseDurationImmediate[] =
@@ -218,6 +233,9 @@ const char kHistogramForegroundToFirstPaint[] =
 const char kRapporMetricsNameCoarseTiming[] =
     "PageLoad.CoarseTiming.NavigationToFirstContentfulPaint";
 
+const char kHistogramFirstMeaningfulPaintStatus[] =
+    "PageLoad.PaintTiming.FirstMeaningfulPaintStatus";
+
 }  // namespace internal
 
 CorePageLoadMetricsObserver::CorePageLoadMetricsObserver()
@@ -356,6 +374,35 @@ void CorePageLoadMetricsObserver::OnFirstContentfulPaint(
   }
 }
 
+void CorePageLoadMetricsObserver::OnFirstMeaningfulPaint(
+    const page_load_metrics::PageLoadTiming& timing,
+    const page_load_metrics::PageLoadExtraInfo& info) {
+  base::Time stamp =

[Bryan McQuade, 2016/07/29 15:02:26]

Ah, we need to get rid of the base::Time for navigation_start, as base::Time is
non-monotonic.

Instead, can you add a base::TimeTicks navigation_start_ member to this class,
and set it in OnCommit:
navigation_start_ = navigation_handle->GetNavigationStart();  (i think that's
the method name...)

Then you can create a timestamp using TimeTicks which is monotonic.

[Kunihiko Sakamoto, 2016/08/01 08:52:07]

Done.

+      timing.navigation_start + timing.first_meaningful_paint.value();
+  if (!first_user_interaction_time_ || stamp < first_user_interaction_time_) {

[Bryan McQuade, 2016/07/29 15:02:26]

I'm not sure I agree that FMP should only be recorded if it's before
interaction. If I just touch the screen while it's loading for instance, with no
intent of actually interacting, that doesn't seem to meaningfully affect whether
we should log FMP.

If you want to log a separate histogram restricted to only cases before any
interactions,
PageLoad.PaintTiming.NavigationToFirstMeaningfulPaint.BeforeInteraction, I could
see potentially doing that, but I'm still not sure it's useful. Not clear to me
that whether the user has touched the screen is indicative of a meaningful paint
happening or not.

[Kunihiko Sakamoto, 2016/08/01 08:52:07]

Since current implementation has fairly long layout observation period, I think
there is a risk of FMPDetector capturing a layout that triggered by user
interactions (for example, user opens a thread in GMail). I wanted to make
initial FMP implementation have as few noises as possible.

Ideally we'd filter user interactions that triggered some JavaScript work (not
sure how hard that is). But before trying to implement complicated heuristics, I
would like to see how often FMP comes after user interaction.

I'm not sure if it would be useful if we log another FMP histogram including
values after user interactions. I'm happy to add it if it has any use cases.

+    if (WasStartedInForegroundOptionalEventInForeground(
+            timing.first_meaningful_paint, info)) {
+      PAGE_LOAD_HISTOGRAM(internal::kHistogramFirstMeaningfulPaintImmediate,
+          timing.first_meaningful_paint.value());
+      PAGE_LOAD_HISTOGRAM(
+          internal::kHistogramFirstMeaningfulPaintToNetworkStable,
+          base::Time::Now() - stamp);
+    } else {
+      PAGE_LOAD_HISTOGRAM(
+          internal::kBackgroundHistogramFirstMeaningfulPaintImmediate,
+          timing.first_meaningful_paint.value());
+      PAGE_LOAD_HISTOGRAM(
+          internal::kBackgroundHistogramFirstMeaningfulPaintToNetworkStable,
+          base::Time::Now() - stamp);
+    }
+    UMA_HISTOGRAM_ENUMERATION(internal::kHistogramFirstMeaningfulPaintStatus,
+        FMP_RECORDED, FMP_ENUM_MAX);
+  } else {
+    UMA_HISTOGRAM_ENUMERATION(internal::kHistogramFirstMeaningfulPaintStatus,
+        FMP_USER_INTERACTION_BEFORE_FMP, FMP_ENUM_MAX);
+  }
+}
+
 void CorePageLoadMetricsObserver::OnParseStart(
     const page_load_metrics::PageLoadTiming& timing,
     const page_load_metrics::PageLoadExtraInfo& info) {
@@ -444,6 +491,14 @@ void CorePageLoadMetricsObserver::OnFailedProvisionalLoad(
   }
 }
 
+void CorePageLoadMetricsObserver::OnUserInput(
+    const blink::WebInputEvent& event) {
+  if (!first_user_interaction_time_ &&
+      event.type != blink::WebInputEvent::MouseMove) {
+    first_user_interaction_time_ = base::Time::Now();

[Bryan McQuade, 2016/07/29 15:02:26]

same - I'm hoping we can remove this, but we should use TimeTicks, which is
monotonic, rather than Time.

[Kunihiko Sakamoto, 2016/08/01 08:52:06]

Done.

+  }
+}
+
 void CorePageLoadMetricsObserver::RecordTimingHistograms(
     const page_load_metrics::PageLoadTiming& timing,
     const page_load_metrics::PageLoadExtraInfo& info) {
@@ -621,6 +676,11 @@ void CorePageLoadMetricsObserver::RecordTimingHistograms(
       PAGE_LOAD_HISTOGRAM(internal::kHistogramFirstForeground,
                           info.first_foreground_time.value());
   }
+
+  if (!timing.first_meaningful_paint) {
+    UMA_HISTOGRAM_ENUMERATION(internal::kHistogramFirstMeaningfulPaintStatus,
+                              FMP_DID_NOT_REACH_NETWORK_STABLE, FMP_ENUM_MAX);
+  }
 }
 
 void CorePageLoadMetricsObserver::RecordRappor(