Adding DRP specfic UMA for FirstContentfulPaint

chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc

+// Copyright 2016 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/data_reduction_proxy_metrics_observer.h"
+
+#include "base/metrics/histogram.h"
+#include "chrome/browser/renderer_host/chrome_navigation_data.h"
+#include "components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h"
+#include "components/data_reduction_proxy/core/common/data_reduction_proxy_params.h"
+#include "components/page_load_metrics/browser/page_load_metrics_observer.h"
+#include "components/page_load_metrics/browser/page_load_metrics_util.h"
+#include "components/page_load_metrics/common/page_load_timing.h"
+#include "content/public/browser/navigation_data.h"
+#include "content/public/browser/navigation_handle.h"
+
+namespace data_reduction_proxy {
+
+namespace internal {

[bengr, 2016/04/29 21:14:20]

Why does this namespace need to be named? Can it be anonymous?

[RyanSturm, 2016/05/02 19:52:18]

So it has external linkage. I don't think the extern keyword is enough to make
unnamed namespace variables externally linked. Alternatively, I could copy these
to the unittests, but I prefer having a shared definition.

+
+const char kHistogramFirstContentfulPaintDataReductionProxy[] =
+    "PageLoad.Clients.DataReductionProxy.Timing2."
+    "NavigationToFirstContentfulPaint";
+const char kHistogramFirstContentfulPaintDataReductionProxyAutoLofiOn[] =
+    "PageLoad.Clients.DataReductionProxy.AutoLoFiOn.Timing2."
+    "NavigationToFirstContentfulPaint";
+const char kHistogramFirstContentfulPaintDataReductionProxyAutoLofiOff[] =
+    "PageLoad.Clients.DataReductionProxy.AutoLoFiOff.Timing2."
+    "NavigationToFirstContentfulPaint";
+
+}  // namespace internal
+
+DataReductionProxyMetricsObserver::DataReductionProxyMetricsObserver()
+    : is_using_lofi_(false), used_data_reduction_proxy_(false) {}

[bengr, 2016/04/29 21:14:20]

Should these be an enum? I ask because used_data_reduction_proxy_ must be true
if is_using_lofi_ is true. In the future, we might want to record UMA per
individual optimization also. Something like:

enum {
  PREVIEW_NONE,
  PREVIEW_NONE_VIA_DATA_REDUCTION_PROXY,
  PREVIEW_LOFI,
  PREVIEW_WEBLITE,
  PREVIEW_RANGE_REQUESTS,
  ...
}

[RyanSturm, 2016/05/02 19:52:18]

I think it should be an enum later if we find a good way. There are a lot more
combinations (what we request and what the sever gave us may differ), so I'm
hesitant right now. At some point we might change this, but right now, I don't
think we should. I think, we might actually want to do an enum of what mode was
requested, but right now, I want the bool for used_data_reduction_proxy to be
it's own state.

+
+DataReductionProxyMetricsObserver::~DataReductionProxyMetricsObserver() {}
+
+// Check if the NavigationData indicates anything about the DataReductionProxy.
+void DataReductionProxyMetricsObserver::OnCommit(
+    content::NavigationHandle* navigation_handle) {
+  // As documented in content, this NavigationData is a clone of the
+  // NavigationData instance returned from
+  // ResourceDispatcherHostDelegate::GetNavigationData during commit, which is

[bengr, 2016/04/29 21:14:20]

My parser stopped working here. When does cloning happen? What is returned
during commit, the original or the clone? Are both always ChromeNavigationData?

[RyanSturm, 2016/05/02 19:52:18]

Done.

+  // always a ChromeNavigationData.
+  ChromeNavigationData* chrome_navigation_data =
+      static_cast<ChromeNavigationData*>(
+          navigation_handle->GetNavigationData());
+  if (!chrome_navigation_data) {

[bengr, 2016/04/29 21:14:20]

Remove curly braces.

[RyanSturm, 2016/05/02 19:52:18]

Done.

+    return;
+  }
+  data_reduction_proxy::DataReductionProxyData* data =
+      chrome_navigation_data->GetDataReductionProxyData();
+  if (data) {

[bengr, 2016/04/29 21:14:20]

I would follow the patter in line 48:

if (!data)
  return;
used_data_reduction_proxy_ = ...
...

[RyanSturm, 2016/05/02 19:52:18]

Done.

+    used_data_reduction_proxy_ = data->get_used_data_reduction_proxy();
+    is_using_lofi_ = data->get_is_using_lofi();
+  }
+}
+
+void DataReductionProxyMetricsObserver::OnComplete(
+    const page_load_metrics::PageLoadTiming& timing,
+    const page_load_metrics::PageLoadExtraInfo& info) {
+  RecordTimingHistograms(timing, info);
+}
+
+// Recording UMA for FirstContentfulPaint for various DataReductionProxy

[bengr, 2016/04/29 21:14:20]

nit: Recording UMA for FirstContentfulPaint -> Record first contentful paint UMA

[RyanSturm, 2016/05/02 19:52:18]

Done.

+// configurations.
+void DataReductionProxyMetricsObserver::RecordTimingHistograms(
+    const page_load_metrics::PageLoadTiming& timing,
+    const page_load_metrics::PageLoadExtraInfo& info) const {
+  if (timing.first_contentful_paint.is_zero() || !used_data_reduction_proxy_)
+    return;
+  PAGE_LOAD_HISTOGRAM(
+      internal::kHistogramFirstContentfulPaintDataReductionProxy,
+      timing.first_contentful_paint);
+  if (is_using_lofi_) {

[bengr, 2016/04/29 21:14:20]

if (!is_using_lofi_)
  return;
...

[RyanSturm, 2016/05/02 19:52:19]

Done.

+    if (data_reduction_proxy::params::IsIncludedInLoFiEnabledFieldTrial()) {
+      PAGE_LOAD_HISTOGRAM(
+          internal::kHistogramFirstContentfulPaintDataReductionProxyAutoLofiOn,
+          timing.first_contentful_paint);
+    } else if (data_reduction_proxy::params::
+                   IsIncludedInLoFiControlFieldTrial()) {
+      PAGE_LOAD_HISTOGRAM(
+          internal::kHistogramFirstContentfulPaintDataReductionProxyAutoLofiOff,
+          timing.first_contentful_paint);
+    }
+  }
+}
+
+}  // namespace data_reduction_proxy