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 {
  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);

[Bryan McQuade, 2015/11/30 19:18:48]

I know WC exposes a similar method, but do we need a public Observe() method?
I'm inclined to support 2 use cases:
1. simple: PLMObserver observes a single PLMObservable, which is specified in
its constructor. PLMObserver's lifetime is scoped to the PLMObservable
2. more flexible: PLMObserver can observe any number of PLMObservables. Some
external entity is responsible for managing the PLMObserver's lifetime.

What do you think about getting rid of Observe(), or making it private if you
see it as a useful helper for just this class to use?

All this said, if we do something similar to my other comment, then I think this
method can go away, since AddObserver* are the only mechanisms needed to
observe.

+  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() {}