Sending a page load pingback to data saver for holdback users

components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc

Index: components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
diff --git a/components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc b/components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
index 9666e3b888db7db49c2719168ff1b9233597a7ff..120141b773b67fa4e4caa043c539c4dd550d16fb 100644
--- a/components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
+++ b/components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
@@ -290,20 +290,7 @@ void DataReductionProxyNetworkDelegate::OnBeforeSendHeadersInternal(
 
   // Reset |request|'s DataReductionProxyData.
   DataReductionProxyData::ClearData(request);
-
-  if (params::IsIncludedInHoldbackFieldTrial()) {
-    if (WasEligibleWithoutHoldback(*request, proxy_info, proxy_retry_info)) {
-      // For the holdback field trial, still log UMA as if the proxy was used.
-      data = DataReductionProxyData::GetDataAndCreateIfNecessary(request);
-      if (data)
-        data->set_used_data_reduction_proxy(true);
-    }
-    // If holdback is enabled, |proxy_info| must not contain a data reduction
-    // proxy.
-    DCHECK(proxy_info.is_empty() ||
-           !data_reduction_proxy_config_->IsDataReductionProxy(
-               proxy_info.proxy_server(), nullptr));
-  }
+  data = nullptr;
 
   bool using_data_reduction_proxy = true;
   // The following checks rule out direct, invalid, and other connection types.
@@ -316,10 +303,42 @@ void DataReductionProxyNetworkDelegate::OnBeforeSendHeadersInternal(
                  proxy_info.proxy_server(), nullptr)) {
     using_data_reduction_proxy = false;
   }
+
+  bool is_holdback_eligible = false;
+
+  if (params::IsIncludedInHoldbackFieldTrial()) {

[tbansal1, 2017/06/09 22:32:55]

nit: combine these into single statement:
bool X = condition_1 && condition_2;

[RyanSturm, 2017/06/09 22:46:02]

Done.

+    if (WasEligibleWithoutHoldback(*request, proxy_info, proxy_retry_info)) {
+      is_holdback_eligible = true;
+    }
+  }
   // If holdback is enabled, |using_data_reduction_proxy| must be false.
   DCHECK(!params::IsIncludedInHoldbackFieldTrial() ||
          !using_data_reduction_proxy);
 
+  // For the holdback field trial, still log UMA and send the pingback as if
+  // the proxy were used.
+  if (is_holdback_eligible || using_data_reduction_proxy) {
+    // Retrieves DataReductionProxyData from a request, creating a new instance
+    // if needed.
+    data = DataReductionProxyData::GetDataAndCreateIfNecessary(request);
+    if (data) {

[tbansal1, 2017/06/09 22:32:55]

Why would data be null here?

[RyanSturm, 2017/06/09 22:46:02]

Done.

+      data->set_used_data_reduction_proxy(true);

[tbansal1, 2017/06/09 22:32:55]

Can you update comment for |used_data_reduction_proxy_| to say that it may be
true if holdback was set. or rename it to |eligible_data_reduction_proxy_|?

[RyanSturm, 2017/06/09 22:46:02]

Done.

+      // Only set GURL, NQE and session key string for main frame requests since
+      // they are not needed for sub-resources.
+      if (request->load_flags() & net::LOAD_MAIN_FRAME_DEPRECATED) {
+        data->set_session_key(
+            data_reduction_proxy_request_options_->GetSecureSession());
+        data->set_request_url(request->url());
+        if (request->context()->network_quality_estimator()) {
+          data->set_effective_connection_type(
+              request->context()
+                  ->network_quality_estimator()
+                  ->GetEffectiveConnectionType());
+        }
+      }
+    }
+  }
+
   LoFiDecider* lofi_decider = nullptr;
   if (data_reduction_proxy_io_data_)
     lofi_decider = data_reduction_proxy_io_data_->lofi_decider();
@@ -336,25 +355,7 @@ void DataReductionProxyNetworkDelegate::OnBeforeSendHeadersInternal(
     return;
   }
 
-  // Retrieves DataReductionProxyData from a request, creating a new instance
-  // if needed.
-  data = DataReductionProxyData::GetDataAndCreateIfNecessary(request);
-  if (data) {
-    data->set_used_data_reduction_proxy(true);
-    // Only set GURL, NQE and session key string for main frame requests since
-    // they are not needed for sub-resources.
-    if (request->load_flags() & net::LOAD_MAIN_FRAME_DEPRECATED) {
-      data->set_session_key(
-          data_reduction_proxy_request_options_->GetSecureSession());
-      data->set_request_url(request->url());
-      if (request->context()->network_quality_estimator()) {
-        data->set_effective_connection_type(request->context()
-                                                ->network_quality_estimator()
-                                                ->GetEffectiveConnectionType());
-      }
-    }
-  }
-
+  DCHECK(data);
   if (data_reduction_proxy_io_data_ &&
       (request->load_flags() & net::LOAD_MAIN_FRAME_DEPRECATED)) {
     data_reduction_proxy_io_data_->SetLoFiModeActiveOnMainFrame(
@@ -362,10 +363,8 @@ void DataReductionProxyNetworkDelegate::OnBeforeSendHeadersInternal(
                      : false);
   }
 
-  if (data) {
-    data->set_lofi_requested(
-        lofi_decider ? lofi_decider->ShouldRecordLoFiUMA(*request) : false);
-  }
+  data->set_lofi_requested(
+      lofi_decider ? lofi_decider->ShouldRecordLoFiUMA(*request) : false);
   MaybeAddBrotliToAcceptEncodingHeader(proxy_info, headers, *request);
 
   // Generate a page ID for main frame requests that don't already have one.