PageLoadMetricsObservers observe individual page loads

components/page_load_metrics/browser/page_load_metrics_observer.h

Index: components/page_load_metrics/browser/page_load_metrics_observer.h
diff --git a/components/page_load_metrics/browser/page_load_metrics_observer.h b/components/page_load_metrics/browser/page_load_metrics_observer.h
index 08a35dddc2fc226a29ea57a2e5b701983cda9031..6088a92bf13b26a7bccf239b03846c3aa74ddb58 100644
--- a/components/page_load_metrics/browser/page_load_metrics_observer.h
+++ b/components/page_load_metrics/browser/page_load_metrics_observer.h
@@ -5,15 +5,19 @@
 #ifndef COMPONENTS_PAGE_LOAD_METRICS_BROWSER_PAGE_LOAD_METRICS_OBSERVER_H_
 #define COMPONENTS_PAGE_LOAD_METRICS_BROWSER_PAGE_LOAD_METRICS_OBSERVER_H_
 
+#include "base/macros.h"
 #include "components/page_load_metrics/common/page_load_timing.h"
 #include "content/public/browser/navigation_handle.h"
 
 namespace page_load_metrics {
 
+class PageLoadMetricsObservable;
+
 struct PageLoadExtraInfo {
   PageLoadExtraInfo(const base::TimeDelta& first_background_time,
                     const base::TimeDelta& first_foreground_time,
-                    bool started_in_foreground);
+                    bool started_in_foreground,
+                    bool has_commit);
 
   // Returns the time to first background if the page load started in the
   // foreground. If the page has not been backgrounded, or the page started in
@@ -27,22 +31,27 @@ struct PageLoadExtraInfo {
 
   // True if the page load started in the foreground.
   const bool started_in_foreground;
+
+  // True if the page load committed and received its first bytes of data.
+  const bool has_commit;
 };
 
-// Interface for PageLoadMetrics observers. Note that it might be possible for
-// OnCommit to be fired without a subsequent OnComplete (i.e. if an error
-// occurs or we don't have any metrics to log). PageLoadMetricsObservers are
-// required to stop observing (call PageLoadMetricsObservable::RemoveObserver)
-// some time before the OnPageLoadMetricsGoingAway trigger finishes. If this
-// class is destroyed before the PageLoadMetricsObservable, it should call
-// RemoveObserver in its destructor.
+// Interface for PageLoadMetrics observers.
 class PageLoadMetricsObserver {
  public:
+  // This constructor will add the object as an observable to the passed in

[Randy Smith (Not in Mondays), 2015/11/28 22:03:14]

nit: "observable" -> "observer"?

[Charlie Harrison, 2015/11/30 16:39:29]

Done.

+  // observable.
+  explicit PageLoadMetricsObserver(PageLoadMetricsObservable* observable);
+
   virtual ~PageLoadMetricsObserver() {}
 
+  // The page load started, with the given navigation handle.
+  virtual void OnStart(content::NavigationHandle* navigation_handle) {}
+
   // OnRedirect is triggered when a page load redirects to another URL.
   // The navigation handle holds relevant data for the navigation, but will
-  // be destroyed soon after this call. Don't hold a reference to it.
+  // be destroyed soon after this call. Don't hold a reference to it. This can
+  // be called multiple times.
   virtual void OnRedirect(content::NavigationHandle* navigation_handle) {}
 
   // OnCommit is triggered when a page load commits, i.e. when we receive the
@@ -52,21 +61,31 @@ class PageLoadMetricsObserver {
   virtual void OnCommit(content::NavigationHandle* navigation_handle) {}
 
   // OnComplete is triggered when we are ready to record metrics for this page
-  // load. This will happen some time after commit, and will be triggered as
-  // long as we've received some data about the page load. The
-  // PageLoadTiming struct contains timing data and the PageLoadExtraInfo struct
-  // contains other useful information about the tab backgrounding/foregrounding
-  // over the course of the page load.
+  // load. This will happen some time after commit, the PageLoadTiming struct

[Randy Smith (Not in Mondays), 2015/11/28 22:03:14]

nit: I think this is better as two separate sentences (i.e. "," -> '." :-}).

[Charlie Harrison, 2015/11/30 16:39:29]

Done.

+  // contains timing data and the PageLoadExtraInfo struct contains other useful
+  // data collected over the course of the page load. If the load did not
+  // receive any timing information, |timing.IsEmpty()| will be true.
+  // OnPageLoadMetricsGoingAway will be called after OnComplete.
   virtual void OnComplete(const PageLoadTiming& timing,
                           const PageLoadExtraInfo& extra_info) {}
 
-  // This is called when the WebContents we are observing is tearing down. No
-  // further callbacks will be triggered.
-  virtual void OnPageLoadMetricsGoingAway() {}
+  // This is called when the metrics we are observing is tearing down. No

[Randy Smith (Not in Mondays), 2015/11/28 22:03:14]

nit: "the metrics ... is" reads funny in English.  Maybe substitute "metrics"
with "<classname> object"?

[Charlie Harrison, 2015/11/30 16:39:29]

Done.

+  // further callbacks will be triggered. If your object has longer lifetime

[Randy Smith (Not in Mondays), 2015/11/28 22:03:14]

nit: "Your" is problematic for the same reason as "we".  Maybe "If the observer
has longer ..."?

[Charlie Harrison, 2015/11/30 16:39:29]

Done.

+  // than the observable, override this method and call RemoveObserver() by this

[Randy Smith (Not in Mondays), 2015/11/28 22:03:14]

nit: "*it should* override this method and call RemoveObserver() *at* this
point."  (It's perfectly reasonable to call RemoveObserver before this point,
but in that case the class isn't overriding this method.)

[Charlie Harrison, 2015/11/30 16:39:29]

Done.

+  // point.
+  virtual void OnPageLoadMetricsGoingAway();
+
+  // Call this if the object will start observing after its construction time.
+  void Observe(PageLoadMetricsObservable* observable);

[Randy Smith (Not in Mondays), 2015/11/28 22:03:14]

I don't see this ever being called; why does it exist?  

[Charlie Harrison, 2015/11/30 16:39:29]

This exists for consumers who want to observe without sharing a lifetime with
the observable. So far none of the observers actually need this. I know there
are some places in the codebase that are against API changes with no consumers,
so I can remove this if you think it's unnecessary. 

[Randy Smith (Not in Mondays), 2015/12/01 22:34:03]

Yeah, if you would.   I'm afraid I'm one of those places in the codebase :-}.

+
+  PageLoadMetricsObservable* GetObservable() { return observable_; }
+
+ private:
+  PageLoadMetricsObservable* observable_;
+  DISALLOW_COPY_AND_ASSIGN(PageLoadMetricsObserver);
 };
 
-// Class which handles notifying observers when loads commit and complete. It
-// must call OnPageLoadMetricsGoingAway in the destructor.
+// Class which handles notifying observers when loads commit and complete.
 class PageLoadMetricsObservable {
  public:
   virtual ~PageLoadMetricsObservable() {}