[subresource_filter] Use the SubresourceFilterObserver for PageLoadMetrics

chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc

Index: chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc
diff --git a/chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc b/chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc
index 1ee8fcaf3c7321457c9448f4ce5c6810fa532c5e..97c24f2c3bd275b15945f60f2f8704a21a89fad9 100644
--- a/chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc
+++ b/chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc
@@ -135,6 +135,25 @@ void LogActivationDecisionMetrics(content::NavigationHandle* navigation_handle,
 
 }  // namespace
 
+SubresourceFilterMetricsObserver::SubresourceFilterMetricsObserver()
+    : scoped_observer_(this) {}
+SubresourceFilterMetricsObserver::~SubresourceFilterMetricsObserver() = default;
+
+page_load_metrics::PageLoadMetricsObserver::ObservePolicy
+SubresourceFilterMetricsObserver::OnStart(
+    content::NavigationHandle* navigation_handle,
+    const GURL& currently_committed_url,
+    bool started_in_foreground) {
+  auto* observer_manager =
+      subresource_filter::SubresourceFilterObserverManager::FromWebContents(
+          navigation_handle->GetWebContents());
+  // |observer_manager| isn't always constructed in unit tests, or if the master
+  // feature for subresource filtering isn't enabled.
+  if (observer_manager)
+    scoped_observer_.Add(observer_manager);
+  return CONTINUE_OBSERVING;
+}
+
 page_load_metrics::PageLoadMetricsObserver::ObservePolicy
 SubresourceFilterMetricsObserver::FlushMetricsOnAppEnterBackground(
     const page_load_metrics::mojom::PageLoadTiming& timing,
@@ -152,13 +171,12 @@ page_load_metrics::PageLoadMetricsObserver::ObservePolicy
 SubresourceFilterMetricsObserver::OnCommit(
     content::NavigationHandle* navigation_handle,
     ukm::SourceId source_id) {
-  const auto* subres_filter =
-      ContentSubresourceFilterDriverFactory::FromWebContents(
-          navigation_handle->GetWebContents());
-  if (subres_filter)
-    LogActivationDecisionMetrics(
-        navigation_handle,
-        subres_filter->GetActivationDecisionForLastCommittedPageLoad());
+  // If |this| is registered as an observer, it should have received the
+  // activation decision by now.
+  DCHECK(!scoped_observer_.IsObservingSources() ||

[Bryan McQuade, 2017/06/05 13:47:04]

if we get to the commit and we're no longer observing, is it possible for
subresource filtering to be active for the current load?

if so, maybe add a comment to explain that case since i was under the impression
that could not happen.

if not, should we just stop observing at this point? are there any metrics we
want to log in this observer, in cases where we don't have an activation
decision?

maybe we could do:

// If |this| is no longer observing, subresource filtering can't activate for
this load,
// so stop observing.
if (!scoped_observer_.IsObservingSources())
  return STOP_OBSERVING;

LogActivationDecisionMetrics(navigation_handle, *activation_decision_);
return CONTINUE_OBSERVING;

[Charlie Harrison, 2017/06/05 18:32:38]

As discussed offline there is some confusion here. We should never stop
observing once we've started (unless we are tearing down the web contents). We
should also never really be in a non-observing state. Currently we are for unit
tests but I've updated the CL so we can immediately stop observing if there
isn't a SubresourceFilterObserverManager.
Does this make sense?

+         activation_decision_.has_value());
+  if (activation_decision_)
+    LogActivationDecisionMetrics(navigation_handle, *activation_decision_);
   return CONTINUE_OBSERVING;
 }
 
@@ -298,6 +316,17 @@ void SubresourceFilterMetricsObserver::MediaStartedPlaying(
   played_media_ = true;
 }
 
+void SubresourceFilterMetricsObserver::OnSubresourceFilterGoingAway() {
+  scoped_observer_.RemoveAll();

[Bryan McQuade, 2017/06/05 13:47:04]

does it make sense to move this logic into the base interface? are are there
some cases where an implementer wouldn't want this implementation?

[Charlie Harrison, 2017/06/05 18:32:38]

I'm not sure. I think most implementations will want this but I'd like to keep
this interface abstract if possible to reduce multiple inheritance. I also don't
want to force observers into using scoped_observer. 

+}
+
+void SubresourceFilterMetricsObserver::OnPageActivationComputed(
+    content::NavigationHandle* navigation_handle,
+    subresource_filter::ActivationDecision activation_decision,
+    const subresource_filter::ActivationState& activation_state) {
+  activation_decision_ = activation_decision;

[Bryan McQuade, 2017/06/05 13:47:04]

can we add a DCHECK(!did_commit_); here (and set the bool to true in the
OnCommit callback)? I want to make sure that we can never invoke this method
after the nav has committed. It looks like the current arch prevents that, but
the path to invoke this callback is nontrivial with various callbacks being
invoked, and it's not impossible that a refactor in the future could lead us to
a place where the ordering breaks. My guess is that's unlikely, but I'd feel
better knowing we DCHECK it here.

and while we are in here, let's also DCHECK(!activation_decision_); just to
verify that this only gets invoked once per page load.

[Charlie Harrison, 2017/06/05 18:32:38]

Done, great idea.

+}
+
 void SubresourceFilterMetricsObserver::OnGoingAway(
     const page_load_metrics::mojom::PageLoadTiming& timing,
     const page_load_metrics::PageLoadExtraInfo& info,