PageLoadMetricsObservers observe individual page loads

components/page_load_metrics/browser/page_load_metrics_observer.cc

Index: components/page_load_metrics/browser/page_load_metrics_observer.cc
diff --git a/components/page_load_metrics/browser/page_load_metrics_observer.cc b/components/page_load_metrics/browser/page_load_metrics_observer.cc
index 05b37303a1be7fa1162a09d31ccbe41b16cf60aa..9f9ecf83ea3c9bc90bb3c0b55b948197bda1dd24 100644
--- a/components/page_load_metrics/browser/page_load_metrics_observer.cc
+++ b/components/page_load_metrics/browser/page_load_metrics_observer.cc
@@ -9,9 +9,27 @@ namespace page_load_metrics {
 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)
     : first_background_time(first_background_time),
       first_foreground_time(first_foreground_time),
-      started_in_foreground(started_in_foreground) {}
+      started_in_foreground(started_in_foreground),
+      has_commit(has_commit) {}
+
+PageLoadMetricsObserver::PageLoadMetricsObserver(
+    PageLoadMetricsObservable* observable)
+    : observable_(observable) {
+  observable_->AddObserver(this);
+}
+
+void PageLoadMetricsObserver::OnPageLoadMetricsGoingAway() {
+  observable_->RemoveObserver(this);
+  delete this;

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

I'm concerned about the confusion in ownership implied here.  This object is, in
some sense, self-owned; it deletes itself when the observable signals that it is
going away.  Except it's not; sometimes the derived classes destroy it in some
other context.  So is the object "self-owned" by the base class or the derived
class?  I think there's a reasonably good chance that future hacking in this
code would trip over that confusion.

I'd suggest instead making the destructor private (make it clear that it must be
destroyed by a method of the class) and create a "Destroy()" method that does
the above work, which is called from this method and may be called from derived
classes.  That would also allow nuking "GetObservable()", which feels like a
weird thing anyway.

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

Your suggestion sgtm, I might add a StopObserving() method for consumers that
don't share a lifetime with the observable. However, I think it's forbidden to
subclass a class with a private destructor, so I'm making it protected.

+}
+
+void PageLoadMetricsObserver::Observe(PageLoadMetricsObservable* observable) {
+  observable_ = observable;
+  observable->AddObserver(this);
+}
 
 }  // namespace page_load_metrics