Clean up DataReductionProxyDelegate::OnResolveProxy

This change cleans up the logic in OnResolveProxy and its two helper
functions (one of which is removed) so that it is clearer what logic is
run only when a request is eligible for the data reduction proxy or only
when the request will be made through the data reduction proxy.
Duplicate checks are removed or the later ones turned into DCHECKs to
clarify assumptions.

Bug: 721403
Change-Id: I271a553e0c1406921c6a2a23ab21241ba3095df5
Reviewed-on: https://chromium-review.googlesource.com/887650
Commit-Queue: Reilly Grant <reillyg@chromium.org>
Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532951}

components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc

@@ -32,52 +32,6 @@ namespace {
 
 static const char kDataReductionCoreProxy[] = "proxy.googlezip.net";
 
-// Adds data reduction proxies to |result|, where applicable, if result
-// otherwise uses a direct connection for |url|, and the data reduction proxy is
-// not bypassed. Also, configures |result| to proceed directly to the origin if
-// |result|'s current proxy is the data reduction proxy.
-void OnResolveProxyHandler(
-    const GURL& url,
-    const std::string& method,
-    const net::ProxyConfig& proxy_config,
-    const net::ProxyRetryInfoMap& proxy_retry_info,
-    const DataReductionProxyConfig& data_reduction_proxy_config,
-    DataReductionProxyIOData* io_data,
-    net::ProxyInfo* result) {
-  DCHECK(result->is_empty() || result->is_direct() ||
-         !data_reduction_proxy_config.IsDataReductionProxy(
-             result->proxy_server(), nullptr));
-
-  if (!util::EligibleForDataReductionProxy(*result, url, method))
-    return;
-
-  net::ProxyInfo data_reduction_proxy_info;
-  bool data_saver_proxy_used = util::ApplyProxyConfigToProxyInfo(
-      proxy_config, proxy_retry_info, url, &data_reduction_proxy_info);
-  if (data_saver_proxy_used)
-    result->OverrideProxyList(data_reduction_proxy_info.proxy_list());
-
-  if (io_data && io_data->resource_type_provider()) {
-    ResourceTypeProvider::ContentType content_type =
-        io_data->resource_type_provider()->GetContentType(url);
-    DCHECK_GT(ResourceTypeProvider::CONTENT_TYPE_MAX, content_type);
-    UMA_HISTOGRAM_ENUMERATION("DataReductionProxy.ResourceContentType",
-                              content_type,
-                              ResourceTypeProvider::CONTENT_TYPE_MAX);
-  }
-
-  // The |proxy_config| must be valid otherwise the proxy cannot be used.
-  DCHECK(proxy_config.is_valid() || !data_saver_proxy_used);
-
-  if (data_reduction_proxy_config.enabled_by_user_and_reachable() &&
-      url.SchemeIs(url::kHttpScheme) && !net::IsLocalhost(url) &&
-      !params::IsIncludedInHoldbackFieldTrial()) {
-    UMA_HISTOGRAM_BOOLEAN(
-        "DataReductionProxy.ConfigService.HTTPRequests",
-        !data_reduction_proxy_config.GetProxiesForHttp().empty());
-  }
-}
-
 }  // namespace
 
 DataReductionProxyDelegate::DataReductionProxyDelegate(
@@ -121,15 +75,21 @@ void DataReductionProxyDelegate::OnResolveProxy(
     const std::string& method,
     const net::ProxyRetryInfoMap& proxy_retry_info,
     net::ProxyInfo* result) {
-  DCHECK(result);
   DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(result);
+  DCHECK(result->is_empty() || result->is_direct() ||
+         !config_->IsDataReductionProxy(result->proxy_server(), nullptr));
+
+  if (!params::IsIncludedInQuicFieldTrial())
+    RecordQuicProxyStatus(QUIC_PROXY_DISABLED_VIA_FIELD_TRIAL);
+
+  if (!util::EligibleForDataReductionProxy(*result, url, method))
+    return;
 
   ResourceTypeProvider::ContentType content_type =
       ResourceTypeProvider::CONTENT_TYPE_UNKNOWN;
-
-  if (io_data_ && io_data_->resource_type_provider()) {
+  if (io_data_ && io_data_->resource_type_provider())
     content_type = io_data_->resource_type_provider()->GetContentType(url);
-  }
 
   std::vector<DataReductionProxyServer> proxies_for_http =
       params::IsIncludedInHoldbackFieldTrial()
@@ -175,23 +135,33 @@ void DataReductionProxyDelegate::OnResolveProxy(
   net::ProxyConfig proxy_config = configurator_->CreateProxyConfig(
       is_warmup_url, config_->GetNetworkPropertiesManager(), proxies_for_http);
 
-  OnResolveProxyHandler(url, method, proxy_config, proxy_retry_info, *config_,
-                        io_data_, result);
+  net::ProxyInfo data_reduction_proxy_info;
+  if (util::ApplyProxyConfigToProxyInfo(proxy_config, proxy_retry_info, url,
+                                        &data_reduction_proxy_info)) {
+    DCHECK(!data_reduction_proxy_info.is_empty() &&
+           !data_reduction_proxy_info.is_direct());
+    result->OverrideProxyList(data_reduction_proxy_info.proxy_list());
 
-  if (!result->is_empty()) {
-    net::ProxyServer alternative_proxy_server;
-    if (GetAlternativeProxy(url, result->proxy_server(), proxy_retry_info,
-                            &alternative_proxy_server)) {
-      result->SetAlternativeProxy(alternative_proxy_server);
+    GetAlternativeProxy(url, proxy_retry_info, result);
+
+    if (!first_data_saver_request_recorded_) {
+      UMA_HISTOGRAM_MEDIUM_TIMES(
+          "DataReductionProxy.TimeToFirstDataSaverRequest",
+          tick_clock_->NowTicks() - last_network_change_time_);
+      first_data_saver_request_recorded_ = true;
     }
   }
 
-  if (!first_data_saver_request_recorded_ && !result->is_empty() &&
-      config_->IsDataReductionProxy(result->proxy_server(), nullptr)) {
-    UMA_HISTOGRAM_MEDIUM_TIMES(
-        "DataReductionProxy.TimeToFirstDataSaverRequest",
-        tick_clock_->NowTicks() - last_network_change_time_);
-    first_data_saver_request_recorded_ = true;
+  DCHECK_GT(ResourceTypeProvider::CONTENT_TYPE_MAX, content_type);
+  UMA_HISTOGRAM_ENUMERATION("DataReductionProxy.ResourceContentType",
+                            content_type,
+                            ResourceTypeProvider::CONTENT_TYPE_MAX);
+
+  if (config_->enabled_by_user_and_reachable() &&
+      url.SchemeIs(url::kHttpScheme) && !net::IsLocalhost(url) &&
+      !params::IsIncludedInHoldbackFieldTrial()) {
+    UMA_HISTOGRAM_BOOLEAN("DataReductionProxy.ConfigService.HTTPRequests",
+                          !config_->GetProxiesForHttp().empty());
   }
 }
 
@@ -216,34 +186,32 @@ void DataReductionProxyDelegate::SetTickClockForTesting(
   last_network_change_time_ = tick_clock_->NowTicks();
 }
 
-bool DataReductionProxyDelegate::GetAlternativeProxy(
+void DataReductionProxyDelegate::GetAlternativeProxy(
     const GURL& url,
-    const net::ProxyServer& resolved_proxy_server,
     const net::ProxyRetryInfoMap& proxy_retry_info,
-    net::ProxyServer* alternative_proxy_server) const {
+    net::ProxyInfo* result) const {
   DCHECK(thread_checker_.CalledOnValidThread());
 
+  net::ProxyServer resolved_proxy_server = result->proxy_server();
+  DCHECK(resolved_proxy_server.is_valid());
+  DCHECK(config_->IsDataReductionProxy(resolved_proxy_server, nullptr));
+
   if (!url.is_valid() || !url.SchemeIsHTTPOrHTTPS() ||
       url.SchemeIsCryptographic()) {
-    return false;
+    return;
   }
 
   if (!params::IsIncludedInQuicFieldTrial()) {
-    RecordQuicProxyStatus(QUIC_PROXY_DISABLED_VIA_FIELD_TRIAL);
-    return false;
+    // RecordQuicProxyStatus already called by OnResolveProxy.
+    return;
   }
 
-  if (!resolved_proxy_server.is_valid() || !resolved_proxy_server.is_https())
-    return false;
-
-  if (!config_ ||
-      !config_->IsDataReductionProxy(resolved_proxy_server, nullptr)) {
-    return false;
-  }
+  if (!resolved_proxy_server.is_https())
+    return;
 
   if (!SupportsQUIC(resolved_proxy_server)) {
     RecordQuicProxyStatus(QUIC_PROXY_NOT_SUPPORTED);
-    return false;
+    return;
   }
 
   net::ProxyInfo alternative_proxy_info;
@@ -253,12 +221,11 @@ bool DataReductionProxyDelegate::GetAlternativeProxy(
 
   if (alternative_proxy_info.is_empty()) {
     RecordQuicProxyStatus(QUIC_PROXY_STATUS_MARKED_AS_BROKEN);
-    return false;
+    return;
   }
 
   RecordQuicProxyStatus(QUIC_PROXY_STATUS_AVAILABLE);
-  *alternative_proxy_server = alternative_proxy_info.proxy_server();
-  return true;
+  result->SetAlternativeProxy(alternative_proxy_info.proxy_server());
 }
 
 bool DataReductionProxyDelegate::SupportsQUIC(

components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.h

@@ -80,10 +80,11 @@ class DataReductionProxyDelegate
   // NetworkChangeNotifier::IPAddressObserver:
   void OnIPAddressChanged() override;
 
-  bool GetAlternativeProxy(const GURL& url,
-                           const net::ProxyServer& resolved_proxy_server,
+  // Checks if the first proxy server in |result| supports QUIC and if so
+  // adds an alternative proxy configuration to |result|.
+  void GetAlternativeProxy(const GURL& url,
                            const net::ProxyRetryInfoMap& proxy_retry_info,
-                           net::ProxyServer* alternative_proxy_server) const;
+                           net::ProxyInfo* result) const;
 
   const DataReductionProxyConfig* config_;
   const DataReductionProxyConfigurator* configurator_;