A different approach to fixing FirstWebContentsProfiler with PlzNavigate.

The previous fix depended on ensuring we add hooks to all the code paths that cause the first active WebContents to get added to the tab strip. We're still missing some code paths. So instead restore the old behavior. Even though it did miss navigation starts, fix that by using the time from NavigationHandle for PlzNavigate.

This should restore the metrics for non-PlzNavigate case.
For PlzNavigate, the start timings were already different regardless from non-PlzNavigate because of how navigations are structured very differently. This change shouldn't affects things too much for PlzNavigate though, because the only difference should be the IPC time from the renderer to the browser thread.

BUG=650349

Review-Url: https://codereview.chromium.org/2448553002
Cr-Commit-Position: refs/heads/master@{#427144}

chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc

@@ -28,6 +28,10 @@
 #include "ui/display/screen.h"
 #include "ui/events/event_switches.h"
 
+#if !defined(OS_ANDROID)
+#include "chrome/browser/metrics/first_web_contents_profiler.h"
+#endif  // !defined(OS_ANDROID)
+
 #if defined(OS_ANDROID) && defined(__arm__)
 #include <cpu-features.h>
 #endif  // defined(OS_ANDROID) && defined(__arm__)
@@ -368,6 +372,7 @@ void ChromeBrowserMainExtraPartsMetrics::PostBrowserStart() {
   is_screen_observer_ = true;
 
 #if !defined(OS_ANDROID)
+  FirstWebContentsProfiler::Start();
   metrics::TabUsageRecorder::Initialize();
 #endif  // !defined(OS_ANDROID)
 }

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;
+#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.
+    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());

chrome/browser/metrics/first_web_contents_profiler.h

@@ -23,7 +23,7 @@ class FirstWebContentsProfiler : public content::WebContentsObserver {
   // Creates a profiler for the active web contents. If there are multiple
   // browsers, the first one is chosen. The resulting FirstWebContentsProfiler
   // owns itself.
-  static void WebContentsStarted(content::WebContents* web_contents);
+  static void Start();
 
  private:
   // Reasons for which profiling is deemed complete. Logged in UMA (do not re-

chrome/browser/sessions/session_restore.cc

@@ -31,7 +31,6 @@
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/lifetime/keep_alive_types.h"
 #include "chrome/browser/lifetime/scoped_keep_alive.h"
-#include "chrome/browser/metrics/first_web_contents_profiler.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/search/search.h"
 #include "chrome/browser/sessions/session_restore_delegate.h"
@@ -549,7 +548,6 @@ class SessionRestoreImpl : public content::NotificationObserver {
         if (!is_selected_tab)
           continue;
 
-        FirstWebContentsProfiler::WebContentsStarted(contents);
         ShowBrowser(browser, browser->tab_strip_model()->GetIndexOfWebContents(
                                  contents));
         // TODO(sky): remove. For debugging 368236.

chrome/browser/ui/browser_navigator.cc

@@ -55,10 +55,6 @@
 #include "extensions/common/extension_set.h"
 #endif
 
-#if !defined(OS_ANDROID)
-#include "chrome/browser/metrics/first_web_contents_profiler.h"
-#endif  // !defined(OS_ANDROID)
-
 using content::GlobalRequestID;
 using content::NavigationController;
 using content::WebContents;
@@ -385,9 +381,6 @@ content::WebContents* CreateTargetContents(const chrome::NavigateParams& params,
       SetExtensionAppById(params.extension_app_id);
 #endif
 
-#if !defined(OS_ANDROID)
-  FirstWebContentsProfiler::WebContentsStarted(target_contents);
-#endif  // !defined(OS_ANDROID)
   return target_contents;
 }