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..4a87922473619c7f7eab071175436e17dc2a5f3a 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,29 @@ 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 {

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

I'd like a comment about ownership, given that it's a subtle ownership model
(I'd call it self-owned but tied to the PageLoadMetricsObservable lifetime. 
Alternatively, you could say that it's owned by the PageLoadMetricsObserveable,
but in a slightly funky way that doesn't involve a scoped_ptr<> anywhere. 
Either way, it should be commented (unless I argue you into a different,
simpler, ownership model; I'm not yet sure if I'm going to try :-}).

[Charlie Harrison, 2015/12/02 01:30:28]

Done.

  public:
-  virtual ~PageLoadMetricsObserver() {}
+  // This constructor should be used if the derived class will not start
+  // observing a PageLoadMetricsObservable at creation time.
+  PageLoadMetricsObserver();
+
+  // This constructor will add the object as an observer to the passed in
+  // observable.
+  explicit PageLoadMetricsObserver(PageLoadMetricsObservable* observable);
+
+  // 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 +63,38 @@ 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
+  // 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 PageLoadMetricsObservable we are observing is
+  // tearing down. No further callbacks will be triggered. If the observer has
+  // longer lifetime than the observable, it should override this method and
+  // call RemoveObserver() at this point.
+  virtual void OnPageLoadMetricsGoingAway();
+
+  // Derived objects should use this method to destroy themselves. This will
+  // properly remove the observer from the observable.
+  void Destroy();
+
+  // Call these methods start and stop observing if the derived class does not
+  // share a lifetime with the PageLoadMetricsObservable.
+  void Observe(PageLoadMetricsObservable* observable);
+  void StopObserving();
+
+ protected:
+  virtual ~PageLoadMetricsObserver() {}
+
+ 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() {}