[subresource_filter] Use the SubresourceFilterObserver for PageLoadMetrics

chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.cc

 // Copyright 2017 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 "chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer.h"
 
 #include "chrome/browser/page_load_metrics/page_load_metrics_util.h"
 #include "components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h"
 #include "components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.h"
 #include "components/subresource_filter/core/common/activation_decision.h"
@@ skipping 117 matching lines @@
           navigation_handle)) {
     UMA_HISTOGRAM_ENUMERATION(
         internal::kHistogramSubresourceFilterActivationDecisionReload,
         static_cast<int>(decision),
         static_cast<int>(ActivationDecision::ACTIVATION_DECISION_MAX));
   }
 }
 
 }  // 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,
     const page_load_metrics::PageLoadExtraInfo& info) {
   // FlushMetricsOnAppEnterBackground is invoked on Android in cases where the
   // app is about to be backgrounded, as part of the Activity.onPause()
   // flow. After this method is invoked, Chrome may be killed without further
   // notification, so we record final metrics collected up to this point.
   if (info.did_commit)
     OnGoingAway(timing, info, base::TimeTicks::Now());
   return STOP_OBSERVING;
 }
 
 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;
 }
 
 void SubresourceFilterMetricsObserver::OnComplete(
     const page_load_metrics::mojom::PageLoadTiming& timing,
     const page_load_metrics::PageLoadExtraInfo& info) {
   OnGoingAway(timing, info, base::TimeTicks());
 }
 
 void SubresourceFilterMetricsObserver::OnLoadedResource(
@@ skipping 119 matching lines @@
     UMA_HISTOGRAM_BOOLEAN(internal::kHistogramSubresourceFilterCount, true);
   }
 }
 
 void SubresourceFilterMetricsObserver::MediaStartedPlaying(
     const content::WebContentsObserver::MediaPlayerInfo& video_type,
     bool is_in_main_frame) {
   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,
     base::TimeTicks app_background_time) {
   if (!subresource_filter_observed_)
     return;
 
   PAGE_RESOURCE_COUNT_HISTOGRAM(
       internal::kHistogramSubresourceFilterNetworkResources,
       num_network_resources_);
@@ skipping 56 matching lines @@
         foreground_duration.value());
     if (timing.paint_timing->first_paint &&
         timing.paint_timing->first_paint < foreground_duration) {
       PAGE_LOAD_LONG_HISTOGRAM(
           internal::kHistogramSubresourceFilterForegroundDurationAfterPaint,
           foreground_duration.value() -
               timing.paint_timing->first_paint.value());
     }
   }
 }