A different approach to fixing FirstWebContentsProfiler with PlzNavigate.

chrome/browser/metrics/first_web_contents_profiler.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #if !defined(OS_ANDROID)
 
 #include "chrome/browser/metrics/first_web_contents_profiler.h"
 
 #include <string>
 
 #include "base/location.h"
 #include "base/logging.h"
 #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"
+#include "content/public/common/browser_side_navigation_policy.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);
+// 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(
     content::WebContents* web_contents)
     : content::WebContentsObserver(web_contents),
       collected_paint_metric_(false),
       collected_load_metric_(false),
       collected_main_navigation_start_metric_(false),
       collected_main_navigation_finished_metric_(false) {}
 
@@ skipping 34 matching lines @@
 
 void FirstWebContentsProfiler::DidStartNavigation(
     content::NavigationHandle* navigation_handle) {
   if (collected_main_navigation_start_metric_)
     return;
   if (startup_metric_utils::WasNonBrowserUIDisplayed()) {
     FinishedCollectingMetrics(FinishReason::ABANDON_BLOCKING_UI);
     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());
 
   collected_main_navigation_start_metric_ = true;
   startup_metric_utils::RecordFirstWebContentsMainNavigationStart(
       base::TimeTicks::Now());
 }
 
 void FirstWebContentsProfiler::DidFinishNavigation(
     content::NavigationHandle* navigation_handle) {
@@ skipping 19 matching lines @@
 
   // The first navigation has to be the main frame's.
   DCHECK(navigation_handle->IsInMainFrame());
 
   if (!navigation_handle->HasCommitted() ||
       navigation_handle->IsErrorPage()) {
     FinishedCollectingMetrics(FinishReason::ABANDON_NAVIGATION_ERROR);
     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());
 }
 
 void FirstWebContentsProfiler::WasHidden() {
   // Stop profiling if the content gets hidden as its load may be deprioritized
   // and timing it becomes meaningless.
   FinishedCollectingMetrics(FinishReason::ABANDON_CONTENT_HIDDEN);
 }
@@ skipping 16 matching lines @@
   }
   if (!collected_load_metric_) {
     UMA_HISTOGRAM_ENUMERATION("Startup.FirstWebContents.FinishReason_NoLoad",
                               finish_reason, FinishReason::ENUM_MAX);
   }
 
   delete this;
 }
 
 #endif  // !defined(OS_ANDROID)