DRP metrics observer stop observing after hidden

chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc

Index: chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc
diff --git a/chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc b/chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc
index 57d0cc05a3e65f0238e29c1b3681616a802cafd4..1534bb7aaf9ffed4c0bd70df703ea4bc9cc4b2f0 100644
--- a/chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc
+++ b/chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc
@@ -22,46 +22,36 @@
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/navigation_data.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/web_contents.h"
 #include "url/gurl.h"
 
 namespace data_reduction_proxy {
 
 namespace {
 
-bool ShouldRecordHistogram(const DataReductionProxyData* data,
-                           const base::Optional<base::TimeDelta>& event,
-                           const page_load_metrics::PageLoadExtraInfo& info) {
-  return data && data->used_data_reduction_proxy() &&
-         WasStartedInForegroundOptionalEventInForeground(event, info);
-}
-
 // A macro is needed because PAGE_LOAD_HISTOGRAM creates a static instance of
 // the histogram. A distinct histogram is needed for each place that calls
 // RECORD_HISTOGRAMS_FOR_SUFFIX. |event| is the timing event representing when
 // |value| became available.
-#define RECORD_HISTOGRAMS_FOR_SUFFIX(data, event, value, info,              \
-                                     histogram_suffix)                      \
-  do {                                                                      \
-    if (ShouldRecordHistogram(data.get(), event, info)) {                   \
-      PAGE_LOAD_HISTOGRAM(                                                  \
-          std::string(internal::kHistogramDataReductionProxyPrefix)         \
-              .append(histogram_suffix),                                    \
-          value);                                                           \
-      if (data->lofi_requested()) {                                         \
-        PAGE_LOAD_HISTOGRAM(                                                \
-            std::string(internal::kHistogramDataReductionProxyLoFiOnPrefix) \
-                .append(histogram_suffix),                                  \
-            value);                                                         \
-      }                                                                     \
-    }                                                                       \
+#define RECORD_HISTOGRAMS_FOR_SUFFIX(data, value, histogram_suffix)       \
+  do {                                                                    \
+    PAGE_LOAD_HISTOGRAM(                                                  \
+        std::string(internal::kHistogramDataReductionProxyPrefix)         \
+            .append(histogram_suffix),                                    \
+        value);                                                           \
+    if (data->lofi_requested()) {                                         \
+      PAGE_LOAD_HISTOGRAM(                                                \
+          std::string(internal::kHistogramDataReductionProxyLoFiOnPrefix) \
+              .append(histogram_suffix),                                  \
+          value);                                                         \
+    }                                                                     \
   } while (false)
 
 }  // namespace
 
 namespace internal {
 
 const char kHistogramDataReductionProxyPrefix[] =
     "PageLoad.Clients.DataReductionProxy.";
 const char kHistogramDataReductionProxyLoFiOnPrefix[] =
     "PageLoad.Clients.DataReductionProxy.LoFiOn.";
@@ -109,38 +99,49 @@ DataReductionProxyMetricsObserver::OnCommit(
   // ResourceDispatcherHostDelegate::GetNavigationData during commit.
   // Because ChromeResourceDispatcherHostDelegate always returns a
   // ChromeNavigationData, it is safe to static_cast here.
   ChromeNavigationData* chrome_navigation_data =
       static_cast<ChromeNavigationData*>(
           navigation_handle->GetNavigationData());
   if (!chrome_navigation_data)
     return STOP_OBSERVING;
   data_reduction_proxy::DataReductionProxyData* data =
       chrome_navigation_data->GetDataReductionProxyData();
-  if (!data)
+  if (!data || !data->used_data_reduction_proxy())
     return STOP_OBSERVING;
   data_ = data->DeepCopy();
   // DataReductionProxy page loads should only occur on HTTP navigations.
-  DCHECK(!data_->used_data_reduction_proxy() ||
-         !navigation_handle->GetURL().SchemeIsCryptographic());
+  DCHECK(!navigation_handle->GetURL().SchemeIsCryptographic());
   return CONTINUE_OBSERVING;
 }
 
+page_load_metrics::PageLoadMetricsObserver::ObservePolicy

[Bryan McQuade, 2016/11/04 22:42:56]

great! we should also implement OnStart and return STOP_OBSERVING if the boolean
foreground/background param indicates we started in background.

[RyanSturm, 2016/11/04 22:54:06]

Done.

+DataReductionProxyMetricsObserver::OnHidden(
+    const page_load_metrics::PageLoadTiming& timing,
+    const page_load_metrics::PageLoadExtraInfo& info) {
+  SendPingback(timing, info);
+  return STOP_OBSERVING;
+}
+
 void DataReductionProxyMetricsObserver::OnComplete(
     const page_load_metrics::PageLoadTiming& timing,
     const page_load_metrics::PageLoadExtraInfo& info) {
+  SendPingback(timing, info);
+}
+
+void DataReductionProxyMetricsObserver::SendPingback(
+    const page_load_metrics::PageLoadTiming& timing,
+    const page_load_metrics::PageLoadExtraInfo& info) {
   // TODO(ryansturm): Move to OnFirstBackgroundEvent to handle some fast
   // shutdown cases. crbug.com/618072
   if (!browser_context_)
     return;
-  if (!data_ || !data_->used_data_reduction_proxy())
-    return;
   if (data_reduction_proxy::params::IsIncludedInHoldbackFieldTrial() ||
       data_reduction_proxy::params::IsIncludedInTamperDetectionExperiment()) {
     return;
   }
   // Only consider timing events that happened before the first background
   // event.
   base::Optional<base::TimeDelta> response_start;
   base::Optional<base::TimeDelta> load_event_start;
   base::Optional<base::TimeDelta> first_image_paint;
   base::Optional<base::TimeDelta> first_contentful_paint;
@@ -182,99 +183,89 @@ void DataReductionProxyMetricsObserver::OnComplete(
       first_image_paint, first_contentful_paint,
       experimental_first_meaningful_paint,
       parse_blocked_on_script_load_duration, parse_stop);
   GetPingbackClient()->SendPingback(*data_, data_reduction_proxy_timing);
 }
 
 void DataReductionProxyMetricsObserver::OnDomContentLoadedEventStart(
     const page_load_metrics::PageLoadTiming& timing,
     const page_load_metrics::PageLoadExtraInfo& info) {
   RECORD_HISTOGRAMS_FOR_SUFFIX(
-      data_, timing.dom_content_loaded_event_start,
-      timing.dom_content_loaded_event_start.value(), info,
+      data_, timing.dom_content_loaded_event_start.value(),
       internal::kHistogramDOMContentLoadedEventFiredSuffix);
 }
 
 void DataReductionProxyMetricsObserver::OnLoadEventStart(
     const page_load_metrics::PageLoadTiming& timing,
     const page_load_metrics::PageLoadExtraInfo& info) {
-  RECORD_HISTOGRAMS_FOR_SUFFIX(data_, timing.load_event_start,
-                               timing.load_event_start.value(), info,
+  RECORD_HISTOGRAMS_FOR_SUFFIX(data_, timing.load_event_start.value(),
                                internal::kHistogramLoadEventFiredSuffix);
 }
 
 void DataReductionProxyMetricsObserver::OnFirstLayout(
     const page_load_metrics::PageLoadTiming& timing,
     const page_load_metrics::PageLoadExtraInfo& info) {
-  RECORD_HISTOGRAMS_FOR_SUFFIX(data_, timing.first_layout,
-                               timing.first_layout.value(), info,
+  RECORD_HISTOGRAMS_FOR_SUFFIX(data_, timing.first_layout.value(),
                                internal::kHistogramFirstLayoutSuffix);
 }
 
 void DataReductionProxyMetricsObserver::OnFirstPaint(
     const page_load_metrics::PageLoadTiming& timing,
     const page_load_metrics::PageLoadExtraInfo& info) {
-  RECORD_HISTOGRAMS_FOR_SUFFIX(data_, timing.first_paint,
-                               timing.first_paint.value(), info,
+  RECORD_HISTOGRAMS_FOR_SUFFIX(data_, timing.first_paint.value(),
                                internal::kHistogramFirstPaintSuffix);
 }
 
 void DataReductionProxyMetricsObserver::OnFirstTextPaint(
     const page_load_metrics::PageLoadTiming& timing,
     const page_load_metrics::PageLoadExtraInfo& info) {
-  RECORD_HISTOGRAMS_FOR_SUFFIX(data_, timing.first_text_paint,
-                               timing.first_text_paint.value(), info,
+  RECORD_HISTOGRAMS_FOR_SUFFIX(data_, timing.first_text_paint.value(),
                                internal::kHistogramFirstTextPaintSuffix);
 }
 
 void DataReductionProxyMetricsObserver::OnFirstImagePaint(
     const page_load_metrics::PageLoadTiming& timing,
     const page_load_metrics::PageLoadExtraInfo& info) {
-  RECORD_HISTOGRAMS_FOR_SUFFIX(data_, timing.first_image_paint,
-                               timing.first_image_paint.value(), info,
+  RECORD_HISTOGRAMS_FOR_SUFFIX(data_, timing.first_image_paint.value(),
                                internal::kHistogramFirstImagePaintSuffix);
 }
 
 void DataReductionProxyMetricsObserver::OnFirstContentfulPaint(
     const page_load_metrics::PageLoadTiming& timing,
     const page_load_metrics::PageLoadExtraInfo& info) {
-  RECORD_HISTOGRAMS_FOR_SUFFIX(data_, timing.first_contentful_paint,
-                               timing.first_contentful_paint.value(), info,
+  RECORD_HISTOGRAMS_FOR_SUFFIX(data_, timing.first_contentful_paint.value(),
                                internal::kHistogramFirstContentfulPaintSuffix);
 }
 
 void DataReductionProxyMetricsObserver::OnFirstMeaningfulPaint(
     const page_load_metrics::PageLoadTiming& timing,
     const page_load_metrics::PageLoadExtraInfo& info) {
-  RECORD_HISTOGRAMS_FOR_SUFFIX(data_, timing.first_meaningful_paint,
-                               timing.first_meaningful_paint.value(), info,
+  RECORD_HISTOGRAMS_FOR_SUFFIX(data_, timing.first_meaningful_paint.value(),
                                internal::kHistogramFirstMeaningfulPaintSuffix);
 }
 
 void DataReductionProxyMetricsObserver::OnParseStart(
     const page_load_metrics::PageLoadTiming& timing,
     const page_load_metrics::PageLoadExtraInfo& info) {
-  RECORD_HISTOGRAMS_FOR_SUFFIX(data_, timing.parse_start,
-                               timing.parse_start.value(), info,
+  RECORD_HISTOGRAMS_FOR_SUFFIX(data_, timing.parse_start.value(),
                                internal::kHistogramParseStartSuffix);
 }
 
 void DataReductionProxyMetricsObserver::OnParseStop(
     const page_load_metrics::PageLoadTiming& timing,
     const page_load_metrics::PageLoadExtraInfo& info) {
   base::TimeDelta parse_duration =
       timing.parse_stop.value() - timing.parse_start.value();
-  RECORD_HISTOGRAMS_FOR_SUFFIX(data_, timing.parse_stop, parse_duration, info,
+  RECORD_HISTOGRAMS_FOR_SUFFIX(data_, parse_duration,
                                internal::kHistogramParseDurationSuffix);
   RECORD_HISTOGRAMS_FOR_SUFFIX(
-      data_, timing.parse_stop,
-      timing.parse_blocked_on_script_load_duration.value(), info,
+      data_, timing.parse_blocked_on_script_load_duration.value(),
       internal::kHistogramParseBlockedOnScriptLoadSuffix);
 }
 
 DataReductionProxyPingbackClient*
 DataReductionProxyMetricsObserver::GetPingbackClient() const {
   return DataReductionProxyChromeSettingsFactory::GetForBrowserContext(
              browser_context_)
       ->data_reduction_proxy_service()
       ->pingback_client();
 }