A different approach to fixing FirstWebContentsProfiler with PlzNavigate.

chrome/browser/metrics/first_web_contents_profiler.cc

Index: chrome/browser/metrics/first_web_contents_profiler.cc
diff --git a/chrome/browser/metrics/first_web_contents_profiler.cc b/chrome/browser/metrics/first_web_contents_profiler.cc
index 2805f25b248b395b7743465dac20221d548a0412..76ac773c9e197c36d240c1fd3a2fc030af891e09 100644
--- a/chrome/browser/metrics/first_web_contents_profiler.cc
+++ b/chrome/browser/metrics/first_web_contents_profiler.cc
@@ -13,20 +13,27 @@
 #include "base/metrics/histogram_macros.h"
 #include "base/time/time.h"
 #include "build/build_config.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "components/metrics/profiler/tracking_synchronizer.h"
 #include "components/metrics/proto/profiler_event.pb.h"
 #include "components/startup_metric_utils/browser/startup_metric_utils.h"
 #include "content/public/browser/navigation_handle.h"
-
-void FirstWebContentsProfiler::WebContentsStarted(
-    content::WebContents* web_contents) {
-  static bool first_web_contents_profiled = false;
-  if (first_web_contents_profiled)
-    return;
-
-  first_web_contents_profiled = true;
-  new FirstWebContentsProfiler(web_contents);
+#include "content/public/common/browser_side_navigation_policy.h"
+
+// static
+void FirstWebContentsProfiler::Start() {
+  for (auto* browser : *BrowserList::GetInstance()) {
+    content::WebContents* web_contents =
+        browser->tab_strip_model()->GetActiveWebContents();
+    if (web_contents) {
+      // FirstWebContentsProfiler owns itself and is also bound to
+      // |web_contents|'s lifetime by observing WebContentsDestroyed().
+      new FirstWebContentsProfiler(web_contents);
+      return;
+    }
+  }
 }
 
 FirstWebContentsProfiler::FirstWebContentsProfiler(
@@ -81,6 +88,13 @@ void FirstWebContentsProfiler::DidStartNavigation(
     return;
   }
 
+  if (content::IsBrowserSideNavigationEnabled()) {
+    // With PlzNavigate, DidStartNavigation is called synchronously on
+    // browser-initiated loads instead of through an IPC. This means that we
+    // will miss this signal. Instead we record it when the commit completes.

[gab, 2016/10/24 19:48:17]

Does that mean that DidStartNavigation is invoked before creating the
FirstWebContentsProfiler? In that case I think we should either not record the
metric at all (and update its histograms.xml description to mention that it's
not recorded for PlzNavigate) or record it in the FirstWebContentsProfiler's
constructor if we think it's close enough?

Recording it on the finished signal seems wrong to me

[gab, 2016/10/24 19:58:15]

Ah nvm, read too fast, 139 doesn't record TimeTicks::Now() but time from handle,
maybe be explicit about that in this comment?

[jam, 2016/10/24 20:39:43]

IMO I don't think this is worth documenting (i.e. you found it yourself), since
anyone looking at this code can look at the function comment for
NavigationStart. I prefer not to duplicate the documentation for it here.

+    return;
+  }
+
   // The first navigation has to be the main frame's.
   DCHECK(navigation_handle->IsInMainFrame());
 
@@ -120,6 +134,12 @@ void FirstWebContentsProfiler::DidFinishNavigation(
     return;
   }
 
+  if (content::IsBrowserSideNavigationEnabled()) {
+    startup_metric_utils::RecordFirstWebContentsMainNavigationStart(
+        navigation_handle->NavigationStart());
+    collected_main_navigation_start_metric_ = true;
+  }
+
   collected_main_navigation_finished_metric_ = true;
   startup_metric_utils::RecordFirstWebContentsMainNavigationFinished(
       base::TimeTicks::Now());