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..bd1931c54d2726b57a18da473b11ad7df586e041 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,38 @@ 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);

[Bryan McQuade, 2015/11/30 17:00:53]

I like having the default case be that a PLMObserver observes a single
PLMObservable, with lifetime of the observer bound to the observable.

Are we intending to support the use case where a single PLMObserver can observe
multiple PLMObservables? Does Tarun's use case require that? If so, it seems
that we should allow a caller to pass nullptr here, since the PLMObserver may be
observing many PLMObservables concurrently (in cases where there are multiple
page loads happening in parallel, e.g. a provisional and committed load in a
single tab, or two page loads in different tabs).

Overall, if we can require that each PLMObserver observes a single
PLMObservable, and the PLMObserver's lifetime is scoped to the PLMObservable, I
think that makes things simpler and easier to reason about, so I prefer it. But
I want to make sure we can support all required use cases if we go that route.

[Charlie Harrison, 2015/11/30 17:49:39]

Yeah a PLMObserver can observe multiple PLMObservables. In those cases, I guess
I was assuming that the derived classes would not call this constructor, but one
with no arguments. I'll add that with an additional comment.

+}
+
+void PageLoadMetricsObserver::OnPageLoadMetricsGoingAway() {
+  Destroy();
+}
+
+void PageLoadMetricsObserver::Destroy() {
+  if (observable_)
+    observable_->RemoveObserver(this);
+  delete this;
+}
+
+void PageLoadMetricsObserver::Observe(PageLoadMetricsObservable* observable) {
+  observable_ = observable;
+  observable->AddObserver(this);
+}
+
+void PageLoadMetricsObserver::StopObserving() {
+  DCHECK(observable_);
+  observable_->RemoveObserver(this);
+  observable_ = nullptr;
+}
 
 }  // namespace page_load_metrics