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

chrome/browser/profiles/profile_impl_io_data.cc

 // Copyright (c) 2012 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/profiles/profile_impl_io_data.h"
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/metrics/field_trial.h"
@@ skipping 19 matching lines @@
 #include "chrome/browser/net/quota_policy_channel_id_store.h"
 #include "chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.h"
 #include "chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_settings.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
 #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"
 #include "components/data_reduction_proxy/core/common/data_reduction_proxy_params.h"
 #include "components/data_reduction_proxy/core/common/data_reduction_proxy_pref_names.h"
 #include "components/domain_reliability/monitor.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/cookie_store_factory.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/resource_context.h"
@@ skipping 60 matching lines @@
     save_prefs = !chromeos::ProfileHelper::IsSigninProfile(profile_);
 #endif
     if (save_prefs)
       io_data_->predictor_->SaveStateForNextStartupAndTrim();
     io_data_->predictor_->ShutdownOnUIThread();
   }
 
   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());
 }
 
 void ProfileImplIOData::Handle::Init(
     const base::FilePath& cookie_path,
     const base::FilePath& channel_id_path,
     const base::FilePath& cache_path,
     int cache_max_size,
     const base::FilePath& media_cache_path,
     int media_cache_max_size,
@@ skipping 227 matching lines @@
   io_data_->session_startup_pref()->Init(
       prefs::kRestoreOnStartup, pref_service);
   io_data_->session_startup_pref()->MoveToThread(
       BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO));
 #if defined(FULL_SAFE_BROWSING) || defined(MOBILE_SAFE_BROWSING)
   io_data_->safe_browsing_enabled()->Init(prefs::kSafeBrowsingEnabled,
       pref_service);
   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_);
 }
 
 scoped_ptr<ProfileIOData::ChromeURLRequestContextGetterVector>
 ProfileImplIOData::Handle::GetAllContextGetters() {
   ChromeURLRequestContextGetterMap::iterator iter;
   scoped_ptr<ChromeURLRequestContextGetterVector> context_getters(
       new ChromeURLRequestContextGetterVector());
 
@@ skipping 26 matching lines @@
 ProfileImplIOData::LazyParams::~LazyParams() {}
 
 ProfileImplIOData::ProfileImplIOData()
     : ProfileIOData(Profile::REGULAR_PROFILE),
       http_server_properties_manager_(NULL),
       app_cache_max_size_(0),
       app_media_cache_max_size_(0) {
 }
 
 ProfileImplIOData::~ProfileImplIOData() {
-  if (initialized())
-    network_delegate()->set_domain_reliability_monitor(NULL);
-
   DestroyResourceContext();
 
   if (media_request_context_)
     media_request_context_->AssertNoURLRequests();
 }
 
 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 {
   net::URLRequestContext* main_context = main_request_context();
 
   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(
               DataReductionProxyChromeSettings::GetClient(),
               data_reduction_proxy_params(),
               BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO))));
   set_data_reduction_proxy_usage_stats(
       scoped_ptr<data_reduction_proxy::DataReductionProxyUsageStats>
           (new data_reduction_proxy::DataReductionProxyUsageStats(
               data_reduction_proxy_params(),
               BrowserThread::GetMessageLoopProxyForThread(BrowserThread::UI)
                   .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(
+  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));
+  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());
-  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(
-      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());
 
   // Initialize context members.
 
   ApplyProfileParamsToContext(main_context);
 
   if (http_server_properties_manager_)
     http_server_properties_manager_->InitializeOnNetworkThread();
 
   main_context->set_transport_security_state(transport_security_state());
 
   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());
 
   main_context->set_host_resolver(
       io_thread_globals->host_resolver.get());
   main_context->set_cert_transparency_verifier(
       io_thread_globals->cert_transparency_verifier.get());
   main_context->set_http_auth_handler_factory(
       io_thread_globals->http_auth_handler_factory.get());
 
@@ skipping 89 matching lines @@
   request_interceptors.insert(
       request_interceptors.begin(),
       new data_reduction_proxy::DataReductionProxyInterceptor(
           data_reduction_proxy_params(),
           data_reduction_proxy_usage_stats(),
           data_reduction_proxy_event_store()));
   main_job_factory_ = SetUpJobFactoryDefaults(
       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());
 
 #if defined(ENABLE_EXTENSIONS)
   InitializeExtensionsRequestContext(profile_params);
 #endif
 
   // Setup SDCH for this profile.
   sdch_manager_.reset(new net::SdchManager);
   sdch_policy_.reset(new ChromeSdchPolicy(sdch_manager_.get(), main_context));
   main_context->set_sdch_manager(sdch_manager_.get());
 
   // Create a media request context based on the main context, but using a
   // media cache.  It shares the same job factory as the main context.
   StoragePartitionDescriptor details(profile_path_, false);
   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();
 }
 
 void ProfileImplIOData::
     InitializeExtensionsRequestContext(ProfileParams* profile_params) const {
   net::URLRequestContext* extensions_context = extensions_request_context();
   IOThread* const io_thread = profile_params->io_thread;
   IOThread::Globals* const io_thread_globals = io_thread->globals();
   ApplyProfileParamsToContext(extensions_context);
 
@@ skipping 113 matching lines @@
   request_interceptors.insert(
       request_interceptors.begin(),
       new data_reduction_proxy::DataReductionProxyInterceptor(
           data_reduction_proxy_params(),
           data_reduction_proxy_usage_stats(),
           data_reduction_proxy_event_store()));
   scoped_ptr<net::URLRequestJobFactory> top_job_factory(
       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());
 
   return context;
 }
 
 net::URLRequestContext*
 ProfileImplIOData::InitializeMediaRequestContext(
     net::URLRequestContext* original_context,
     const StoragePartitionDescriptor& partition_descriptor) const {
@@ skipping 85 matching lines @@
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   DCHECK(initialized());
 
   DCHECK(transport_security_state());
   // Completes synchronously.
   transport_security_state()->DeleteAllDynamicDataSince(time);
   DCHECK(http_server_properties_manager_);
   http_server_properties_manager_->Clear(completion);
 }
 
 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);
 }