[Prerender] first contentful paint histograms.

chrome/browser/prerender/prerender_manager.cc

Index: chrome/browser/prerender/prerender_manager.cc
diff --git a/chrome/browser/prerender/prerender_manager.cc b/chrome/browser/prerender/prerender_manager.cc
index f908885fbe4ae8ed9c6137a9c76683a160aca4ef..c2e92bafd5def0795ebca21e2e9b227376c0b07b 100644
--- a/chrome/browser/prerender/prerender_manager.cc
+++ b/chrome/browser/prerender/prerender_manager.cc
@@ -157,6 +157,8 @@ class PrerenderManager::OnCloseWebContentsDeleter
   DISALLOW_COPY_AND_ASSIGN(OnCloseWebContentsDeleter);
 };
 
+PrerenderManagerObserver::~PrerenderManagerObserver() {}
+
 // static
 int PrerenderManager::prerenders_per_session_count_ = 0;
 
@@ -182,6 +184,7 @@ PrerenderManager::PrerenderManager(Profile* profile)
       last_recorded_profile_network_bytes_(0),
       clock_(new base::DefaultClock()),
       tick_clock_(new base::DefaultTickClock()),
+      page_load_metric_observer_disabled_(false),
       weak_factory_(this) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
@@ -573,31 +576,52 @@ void PrerenderManager::RecordPrefetchRedirectCount(Origin origin,
                                            redirect_count);
 }
 
-void PrerenderManager::RecordFirstContentfulPaint(const GURL& url,
-                                                  bool is_no_store,
-                                                  base::TimeDelta time) {
-  CleanUpOldNavigations(&prefetches_, base::TimeDelta::FromMinutes(30));
-
-  // Compute the prefetch age.
+void PrerenderManager::RecordNoStateFirstContentfulPaint(const GURL& url,
+                                                         bool is_no_store,
+                                                         bool was_hidden,
+                                                         base::TimeDelta time) {
   base::TimeDelta prefetch_age;
-  Origin origin = ORIGIN_NONE;
-  for (auto it = prefetches_.crbegin(); it != prefetches_.crend(); ++it) {
-    if (it->url == url) {
-      prefetch_age = GetCurrentTimeTicks() - it->time;
-      origin = it->origin;
-      break;
-    }
+  Origin origin;
+  GetPrefetchInformation(url, &prefetch_age, &origin);
+
+  histograms_->RecordPrefetchFirstContentfulPaintTime(
+      origin, is_no_store, was_hidden, time, prefetch_age);
+
+  for (auto& observer : observers_) {
+    observer->OnFirstContentfulPaint();
   }
+}
 
-  histograms_->RecordFirstContentfulPaint(origin, is_no_store, time,
-                                          prefetch_age);
+void PrerenderManager::RecordPrerenderFirstContentfulPaint(
+    const GURL& url,
+    content::WebContents* web_contents,
+    bool is_no_store,
+    bool was_hidden,
+    base::TimeTicks first_contentful_paint) {
+  PrerenderTabHelper* tab_helper =
+      PrerenderTabHelper::FromWebContents(web_contents);
+  DCHECK(tab_helper);
 
-  // Loading a prefetched URL resets the revalidation bypass. Remove the url
-  // from the prefetch list for more accurate metrics.
-  prefetches_.erase(
-      std::remove_if(prefetches_.begin(), prefetches_.end(),
-                     [url](const NavigationRecord& r) { return r.url == url; }),
-      prefetches_.end());
+  base::TimeDelta prefetch_age;
+  // The prefetch origin may not be relevant to the prerender.
+  Origin unused_origin;
+  GetPrefetchInformation(url, &prefetch_age, &unused_origin);
+
+  base::TimeTicks last_swap = tab_helper->last_swap();
+  if (!last_swap.is_null() && !first_contentful_paint.is_null()) {

[pasko, 2016/12/21 18:39:09]

this gets recorded from a notification for FCP. In which cases do we hit
first_contentful_paint.is_null()? Is it only for testing?

[mattcary, 2016/12/22 10:49:07]

Maybe something goes wrong with the PageLoadMetricsObserver timing? Who knows,
data can go bad in many ways. Even if we DCHECK'd this, we'd still want to avoid
putting corrupt data into histogram: note that a null FCP time would mean that
we'd record a negative bucket into the histogram which would cause all sorts of
hilarity.

[pasko, 2016/12/22 18:22:00]

OK, I started suspecting that I am crazy. Glad that we don't have the evidence
coming from here.

Then this lg. Lemme suggest to add DCHECK(!first_contentful_paint.is_null())
without removing the test for it before recording the histogram. Adds potential
to catch it in testing, also some readability about us being unsure about this
thing we depend on not going out of control. The effect would be small, up to
you.

[mattcary, 2016/12/23 09:58:15]

Done.

+    histograms_->RecordPrefetchFirstContentfulPaintTime(
+        tab_helper->origin(), is_no_store, was_hidden,
+        first_contentful_paint - last_swap, prefetch_age);
+    histograms_->RecordPerceivedFirstContentfulPaintStatus(tab_helper->origin(),
+                                                           true, was_hidden);
+  } else {
+    histograms_->RecordPerceivedFirstContentfulPaintStatus(tab_helper->origin(),
+                                                           false, was_hidden);
+  }
+
+  for (auto& observer : observers_) {
+    observer->OnFirstContentfulPaint();
+  }
 }
 
 // static
@@ -1048,6 +1072,8 @@ void PrerenderManager::PeriodicCleanup() {
 
   to_delete_prerenders_.clear();
 
+  CleanUpOldNavigations(&prefetches_, base::TimeDelta::FromMinutes(30));
+
   // Measure how long a the various cleanup tasks took. http://crbug.com/305419.
   UMA_HISTOGRAM_TIMES("Prerender.PeriodicCleanupDeleteContentsTime",
                       cleanup_timer.Elapsed());
@@ -1101,6 +1127,11 @@ void PrerenderManager::SetTickClockForTesting(
   tick_clock_ = std::move(tick_clock);
 }
 
+void PrerenderManager::AddObserver(
+    std::unique_ptr<PrerenderManagerObserver> observer) {
+  observers_.push_back(std::move(observer));
+}
+
 std::unique_ptr<PrerenderContents> PrerenderManager::CreatePrerenderContents(
     const GURL& url,
     const content::Referrer& referrer,
@@ -1163,6 +1194,31 @@ void PrerenderManager::DeleteOldWebContents() {
   old_web_contents_list_.clear();
 }
 
+void PrerenderManager::GetPrefetchInformation(const GURL& url,
+                                              base::TimeDelta* prefetch_age,
+                                              Origin* origin) {
+  DCHECK(prefetch_age);
+  DCHECK(origin);
+  CleanUpOldNavigations(&prefetches_, base::TimeDelta::FromMinutes(30));
+
+  *prefetch_age = base::TimeDelta();
+  *origin = ORIGIN_NONE;
+  for (auto it = prefetches_.crbegin(); it != prefetches_.crend(); ++it) {
+    if (it->url == url) {
+      *prefetch_age = GetCurrentTimeTicks() - it->time;
+      *origin = it->origin;
+      break;
+    }
+  }
+
+  // Loading a prefetched URL resets the revalidation bypass. Remove all
+  // matching urls from the prefetch list for more accurate metrics.
+  prefetches_.erase(
+      std::remove_if(prefetches_.begin(), prefetches_.end(),
+                     [url](const NavigationRecord& r) { return r.url == url; }),
+      prefetches_.end());
+}
+
 void PrerenderManager::CleanUpOldNavigations(
     std::vector<NavigationRecord>* navigations,
     base::TimeDelta max_age) {