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..659de29fa591b51fb60dca8e1e3e80fca60dd08b 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,40 @@ 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() : observable_(nullptr) {}
+
+PageLoadMetricsObserver::PageLoadMetricsObserver(
+    PageLoadMetricsObservable* observable)
+    : observable_(observable) {
+  observable_->AddObserver(this);
+}
+
+void PageLoadMetricsObserver::OnPageLoadMetricsGoingAway() {
+  Destroy();
+}
+
+void PageLoadMetricsObserver::Destroy() {
+  if (observable_)

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

what happens if someone calls MWCO::RemoveObserver() directly, for a PLMObserver
whose lifetime is scoped to the MWCO? In that case they'd also need to
explicitly delete the PLMObserver, but I could imagine it being easy to forget
to do that, which leads to a memory leak. It seems unfortunate that callers have
to know to call Destroy() and that it is a subtle runtime error to call
RemoveObserver() directly.

It seems like MWCO::RemoveObserver() is the real signal to indicate that a
PLMObserver is no longer associated with the MWCO, and for PLMObservers
associated with a single PLMObservable, invoking RemoveObserver should result in
deleting of the PLMObserver rather than having to call the alternative method
Destroy(). Alternatively, if a caller must call Destroy(), they should not be
allowed to call RemoveObserver() by mistake (but I'm not sure how to enforce
that).

Rethinking this slightly, perhaps these PLMObservers associated with a single
PLMObservable should be 'owned' by the PLMObservable. Then, we could have two
add observer mechanisms on PLMObservable:

void AddObserver(PLMObserver* observer);
void AddObserverAndTakeOwnership(scoped_ptr<PLMObserver> observer);

When RemoveObserver() is called for a PLMObserver previously passed to
AddObserverAndTakeOwnership, the MWCO would be responsible for deleting the
owned PLMObserver at some reasonable time after RemoveObserver was invoked, such
as via a call to base::MessageLoop::current()->DeleteSoon(FROM_HERE, observer);,
or, if RemoveObserver is never called, in the MWCO destructor.

PageLoadMetricsEmbedderInterfaceImpl::RegisterObservers() becomes something
like:
metrics->AddObserverAndTakeOwnership(make_scoped_ptr(new
FromGWSPageLoadMetricsObserver()));
...


You could take this a step further and make the protected convenience method
PLMObserver::StopObserving() just call observable_->RemoveObserver(this), to
continue to allow PLMObservers to easily stop observing without having to know
about their associated PLMObservable.

One small challenge is it's unclear how to tell the PLMObserver what its
observable is, for the purpose of supporting StopObserver(). I could imagine
having a PLMObserver::SetOwner(PLMObservable*) method, but it seems very odd to
make that public, and the only class allowed to call it is PLMObservable, as
part of the AddObserverAndTakeOwnership method. Given the intertwined
relationship between PLMObserver and PLMObservable I do see this as a place
where making PLMObservable a friend of PLMObserver (and making SetOwner private)
seems reasonable, though if there is some way to accomplish these goals without
friending, that's always preferrable.

All this said, there are clearly some shortcomings with this approach
(supporting StopObserving cleanly is one) but I think it's worth spending some
time to consider whether moving to a model where the MWCO 'owns' PLMObservables
that are scoped to the MWCO's lifetime could work. I think that more clearly
documents the relationship/lifetimes between the 2 classes.

Randy, given your earlier comments about least astonishment, lifetime,
ownership, etc, I'm also interested to hear if you think this helps. In general,
I feel that calling 'delete this;' is a hint that there might be an opportunity
to better model object ownership.

[Charlie Harrison, 2015/11/30 22:22:02]

I really like having the Observable own the observers. In fact, my original
implementation used a ScopedVector to track the observers. I'd be happy to go
back to that for the simple case. You don't even need
AddObserverAndTakeOwnership, just another AddObserver method that takes a scoped
pointer. The fact that it takes a scoped pointer indicates it will take
ownership. I don't think it's unreasonable to also make owned observers take the
Observable in their constructor, so we don't need a SetOwner method.

RE: Remove/StopObserving. The underlying problem is keeping track of the
observable_ pointer. I think we should just document that classes have to call
StopObserving when they want to stop observing. If an owned observer calls
StopObserving, it will be deleted.

I think it's reasonable given that the observable is a private member, and it is
not exposed anywhere that you shouldn't directly call RemoveObserver().

+    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