Move data reduction proxy logic out of chrome and android webview network delegate

chrome/browser/profiles/profile_impl_io_data.cc

Index: chrome/browser/profiles/profile_impl_io_data.cc
diff --git a/chrome/browser/profiles/profile_impl_io_data.cc b/chrome/browser/profiles/profile_impl_io_data.cc
index 04dbdf2f08675a2589d2f0052cfc43b596f92186..8098642e4ac0dfd943f59d8d4ba186da9792b572 100644
--- a/chrome/browser/profiles/profile_impl_io_data.cc
+++ b/chrome/browser/profiles/profile_impl_io_data.cc
@@ -37,6 +37,7 @@
 #include "chrome/common/url_constants.h"
 #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_auth_request_handler.h"
 #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor.h"
+#include "components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h"
 #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_protocol.h"
 #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_statistics_prefs.h"
 #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.h"
@@ -117,7 +118,8 @@ ProfileImplIOData::Handle::~Handle() {
   if (io_data_->http_server_properties_manager_)
     io_data_->http_server_properties_manager_->ShutdownOnPrefThread();
 
-  io_data_->data_reduction_proxy_enabled()->Destroy();
+  io_data_->data_reduction_proxy_enabled_ui()->Destroy();
+  io_data_->data_reduction_proxy_enabled_io()->Destroy();

[mmenke, 2014/12/04 19:20:07]

Rather than having accessors for these, the handle can just access the private
fields directly (Inner classes have full access to the cuts of their parents -
see the code above to muck with the predictor_, for instance).

[megjablon, 2014/12/04 20:31:00]

Done.

   io_data_->ShutdownOnUIThread(GetAllContextGetters().Pass());
 }
 
@@ -365,9 +367,11 @@ void ProfileImplIOData::Handle::LazyInitialize() const {
   io_data_->safe_browsing_enabled()->MoveToThread(
       BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO));
 #endif
-  io_data_->data_reduction_proxy_enabled()->Init(
+  io_data_->data_reduction_proxy_enabled_ui()->Init(
+       data_reduction_proxy::prefs::kDataReductionProxyEnabled, pref_service);
+  io_data_->data_reduction_proxy_enabled_io()->Init(
       data_reduction_proxy::prefs::kDataReductionProxyEnabled, pref_service);
-  io_data_->data_reduction_proxy_enabled()->MoveToThread(
+  io_data_->data_reduction_proxy_enabled_io()->MoveToThread(
       BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO));
   io_data_->InitializeOnUIThread(profile_);
 }
@@ -414,9 +418,6 @@ ProfileImplIOData::ProfileImplIOData()
 }
 
 ProfileImplIOData::~ProfileImplIOData() {
-  if (initialized())
-    network_delegate()->set_domain_reliability_monitor(NULL);
-
   DestroyResourceContext();
 
   if (media_request_context_)
@@ -424,6 +425,7 @@ ProfileImplIOData::~ProfileImplIOData() {
 }
 
 void ProfileImplIOData::InitializeInternal(
+    scoped_ptr<ChromeNetworkDelegate> chrome_network_delegate,

[mmenke, 2014/12/04 19:20:07]

With your current code, the ProfileIOData creates it, passes ownership of it to
ProfileImplIOData.  The ProfileImplIOData wraps in a DRPNetworkDelegate, and
then passes ownership of that back to the ProfileIOData.  Can we skip the last
step, and have the ProfileIOData keep ownership of the DRPNetworkDelegate (Or,
in the case of incognito mode, the ChromeNetworkDelegate).

[mmenke, 2014/12/04 19:20:57]

Have the ProfileImplIOData keep ownership, rather.

[megjablon, 2014/12/04 20:31:00]

Done. Let me know if I misunderstood what you wanted. 

     ProfileParams* profile_params,
     content::ProtocolHandlerMap* protocol_handlers,
     content::URLRequestInterceptorScopedVector request_interceptors) const {
@@ -432,6 +434,17 @@ void ProfileImplIOData::InitializeInternal(
   IOThread* const io_thread = profile_params->io_thread;
   IOThread::Globals* const io_thread_globals = io_thread->globals();
 
+  chrome_network_delegate->set_predictor(predictor_.get());
+
+  if (domain_reliability_monitor_) {
+    domain_reliability::DomainReliabilityMonitor* monitor =
+        domain_reliability_monitor_.get();
+    monitor->InitURLRequestContext(main_context);
+    monitor->AddBakedInConfigs();
+    monitor->SetDiscardUploads(!GetMetricsEnabledStateOnIOThread());
+    chrome_network_delegate->set_domain_reliability_monitor(monitor);
+  }
+
   set_data_reduction_proxy_auth_request_handler(
       scoped_ptr<data_reduction_proxy::DataReductionProxyAuthRequestHandler>
           (new data_reduction_proxy::DataReductionProxyAuthRequestHandler(
@@ -446,24 +459,24 @@ void ProfileImplIOData::InitializeInternal(
                   .get())));
   data_reduction_proxy_usage_stats()->set_unavailable_callback(
       data_reduction_proxy_unavailable_callback());
-
-  network_delegate()->set_data_reduction_proxy_enabled_pref(
-      &data_reduction_proxy_enabled_);
-  network_delegate()->set_data_reduction_proxy_params(
-      data_reduction_proxy_params());
-  network_delegate()->set_data_reduction_proxy_usage_stats(
-      data_reduction_proxy_usage_stats());
-  network_delegate()->set_data_reduction_proxy_auth_request_handler(
-      data_reduction_proxy_auth_request_handler());
-  network_delegate()->set_data_reduction_proxy_statistics_prefs(
-      data_reduction_proxy_statistics_prefs());
-  network_delegate()->set_on_resolve_proxy_handler(
+  scoped_ptr<data_reduction_proxy::DataReductionProxyNetworkDelegate>

[mmenke, 2014/12/04 19:20:07]

Maybe a linebreak here?  Or better, just pass in everything the
DRPNetworkDelegate needs into its constructor, rather than having 4 calls into
it initializing various things.  The profile code shouldn't know or care how it
uses the stuff we're passing into it.

[megjablon, 2014/12/04 20:31:00]

On webview we don't need all of these various arguments. This was so that we
could add specific DRP behavior to various platforms and know which arguments we
need to get that behavior. Since the Prefs and UMA are now sharing the
drp_enabled_ boolean pref I grouped those into one init call.

+  data_reduction_proxy_network_delegate(
+      new data_reduction_proxy::DataReductionProxyNetworkDelegate(
+          chrome_network_delegate.Pass(),
+          data_reduction_proxy_params(),
+          data_reduction_proxy_auth_request_handler(),
+          base::Bind(
+              &DataReductionProxyChromeConfigurator::GetProxyConfigOnIOThread,
+              base::Unretained(data_reduction_proxy_chrome_configurator()))));
+  data_reduction_proxy_network_delegate->InitProxyConfigOverrides(
       base::Bind(data_reduction_proxy::OnResolveProxyHandler));
-  network_delegate()->set_proxy_config_getter(
-      base::Bind(
-          &DataReductionProxyChromeConfigurator::GetProxyConfigOnIOThread,
-          base::Unretained(data_reduction_proxy_chrome_configurator())));
-  network_delegate()->set_predictor(predictor_.get());
+  data_reduction_proxy_network_delegate->InitStatisticsPrefs(
+      &data_reduction_proxy_enabled_ui_,

[mmenke, 2014/12/04 19:20:07]

Why are you passing a preference that lives on the UI thread to an IO thread
object on the IO thread?  If the NetworkDelegate needs to check the value on the
UI thread, better to grab the value from data_reduction_proxy_enabled_io_ on the
IO thread, and pass that value back.  Otherwise, the profile may have been
deleted by the time you run the task on the UI thread, which would get you into
trouble.

Even if you have protects against that, very odd to be passing this at all on
the IO thread, and seems regression prone.

[megjablon, 2014/12/04 20:31:00]

Good to know! Thanks. Fixed.

+      BrowserThread::GetMessageLoopProxyForThread(BrowserThread::UI),
+      data_reduction_proxy_statistics_prefs());
+  data_reduction_proxy_network_delegate->InitStatisticsUMA(
+      &data_reduction_proxy_enabled_io_,
+      data_reduction_proxy_usage_stats());
 
   // Initialize context members.
 
@@ -476,7 +489,10 @@ void ProfileImplIOData::InitializeInternal(
 
   main_context->set_net_log(io_thread->net_log());
 
-  main_context->set_network_delegate(network_delegate());
+  main_context->set_network_delegate(
+      data_reduction_proxy_network_delegate.get());
+
+  set_network_delegate(data_reduction_proxy_network_delegate.Pass());
 
   main_context->set_http_server_properties(http_server_properties());
 
@@ -586,7 +602,7 @@ void ProfileImplIOData::InitializeInternal(
       main_job_factory.Pass(),
       request_interceptors.Pass(),
       profile_params->protocol_handler_interceptor.Pass(),
-      network_delegate(),
+      main_context->network_delegate(),
       ftp_factory_.get());
   main_context->set_job_factory(main_job_factory_.get());
 
@@ -605,15 +621,6 @@ void ProfileImplIOData::InitializeInternal(
   media_request_context_.reset(InitializeMediaRequestContext(main_context,
                                                              details));
 
-  if (domain_reliability_monitor_) {
-    domain_reliability::DomainReliabilityMonitor* monitor =
-        domain_reliability_monitor_.get();
-    monitor->InitURLRequestContext(main_context);
-    monitor->AddBakedInConfigs();
-    monitor->SetDiscardUploads(!GetMetricsEnabledStateOnIOThread());
-    network_delegate()->set_domain_reliability_monitor(monitor);
-  }
-
   lazy_params_.reset();
 }
 
@@ -747,7 +754,7 @@ net::URLRequestContext* ProfileImplIOData::InitializeAppRequestContext(
       SetUpJobFactoryDefaults(job_factory.Pass(),
                               request_interceptors.Pass(),
                               protocol_handler_interceptor.Pass(),
-                              network_delegate(),
+                              main_context->network_delegate(),
                               ftp_factory_.get()));
   context->SetJobFactory(top_job_factory.Pass());
 
@@ -854,7 +861,7 @@ void ProfileImplIOData::ClearNetworkingHistorySinceOnIOThread(
 }
 
 bool ProfileImplIOData::IsDataReductionProxyEnabled() const {

[mmenke, 2014/12/04 19:20:07]

Is this called from anywhere?  If so, fine to keep it.  If not, let's get rid of
it.

[megjablon, 2014/12/04 20:31:00]

Used here
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ren...

-  return data_reduction_proxy_enabled_.GetValue() ||
+  return data_reduction_proxy_enabled_io_.GetValue() ||
          CommandLine::ForCurrentProcess()->HasSwitch(
             data_reduction_proxy::switches::kEnableDataReductionProxy);
 }