[Prerender] first contentful paint histograms.

chrome/browser/prerender/prerender_histograms.cc

Index: chrome/browser/prerender/prerender_histograms.cc
diff --git a/chrome/browser/prerender/prerender_histograms.cc b/chrome/browser/prerender/prerender_histograms.cc
index 1e892bfd37ed016703bd6c054e454e89bb9c514d..1718d01a58d5147f0612496376b505c35ba9566d 100644
--- a/chrome/browser/prerender/prerender_histograms.cc
+++ b/chrome/browser/prerender/prerender_histograms.cc
@@ -113,6 +113,10 @@ bool OriginIsOmnibox(Origin origin) {
   return origin == ORIGIN_OMNIBOX;
 }
 
+const char* FirstContentfulPaintHiddenName(bool was_hidden) {
+  return was_hidden ? ".Hidden" : ".Visible";
+}
+
 }  // namespace
 
 // Helper macros for origin-based histogram reporting. All HISTOGRAM arguments
@@ -301,6 +305,16 @@ void PrerenderHistograms::RecordPerceivedPageLoadTime(
   }
 }
 
+void PrerenderHistograms::RecordPerceivedFirstContentfulPaintStatus(
+    Origin origin,
+    bool successful,
+    bool was_hidden) {
+  RecordHistogramEnum(
+      GetHistogramName(origin, IsOriginWash(), "PerceivedTTFCPRecorded") +
+          FirstContentfulPaintHiddenName(was_hidden),
+      successful ? 1 : 0, 2);

[Ilya Sherman, 2016/12/27 22:46:06]

nit: Please use UmaHistogramBoolean from //base/histogram_functions.h.  (In a
separate CL, you might want to switch other uses of RecordHistogramEnum over to
these shared functions as well.)

[mattcary, 2016/12/28 09:52:33]

Done; created crbug/677257 so I don't drop switching over the other uses.

+}
+
 void PrerenderHistograms::RecordPageLoadTimeNotSwappedIn(
     Origin origin,
     base::TimeDelta page_load_time,
@@ -443,51 +457,45 @@ void PrerenderHistograms::RecordPrefetchRedirectCount(
   RecordHistogramEnum(histogram_name, redirect_count, kMaxRedirectCount);
 }
 
-void PrerenderHistograms::RecordFirstContentfulPaint(
+void PrerenderHistograms::RecordPrefetchFirstContentfulPaintTime(
     Origin origin,
     bool is_no_store,
+    bool was_hidden,
     base::TimeDelta time,
     base::TimeDelta prefetch_age) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   if (!prefetch_age.is_zero()) {
     DCHECK_NE(origin, ORIGIN_NONE);
-    RecordHistogramTime(GetHistogramName(origin, IsOriginWash(),
-                                         "Prerender.NoStatePrefetchAge"),
-                        base::TimeDelta::FromMilliseconds(10),
-                        base::TimeDelta::FromMinutes(30), prefetch_age, 50);
+    RecordHistogramTime(
+        GetHistogramName(origin, IsOriginWash(), "Prerender.PrefetchAge"),

[Ilya Sherman, 2016/12/27 22:46:06]

Why did you drop "NoState" from this histogram name?  I don't see a
corresponding update in histograms.xml.

[mattcary, 2016/12/28 09:52:33]

Oops, thanks. Updated histograms.xml to obsolete the old one and add this one.

This histogram is no recording more than just NoState prefetch ages; it will
also record effective prefetch age for prerender and non-prefetched loads as
well (depending on the experiment branch).

+        base::TimeDelta::FromMilliseconds(10), base::TimeDelta::FromMinutes(30),
+        prefetch_age, 50);
   }
 
-  RecordHistogramTime(GetFirstContentfulPaintHistogramName(
-                          origin, IsOriginWash(), is_no_store, prefetch_age),
-                      base::TimeDelta::FromMilliseconds(10),
-                      base::TimeDelta::FromMinutes(2), time, 50);
-}
-
-bool PrerenderHistograms::IsOriginWash() const {
-  if (!WithinWindow())
-    return false;
-  return origin_wash_;
-}
-
-// static
-std::string PrerenderHistograms::GetFirstContentfulPaintHistogramName(
-    Origin origin,
-    bool is_wash,
-    bool is_no_store,
-    base::TimeDelta prefetch_age) {
   std::string histogram_base_name;
   if (prefetch_age.is_zero()) {
-    histogram_base_name = "NoStatePrefetchTTFCP.Reference";
+    histogram_base_name = "PrefetchTTFCP.Reference";
   } else {
     histogram_base_name = prefetch_age < base::TimeDelta::FromMinutes(
                                              net::HttpCache::kPrefetchReuseMins)
-                              ? "NoStatePrefetchTTFCP.Warm"
-                              : "NoStatePrefetchTTFCP.Cold";

[Ilya Sherman, 2016/12/27 22:46:06]

It looks like you dropped "NoState" from this name.  Could you please mark the
corresponding histograms in histograms.xml as <obsolete>?

[mattcary, 2016/12/28 09:52:33]

Done.

I have left the obsolete histogram (both this and NoStatePrefetchAge) in
histogram_suffixes, is that appropriate?

+                              ? "PrefetchTTFCP.Warm"
+                              : "PrefetchTTFCP.Cold";
   }
 
   histogram_base_name += is_no_store ? ".NoStore" : ".Cacheable";
-  return GetHistogramName(origin, is_wash, histogram_base_name);
+  histogram_base_name += FirstContentfulPaintHiddenName(was_hidden);
+  std::string histogram_name =
+      GetHistogramName(origin, IsOriginWash(), histogram_base_name);
+
+  RecordHistogramTime(histogram_name, base::TimeDelta::FromMilliseconds(10),
+                      base::TimeDelta::FromMinutes(2), time, 50);
+}
+
+bool PrerenderHistograms::IsOriginWash() const {
+  if (!WithinWindow())
+    return false;
+  return origin_wash_;
 }
 
 }  // namespace prerender