Expand PageLoadMetricsObserverDelegate API to cover PageLoadExtraInfo

This allows observers to access page load info from the metrics
observer delegate directly, instead of being passed the
PageLoadExtraInfo struct by the delegate. Previously the entire
PageLoadExtraInfo struct was created and passed independent of the
data needed by the observers. This also removes the duplication of
delegate and PageLoadExtraInfo fields (navigation_start, did commit).

Each API function returns the same data as used to populate the
PageLoadExtraInfo struct.

This API will be followed up with the following sets of CLs:
1. Replace common observer utility functions to use the delegate
   returned values instead of the PageLoadExtraInfo struct.
2. Group page_load_metrics_observer On* functions that use
   PageLoadExtraInfo structs and replace them with delegate returned
   values.

This also adds first_background_time and first_foreground_time as
members of the page_load_tracker, so as to not compute these with
each call to their respective getter function.

BUG=939403

Change-Id: I84af9f19fc2d14a727318ea2eb0668229f795cc3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1713153
Commit-Queue: Justin Miron <justinmiron@google.com>
Reviewed-by: Bryan McQuade <bmcquade@chromium.org>
Reviewed-by: John Delaney <johnidel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#681813}

chrome/browser/page_load_metrics/page_load_metrics_observer.h

@@ -29,52 +29,6 @@ class RenderFrameHost;
 
 namespace page_load_metrics {
 
-// This enum represents how a page load ends. If the action occurs before the
-// page load finishes (or reaches some point like first paint), then we consider
-// the load to be aborted.
-//
-// These values are persisted to logs. Entries should not be renumbered and
-// numeric values should never be reused. For any additions, also update the
-// corresponding PageEndReason enum in enums.xml.
-enum PageEndReason {
-  // Page lifetime has not yet ended (page is still active).
-  END_NONE = 0,
-
-  // The page was reloaded, possibly by the user.
-  END_RELOAD = 1,
-
-  // The page was navigated away from, via a back or forward navigation.
-  END_FORWARD_BACK = 2,
-
-  // The navigation is replaced with a navigation with the qualifier
-  // ui::PAGE_TRANSITION_CLIENT_REDIRECT, which is caused by Javascript, or the
-  // meta refresh tag.
-  END_CLIENT_REDIRECT = 3,
-
-  // If the page load is replaced by a new navigation. This includes link
-  // clicks, typing in the omnibox (not a reload), and form submissions.
-  END_NEW_NAVIGATION = 4,
-
-  // The page load was stopped (e.g. the user presses the stop X button).
-  END_STOP = 5,
-
-  // Page load ended due to closing the tab or browser.
-  END_CLOSE = 6,
-
-  // The provisional load for this page load failed before committing.
-  END_PROVISIONAL_LOAD_FAILED = 7,
-
-  // The render process hosting the page terminated unexpectedly.
-  END_RENDER_PROCESS_GONE = 8,
-
-  // We don't know why the page load ended. This is the value we assign to a
-  // terminated provisional load if the only signal we get is the load finished
-  // without committing, either without error or with net::ERR_ABORTED.
-  END_OTHER = 9,
-
-  PAGE_END_REASON_COUNT
-};
-
 // Information related to failed provisional loads.
 struct FailedProvisionalLoadInfo {
   FailedProvisionalLoadInfo(base::TimeDelta interval, net::Error error);
@@ -141,6 +95,9 @@ struct PageRenderData {
   float layout_shift_score_before_input_or_scroll;
 };
 
+// TODO(crbug/939403): Migrate usage of this struct in favor of observer
+// delegate. New user's of this data should use
+// PageLoadMetricsObserverDelegate's API instead.
 struct PageLoadExtraInfo {
   PageLoadExtraInfo(
       base::TimeTicks navigation_start,

chrome/browser/page_load_metrics/page_load_metrics_observer_delegate.h

@@ -9,6 +9,7 @@
 #include "base/time/time.h"
 #include "chrome/browser/page_load_metrics/resource_tracker.h"
 #include "chrome/browser/scoped_visibility_tracker.h"
+#include "chrome/common/page_load_metrics/page_end_reason.h"
 #include "services/metrics/public/cpp/ukm_source_id.h"
 
 namespace content {
@@ -17,19 +18,38 @@ class WebContents;
 
 namespace page_load_metrics {
 
+namespace mojom {
+class PageLoadMetadata;
+}  // namespace mojom
+
+struct UserInitiatedInfo;
+struct PageRenderData;
+
 // This class tracks global state for the page load that should be accessible
 // from any PageLoadMetricsObserver.
 class PageLoadMetricsObserverDelegate {
  public:
   virtual content::WebContents* GetWebContents() const = 0;
   virtual base::TimeTicks GetNavigationStart() const = 0;
+  virtual const base::Optional<base::TimeDelta>& GetFirstBackgroundTime()
+      const = 0;
+  virtual const base::Optional<base::TimeDelta>& GetFirstForegroundTime()
+      const = 0;
+  virtual bool StartedInForeground() const = 0;
+  virtual const UserInitiatedInfo& GetUserInitiatedInfo() const = 0;
+  virtual const GURL& GetUrl() const = 0;
+  virtual const GURL& GetStartUrl() const = 0;
   virtual bool DidCommit() const = 0;
+  virtual PageEndReason GetPageEndReason() const = 0;
+  virtual const UserInitiatedInfo& GetPageEndUserInitiatedInfo() const = 0;
+  virtual base::Optional<base::TimeDelta> GetPageEndTime() const = 0;
+  virtual const mojom::PageLoadMetadata& GetMainFrameMetadata() const = 0;
+  virtual const mojom::PageLoadMetadata& GetSubframeMetadata() const = 0;
+  virtual const PageRenderData& GetPageRenderData() const = 0;
+  virtual const PageRenderData& GetMainFrameRenderData() const = 0;
   virtual const ScopedVisibilityTracker& GetVisibilityTracker() const = 0;
   virtual const ResourceTracker& GetResourceTracker() const = 0;
   virtual ukm::SourceId GetSourceId() const = 0;
-
-  // TODO(crbug/939403): Consider migrating PageLoadExtraInfo data to this API
-  // and deprecating that struct.
 };
 
 }  // namespace page_load_metrics

chrome/browser/page_load_metrics/page_load_tracker.cc

@@ -302,13 +302,16 @@ void PageLoadTracker::LogAbortChainHistograms(
 
 void PageLoadTracker::WebContentsHidden() {
   // Only log the first time we background in a given page load.
-  if (background_time_.is_null()) {
+  if (!first_background_time_.has_value()) {
     // Make sure we either started in the foreground and haven't been
     // foregrounded yet, or started in the background and have already been
     // foregrounded.
-    DCHECK_EQ(started_in_foreground_, foreground_time_.is_null());
-    background_time_ = base::TimeTicks::Now();
-    ClampBrowserTimestampIfInterProcessTimeTickSkew(&background_time_);
+    base::TimeTicks background_time;
+    DCHECK_EQ(started_in_foreground_, !first_foreground_time_.has_value());
+    background_time = base::TimeTicks::Now();
+    ClampBrowserTimestampIfInterProcessTimeTickSkew(&background_time);
+    DCHECK_GE(background_time, navigation_start_);
+    first_background_time_ = background_time - navigation_start_;
   }
   visibility_tracker_.OnHidden();
   const PageLoadExtraInfo info = ComputePageLoadExtraInfo();
@@ -318,13 +321,16 @@ void PageLoadTracker::WebContentsHidden() {
 
 void PageLoadTracker::WebContentsShown() {
   // Only log the first time we foreground in a given page load.
-  if (foreground_time_.is_null()) {
+  if (!first_foreground_time_.has_value()) {
     // Make sure we either started in the background and haven't been
     // backgrounded yet, or started in the foreground and have already been
     // backgrounded.
-    DCHECK_NE(started_in_foreground_, background_time_.is_null());
-    foreground_time_ = base::TimeTicks::Now();
-    ClampBrowserTimestampIfInterProcessTimeTickSkew(&foreground_time_);
+    base::TimeTicks foreground_time;
+    DCHECK_NE(started_in_foreground_, !first_background_time_.has_value());
+    foreground_time = base::TimeTicks::Now();
+    ClampBrowserTimestampIfInterProcessTimeTickSkew(&foreground_time);
+    DCHECK_GE(foreground_time, navigation_start_);
+    first_foreground_time_ = foreground_time - navigation_start_;
   }
 
   visibility_tracker_.OnShown();
@@ -524,20 +530,8 @@ void PageLoadTracker::ClampBrowserTimestampIfInterProcessTimeTickSkew(
 }
 
 PageLoadExtraInfo PageLoadTracker::ComputePageLoadExtraInfo() const {
-  base::Optional<base::TimeDelta> first_background_time;
-  base::Optional<base::TimeDelta> first_foreground_time;
   base::Optional<base::TimeDelta> page_end_time;
 
-  if (!background_time_.is_null()) {
-    DCHECK_GE(background_time_, navigation_start_);
-    first_background_time = background_time_ - navigation_start_;
-  }
-
-  if (!foreground_time_.is_null()) {
-    DCHECK_GE(foreground_time_, navigation_start_);
-    first_foreground_time = foreground_time_ - navigation_start_;
-  }
-
   if (page_end_reason_ != END_NONE) {
     DCHECK_GE(page_end_time_, navigation_start_);
     page_end_time = page_end_time_ - navigation_start_;
@@ -551,7 +545,7 @@ PageLoadExtraInfo PageLoadTracker::ComputePageLoadExtraInfo() const {
          (!page_end_user_initiated_info_.browser_initiated &&
           !page_end_user_initiated_info_.user_gesture));
   return PageLoadExtraInfo(
-      navigation_start_, first_background_time, first_foreground_time,
+      navigation_start_, first_background_time_, first_foreground_time_,
       started_in_foreground_, user_initiated_info_, url(), start_url_,
       did_commit_, page_end_reason_, page_end_user_initiated_info_,
       page_end_time, metrics_update_dispatcher_.main_frame_metadata(),
@@ -758,10 +752,73 @@ base::TimeTicks PageLoadTracker::GetNavigationStart() const {
   return navigation_start_;
 }
 
+const base::Optional<base::TimeDelta>& PageLoadTracker::GetFirstBackgroundTime()
+    const {
+  return first_background_time_;
+}
+
+const base::Optional<base::TimeDelta>& PageLoadTracker::GetFirstForegroundTime()
+    const {
+  return first_foreground_time_;
+}
+
+bool PageLoadTracker::StartedInForeground() const {
+  return started_in_foreground_;
+}
+
+const UserInitiatedInfo& PageLoadTracker::GetUserInitiatedInfo() const {
+  return user_initiated_info_;
+}
+
+const GURL& PageLoadTracker::GetUrl() const {
+  return url();
+}
+
+const GURL& PageLoadTracker::GetStartUrl() const {
+  return start_url_;
+}
+
 bool PageLoadTracker::DidCommit() const {
   return did_commit_;
 }
 
+PageEndReason PageLoadTracker::GetPageEndReason() const {
+  return page_end_reason_;
+}
+
+const UserInitiatedInfo& PageLoadTracker::GetPageEndUserInitiatedInfo() const {
+  return page_end_user_initiated_info_;
+}
+
+base::Optional<base::TimeDelta> PageLoadTracker::GetPageEndTime() const {
+  base::Optional<base::TimeDelta> page_end_time;
+
+  if (page_end_reason_ != END_NONE) {
+    DCHECK_GE(page_end_time_, navigation_start_);
+    page_end_time = page_end_time_ - navigation_start_;
+  } else {
+    DCHECK(page_end_time_.is_null());
+  }
+
+  return page_end_time;
+}
+
+const mojom::PageLoadMetadata& PageLoadTracker::GetMainFrameMetadata() const {
+  return metrics_update_dispatcher_.main_frame_metadata();
+}
+
+const mojom::PageLoadMetadata& PageLoadTracker::GetSubframeMetadata() const {
+  return metrics_update_dispatcher_.subframe_metadata();
+}
+
+const PageRenderData& PageLoadTracker::GetPageRenderData() const {
+  return metrics_update_dispatcher_.page_render_data();
+}
+
+const PageRenderData& PageLoadTracker::GetMainFrameRenderData() const {
+  return metrics_update_dispatcher_.main_frame_render_data();
+}
+
 const ScopedVisibilityTracker& PageLoadTracker::GetVisibilityTracker() const {
   return visibility_tracker_;
 }

chrome/browser/page_load_metrics/page_load_tracker.h

@@ -16,6 +16,7 @@
 #include "chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.h"
 #include "chrome/browser/page_load_metrics/resource_tracker.h"
 #include "chrome/browser/scoped_visibility_tracker.h"
+#include "chrome/common/page_load_metrics/page_end_reason.h"
 #include "chrome/common/page_load_metrics/page_load_timing.h"
 #include "content/public/browser/global_request_id.h"
 #include "content/public/browser/web_contents_observer.h"
@@ -203,7 +204,22 @@ class PageLoadTracker : public PageLoadMetricsUpdateDispatcher::Client,
   // PageLoadMetricsDelegate implementation:
   content::WebContents* GetWebContents() const override;
   base::TimeTicks GetNavigationStart() const override;
+  const base::Optional<base::TimeDelta>& GetFirstBackgroundTime()
+      const override;
+  const base::Optional<base::TimeDelta>& GetFirstForegroundTime()
+      const override;
+  bool StartedInForeground() const override;
+  const UserInitiatedInfo& GetUserInitiatedInfo() const override;
+  const GURL& GetUrl() const override;
+  const GURL& GetStartUrl() const override;
   bool DidCommit() const override;
+  PageEndReason GetPageEndReason() const override;
+  const UserInitiatedInfo& GetPageEndUserInitiatedInfo() const override;
+  base::Optional<base::TimeDelta> GetPageEndTime() const override;
+  const mojom::PageLoadMetadata& GetMainFrameMetadata() const override;
+  const mojom::PageLoadMetadata& GetSubframeMetadata() const override;
+  const PageRenderData& GetPageRenderData() const override;
+  const PageRenderData& GetMainFrameRenderData() const override;
   const ScopedVisibilityTracker& GetVisibilityTracker() const override;
   const ResourceTracker& GetResourceTracker() const override;
   ukm::SourceId GetSourceId() const override;
@@ -394,8 +410,8 @@ class PageLoadTracker : public PageLoadMetricsUpdateDispatcher::Client,
   // We record separate metrics for events that occur after a background,
   // because metrics like layout/paint are delayed artificially
   // when they occur in the background.
-  base::TimeTicks background_time_;
-  base::TimeTicks foreground_time_;
+  base::Optional<base::TimeDelta> first_background_time_;
+  base::Optional<base::TimeDelta> first_foreground_time_;
   bool started_in_foreground_;
 
   mojom::PageLoadTimingPtr last_dispatched_merged_page_timing_;

chrome/common/BUILD.gn

@@ -158,6 +158,7 @@ static_library("common") {
     "multi_process_lock_win.cc",
     "origin_trials/chrome_origin_trial_policy.cc",
     "origin_trials/chrome_origin_trial_policy.h",
+    "page_load_metrics/page_end_reason.h",
     "page_load_metrics/page_load_metrics_constants.h",
     "page_load_metrics/page_load_metrics_util.cc",
     "page_load_metrics/page_load_metrics_util.h",

chrome/common/page_load_metrics/page_end_reason.h

+// Copyright 2019 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.
+
+#ifndef CHROME_COMMON_PAGE_LOAD_METRICS_PAGE_END_REASON_H_
+#define CHROME_COMMON_PAGE_LOAD_METRICS_PAGE_END_REASON_H_
+
+namespace page_load_metrics {
+
+// This enum represents how a page load ends. If the action occurs before the
+// page load finishes (or reaches some point like first paint), then we consider
+// the load to be aborted.
+//
+// These values are persisted to logs. Entries should not be renumbered and
+// numeric values should never be reused. For any additions, also update the
+// corresponding PageEndReason enum in enums.xml.
+enum PageEndReason {
+  // Page lifetime has not yet ended (page is still active).
+  END_NONE = 0,
+
+  // The page was reloaded, possibly by the user.
+  END_RELOAD = 1,
+
+  // The page was navigated away from, via a back or forward navigation.
+  END_FORWARD_BACK = 2,
+
+  // The navigation is replaced with a navigation with the qualifier
+  // ui::PAGE_TRANSITION_CLIENT_REDIRECT, which is caused by Javascript, or the
+  // meta refresh tag.
+  END_CLIENT_REDIRECT = 3,
+
+  // If the page load is replaced by a new navigation. This includes link
+  // clicks, typing in the omnibox (not a reload), and form submissions.
+  END_NEW_NAVIGATION = 4,
+
+  // The page load was stopped (e.g. the user presses the stop X button).
+  END_STOP = 5,
+
+  // Page load ended due to closing the tab or browser.
+  END_CLOSE = 6,
+
+  // The provisional load for this page load failed before committing.
+  END_PROVISIONAL_LOAD_FAILED = 7,
+
+  // The render process hosting the page terminated unexpectedly.
+  END_RENDER_PROCESS_GONE = 8,
+
+  // We don't know why the page load ended. This is the value we assign to a
+  // terminated provisional load if the only signal we get is the load finished
+  // without committing, either without error or with net::ERR_ABORTED.
+  END_OTHER = 9,
+
+  PAGE_END_REASON_COUNT
+};
+
+}  // namespace page_load_metrics
+
+#endif  // CHROME_COMMON_PAGE_LOAD_METRICS_PAGE_END_REASON_H_