PageLoadMetricsObservers observe individual page loads

components/page_load_metrics/browser/page_load_metrics_observer.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/page_load_metrics/browser/page_load_metrics_observer.h"
 
 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