Mark Startup.FirstWebContents.MainFrameLoad2 as obsolete.

Startup.FirstWebContents.NonEmptyPaint2 is a better indicator of startup time as perceived by users. Also, remove the suffix on Startup.FirstWebContents.FinishReason. Since Startup.FirstWebContents.MainFrameLoad2 is no longer tracked, the non-suffixed FinishReason is always for Startup.FirstWebContents.NonEmptyPaint2 and the suffix does not provide additional information. Bug: 975113 Change-Id: I2ae72627e809d61e939ecb63f8a7a190ee72ffa8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1872316 Reviewed-by: Peter Boström <pbos@chromium.org> Reviewed-by: Gabriel Charette <gab@chromium.org> Reviewed-by: Robert Kaplow <rkaplow@chromium.org> Commit-Queue: François Doray <fdoray@chromium.org> Cr-Commit-Position: refs/heads/master@{#711759}
cptjacky · Nov 1, 2019 · 9e6a2ef · 9e6a2ef
1 parent b6ace0f
commit 9e6a2ef
Show file tree

Hide file tree

Showing 6 changed files with 22 additions and 92 deletions.
diff --git a/chrome/browser/metrics/first_web_contents_profiler.cc b/chrome/browser/metrics/first_web_contents_profiler.cc
@@ -52,33 +52,17 @@ class FirstWebContentsProfiler : public content::WebContentsObserver {
 
   // content::WebContentsObserver:
   void DidFirstVisuallyNonEmptyPaint() override;
-  void DocumentOnLoadCompletedInMainFrame() override;
-  void DidStartNavigation(
-      content::NavigationHandle* navigation_handle) override;
   void DidFinishNavigation(
       content::NavigationHandle* navigation_handle) override;
   void OnVisibilityChanged(content::Visibility visibility) override;
   void WebContentsDestroyed() override;
 
-  // Whether this instance has finished collecting first-paint and main-frame-
-  // load metrics (navigation metrics are recorded on a best effort but don't
-  // prevent the FirstWebContentsProfiler from calling it).
-  bool IsFinishedCollectingMetrics();
-
   // Logs |finish_reason| to UMA and deletes this FirstWebContentsProfiler.
   void FinishedCollectingMetrics(FinishReason finish_reason);
 
-  // Whether an attempt was made to collect the "NonEmptyPaint" metric.
-  bool collected_paint_metric_;
-
-  // Whether an attempt was made to collect the "MainFrameLoad" metric.
-  bool collected_load_metric_;
-
-  // Whether an attempt was made to collect the "MainNavigationStart" metric.
-  bool collected_main_navigation_start_metric_;
-
-  // Whether an attempt was made to collect the "MainNavigationFinished" metric.
-  bool collected_main_navigation_finished_metric_;
+  // Whether an attempt was made to collect the "MainNavigationStart/Finished"
+  // metrics.
+  bool collected_main_navigation_metrics_;
 
   const startup_metric_utils::WebContentsWorkload workload_;
 
@@ -89,66 +73,38 @@ FirstWebContentsProfiler::FirstWebContentsProfiler(
     content::WebContents* web_contents,
     startup_metric_utils::WebContentsWorkload workload)
     : content::WebContentsObserver(web_contents),
-      collected_paint_metric_(false),
-      collected_load_metric_(false),
-      collected_main_navigation_start_metric_(false),
-      collected_main_navigation_finished_metric_(false),
+      collected_main_navigation_metrics_(false),
       workload_(workload) {}
 
 void FirstWebContentsProfiler::DidFirstVisuallyNonEmptyPaint() {
-  if (collected_paint_metric_)
-    return;
   if (startup_metric_utils::WasMainWindowStartupInterrupted()) {
     FinishedCollectingMetrics(FinishReason::ABANDON_BLOCKING_UI);
     return;
   }
 
-  collected_paint_metric_ = true;
   startup_metric_utils::RecordFirstWebContentsNonEmptyPaint(
       base::TimeTicks::Now(), web_contents()
                                   ->GetMainFrame()
                                   ->GetProcess()
                                   ->GetInitTimeForNavigationMetrics());
 
-  if (IsFinishedCollectingMetrics())
-    FinishedCollectingMetrics(FinishReason::DONE);
-}
-
-void FirstWebContentsProfiler::DocumentOnLoadCompletedInMainFrame() {
-  if (collected_load_metric_)
-    return;
-  if (startup_metric_utils::WasMainWindowStartupInterrupted()) {
-    FinishedCollectingMetrics(FinishReason::ABANDON_BLOCKING_UI);
-    return;
-  }
-
-  collected_load_metric_ = true;
-  startup_metric_utils::RecordFirstWebContentsMainFrameLoad(
-      base::TimeTicks::Now());
-
-  if (IsFinishedCollectingMetrics())
-    FinishedCollectingMetrics(FinishReason::DONE);
-}
-
-void FirstWebContentsProfiler::DidStartNavigation(
-    content::NavigationHandle* navigation_handle) {
-  if (collected_main_navigation_start_metric_)
-    return;
-  if (startup_metric_utils::WasMainWindowStartupInterrupted()) {
-    FinishedCollectingMetrics(FinishReason::ABANDON_BLOCKING_UI);
-    return;
-  }
+  FinishedCollectingMetrics(FinishReason::DONE);
 }
 
 void FirstWebContentsProfiler::DidFinishNavigation(
     content::NavigationHandle* navigation_handle) {
-  if (collected_main_navigation_finished_metric_) {
+  if (collected_main_navigation_metrics_) {
     // Abandon profiling on a top-level navigation to a different page as it:
     //   (1) is no longer a fair timing; and
     //   (2) can cause http://crbug.com/525209 where one of the timing
     //       heuristics (e.g. first paint) didn't fire for the initial content
     //       but fires after a lot of idle time when the user finally navigates
     //       to another page that does trigger it.
+    //
+    // TODO(https://crbug.com/1020549): Determine whether
+    // Startup.FirstWebContents.NonEmptyPaint2 can be recorded for a non-initial
+    // navigation if there are multiple DidStartNavigation() before the first
+    // paint.
     if (navigation_handle->IsInMainFrame() &&
         navigation_handle->HasCommitted() &&
         !navigation_handle->IsSameDocument()) {
@@ -173,11 +129,9 @@ void FirstWebContentsProfiler::DidFinishNavigation(
 
   startup_metric_utils::RecordFirstWebContentsMainNavigationStart(
       navigation_handle->NavigationStart(), workload_);
-  collected_main_navigation_start_metric_ = true;
-
-  collected_main_navigation_finished_metric_ = true;
   startup_metric_utils::RecordFirstWebContentsMainNavigationFinished(
       base::TimeTicks::Now());
+  collected_main_navigation_metrics_ = true;
 }
 
 void FirstWebContentsProfiler::OnVisibilityChanged(
@@ -193,23 +147,10 @@ void FirstWebContentsProfiler::WebContentsDestroyed() {
   FinishedCollectingMetrics(FinishReason::ABANDON_CONTENT_DESTROYED);
 }
 
-bool FirstWebContentsProfiler::IsFinishedCollectingMetrics() {
-  return collected_paint_metric_ && collected_load_metric_;
-}
-
 void FirstWebContentsProfiler::FinishedCollectingMetrics(
     FinishReason finish_reason) {
   UMA_HISTOGRAM_ENUMERATION("Startup.FirstWebContents.FinishReason",
                             finish_reason, FinishReason::ENUM_MAX);
-  if (!collected_paint_metric_) {
-    UMA_HISTOGRAM_ENUMERATION("Startup.FirstWebContents.FinishReason_NoPaint",
-                              finish_reason, FinishReason::ENUM_MAX);
-  }
-  if (!collected_load_metric_) {
-    UMA_HISTOGRAM_ENUMERATION("Startup.FirstWebContents.FinishReason_NoLoad",
-                              finish_reason, FinishReason::ENUM_MAX);
-  }
-
   delete this;
 }
 

diff --git a/chrome/browser/metrics/startup_metrics_browsertest.cc b/chrome/browser/metrics/startup_metrics_browsertest.cc
@@ -23,7 +23,6 @@ constexpr const char* kStartupMetrics[] = {
     "Startup.BrowserWindow.FirstPaint",
     "Startup.BrowserWindow.FirstPaint.CompositingEnded",
     "Startup.BrowserWindowDisplay",
-    "Startup.FirstWebContents.MainFrameLoad2",
     "Startup.FirstWebContents.MainNavigationFinished",
     "Startup.FirstWebContents.MainNavigationStart",
     "Startup.FirstWebContents.NonEmptyPaint2",

diff --git a/chrome/browser/ui/profile_error_browsertest.cc b/chrome/browser/ui/profile_error_browsertest.cc
@@ -83,7 +83,6 @@ class ProfileErrorBrowserTest : public InProcessBrowserTest,
 
 IN_PROC_BROWSER_TEST_P(ProfileErrorBrowserTest, MAYBE_CorruptedProfile) {
   const char kPaintHistogram[] = "Startup.FirstWebContents.NonEmptyPaint2";
-  const char kLoadHistogram[] = "Startup.FirstWebContents.MainFrameLoad2";
 
   // Navigate to a URL so the first non-empty paint is registered.
   ui_test_utils::NavigateToURL(browser(), GURL("http://www.example.com/"));
@@ -104,10 +103,8 @@ IN_PROC_BROWSER_TEST_P(ProfileErrorBrowserTest, MAYBE_CorruptedProfile) {
 
   if (do_corrupt_) {
     histogram_tester_.ExpectTotalCount(kPaintHistogram, 0);
-    histogram_tester_.ExpectTotalCount(kLoadHistogram, 0);
   } else {
     histogram_tester_.ExpectTotalCount(kPaintHistogram, 1);
-    histogram_tester_.ExpectTotalCount(kLoadHistogram, 1);
   }
 }
 

diff --git a/components/startup_metric_utils/browser/startup_metric_utils.cc b/components/startup_metric_utils/browser/startup_metric_utils.cc
@@ -494,19 +494,6 @@ void RecordRendererMainEntryTime(base::TimeTicks ticks) {
     g_renderer_main_entry_point_ticks = ticks;
 }
 
-void RecordFirstWebContentsMainFrameLoad(base::TimeTicks ticks) {
-  static bool is_first_call = true;
-  if (!is_first_call || ticks.is_null())
-    return;
-  is_first_call = false;
-  if (!ShouldLogStartupHistogram())
-    return;
-
-  UMA_HISTOGRAM_AND_TRACE_WITH_TEMPERATURE(
-      UMA_HISTOGRAM_LONG_TIMES_100, "Startup.FirstWebContents.MainFrameLoad2",
-      g_process_creation_ticks, ticks);
-}
-
 void RecordFirstWebContentsNonEmptyPaint(
     base::TimeTicks now,
     base::TimeTicks render_process_host_init_time) {

diff --git a/components/startup_metric_utils/browser/startup_metric_utils.h b/components/startup_metric_utils/browser/startup_metric_utils.h
@@ -70,10 +70,6 @@ void RecordBrowserOpenTabsDelta(base::TimeDelta delta);
 // function are ignored.
 void RecordRendererMainEntryTime(base::TimeTicks ticks);
 
-// Call this with the time when the first web contents loaded its main frame,
-// only if the first web contents was unimpended in its attempt to do so.
-void RecordFirstWebContentsMainFrameLoad(base::TimeTicks ticks);
-
 // Call this with the time when the first web contents had a non-empty paint,
 // only if the first web contents was unimpeded in its attempt to do so.
 void RecordFirstWebContentsNonEmptyPaint(

diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml
@@ -142099,6 +142099,10 @@ should be kept until we use this API.  -->
 
 <histogram name="Startup.FirstWebContents.MainFrameLoad2" units="ms"
     expires_after="M77">
+  <obsolete>
+    Deprecated with M77. Startup.FirstWebContents.NonEmptyPaint2 is a better
+    indicator of startup time as perceived by users.
+  </obsolete>
   <owner>fdoray@chromium.org</owner>
   <owner>gab@chromium.org</owner>
   <summary>
@@ -180941,6 +180945,12 @@ regressions. -->
 </histogram_suffixes>
 
 <histogram_suffixes name="StartupProfilingAbandonState" separator="_">
+  <obsolete>
+    Deprecated 10/2019. Since Startup.FirstWebContents.MainFrameLoad2 is no
+    longer tracked, the non-suffixed reason is always for
+    Startup.FirstWebContents.NonEmptyPaint2 and the suffix does not provide
+    additional information.
+  </obsolete>
   <suffix name="NoLoad" label="Abandoned before first main frame load."/>
   <suffix name="NoPaint" label="Abandoned before first paint."/>
   <affected-histogram name="Startup.FirstWebContents.FinishReason"/>