Use common code to set HttpNetworkSession::Param pointers.

chrome/browser/io_thread.cc

Index: chrome/browser/io_thread.cc
diff --git a/chrome/browser/io_thread.cc b/chrome/browser/io_thread.cc
index 56b428dda57052d9ab1f40de7eb16e6ede85dd5a..32355cbff10d3f11a5877c0b1fa47dd6a5c1b77a 100644
--- a/chrome/browser/io_thread.cc
+++ b/chrome/browser/io_thread.cc
@@ -87,6 +87,7 @@
 #include "net/url_request/url_fetcher.h"
 #include "net/url_request/url_request_backoff_manager.h"
 #include "net/url_request/url_request_context.h"
+#include "net/url_request/url_request_context_builder.h"
 #include "net/url_request/url_request_context_getter.h"
 #include "net/url_request/url_request_job_factory_impl.h"
 #include "url/url_constants.h"
@@ -214,68 +215,6 @@ scoped_ptr<net::HostResolver> CreateGlobalHostResolver(net::NetLog* net_log) {
   return remapped_resolver.Pass();
 }
 
-// TODO(willchan): Remove proxy script fetcher context since it's not necessary
-// now that I got rid of refcounting URLRequestContexts.
-// See IOThread::Globals for details.
-net::URLRequestContext*
-ConstructProxyScriptFetcherContext(IOThread::Globals* globals,
-                                   net::NetLog* net_log) {
-  net::URLRequestContext* context = new net::URLRequestContext;
-  context->set_net_log(net_log);
-  context->set_host_resolver(globals->host_resolver.get());
-  context->set_cert_verifier(globals->cert_verifier.get());
-  context->set_transport_security_state(
-      globals->transport_security_state.get());
-  context->set_cert_transparency_verifier(
-      globals->cert_transparency_verifier.get());
-  context->set_http_auth_handler_factory(
-      globals->http_auth_handler_factory.get());
-  context->set_proxy_service(globals->proxy_script_fetcher_proxy_service.get());
-  context->set_http_transaction_factory(
-      globals->proxy_script_fetcher_http_transaction_factory.get());
-  context->set_job_factory(
-      globals->proxy_script_fetcher_url_request_job_factory.get());
-  context->set_cookie_store(globals->system_cookie_store.get());
-  context->set_channel_id_service(
-      globals->system_channel_id_service.get());
-  context->set_network_delegate(globals->system_network_delegate.get());
-  context->set_http_user_agent_settings(
-      globals->http_user_agent_settings.get());
-  // TODO(rtenneti): We should probably use HttpServerPropertiesManager for the
-  // system URLRequestContext too. There's no reason this should be tied to a
-  // profile.
-  return context;
-}
-
-net::URLRequestContext*
-ConstructSystemRequestContext(IOThread::Globals* globals,
-                              net::NetLog* net_log) {
-  net::URLRequestContext* context = new SystemURLRequestContext;
-  context->set_net_log(net_log);
-  context->set_host_resolver(globals->host_resolver.get());
-  context->set_cert_verifier(globals->cert_verifier.get());
-  context->set_transport_security_state(
-      globals->transport_security_state.get());
-  context->set_cert_transparency_verifier(
-      globals->cert_transparency_verifier.get());
-  context->set_http_auth_handler_factory(
-      globals->http_auth_handler_factory.get());
-  context->set_proxy_service(globals->system_proxy_service.get());
-  context->set_http_transaction_factory(
-      globals->system_http_transaction_factory.get());
-  context->set_job_factory(globals->system_url_request_job_factory.get());
-  context->set_cookie_store(globals->system_cookie_store.get());
-  context->set_channel_id_service(
-      globals->system_channel_id_service.get());
-  context->set_network_delegate(globals->system_network_delegate.get());
-  context->set_http_user_agent_settings(
-      globals->http_user_agent_settings.get());
-  context->set_network_quality_estimator(
-      globals->network_quality_estimator.get());
-  context->set_backoff_manager(globals->url_request_backoff_manager.get());
-  return context;
-}
-
 int GetSwitchValueAsInt(const base::CommandLine& command_line,
                         const std::string& switch_name) {
   int value;
@@ -816,54 +755,6 @@ void IOThread::Init() {
           "466432 IOThread::InitAsync::InitializeNetworkOptions"));
   InitializeNetworkOptions(command_line);
 
-  net::HttpNetworkSession::Params session_params;
-  InitializeNetworkSessionParams(&session_params);
-  session_params.net_log = net_log_;
-  session_params.proxy_service =
-      globals_->proxy_script_fetcher_proxy_service.get();
-
-  // TODO(erikchen): Remove ScopedTracker below once http://crbug.com/466432
-  // is fixed.
-  tracked_objects::ScopedTracker tracking_profile14(
-      FROM_HERE_WITH_EXPLICIT_FUNCTION(
-          "466432 IOThread::InitAsync::HttpNetorkSession::Start"));
-  TRACE_EVENT_BEGIN0("startup", "IOThread::InitAsync:HttpNetworkSession");
-  scoped_refptr<net::HttpNetworkSession> network_session(
-      new net::HttpNetworkSession(session_params));
-  // TODO(erikchen): Remove ScopedTracker below once http://crbug.com/466432
-  // is fixed.
-  tracked_objects::ScopedTracker tracking_profile15(
-      FROM_HERE_WITH_EXPLICIT_FUNCTION(
-          "466432 IOThread::InitAsync::HttpNetorkSession::End"));
-  globals_->proxy_script_fetcher_http_transaction_factory
-      .reset(new net::HttpNetworkLayer(network_session.get()));
-  TRACE_EVENT_END0("startup", "IOThread::InitAsync:HttpNetworkSession");
-  scoped_ptr<net::URLRequestJobFactoryImpl> job_factory(
-      new net::URLRequestJobFactoryImpl());
-
-  // TODO(erikchen): Remove ScopedTracker below once http://crbug.com/466432
-  // is fixed.
-  tracked_objects::ScopedTracker tracking_profile16(
-      FROM_HERE_WITH_EXPLICIT_FUNCTION(
-          "466432 IOThread::InitAsync::SetProtocolHandler"));
-  job_factory->SetProtocolHandler(url::kDataScheme,
-                                  new net::DataProtocolHandler());
-  job_factory->SetProtocolHandler(
-      url::kFileScheme,
-      new net::FileProtocolHandler(
-          content::BrowserThread::GetBlockingPool()->
-              GetTaskRunnerWithShutdownBehavior(
-                  base::SequencedWorkerPool::SKIP_ON_SHUTDOWN)));
-#if !defined(DISABLE_FTP_SUPPORT)
-  globals_->proxy_script_fetcher_ftp_transaction_factory.reset(
-      new net::FtpNetworkLayer(globals_->host_resolver.get()));
-  job_factory->SetProtocolHandler(
-      url::kFtpScheme,
-      new net::FtpProtocolHandler(
-          globals_->proxy_script_fetcher_ftp_transaction_factory.get()));
-#endif
-  globals_->proxy_script_fetcher_url_request_job_factory = job_factory.Pass();
-
   globals_->proxy_script_fetcher_context.reset(
       ConstructProxyScriptFetcherContext(globals_, net_log_));
 
@@ -1100,17 +991,12 @@ void IOThread::InitializeNetworkSessionParams(
 void IOThread::InitializeNetworkSessionParamsFromGlobals(
     const IOThread::Globals& globals,
     net::HttpNetworkSession::Params* params) {
-  params->host_resolver = globals.host_resolver.get();
-  params->cert_verifier = globals.cert_verifier.get();
+  // TODO(wjmaclean): The next two properties of the params don't seem to be
+  // elements of URLRequestContext, so they must be set here.
+  // Is it OK that they are shared?
   params->cert_policy_enforcer = globals.cert_policy_enforcer.get();
-  params->channel_id_service = globals.system_channel_id_service.get();
-  params->transport_security_state = globals.transport_security_state.get();
-  params->ssl_config_service = globals.ssl_config_service.get();
-  params->http_auth_handler_factory = globals.http_auth_handler_factory.get();
-  params->http_server_properties =
-      globals.http_server_properties->GetWeakPtr();
-  params->network_delegate = globals.system_network_delegate.get();
   params->host_mapping_rules = globals.host_mapping_rules.get();
+
   params->ignore_certificate_errors = globals.ignore_certificate_errors;
   params->testing_fixed_http_port = globals.testing_fixed_http_port;
   params->testing_fixed_https_port = globals.testing_fixed_https_port;
@@ -1221,22 +1107,8 @@ void IOThread::InitSystemRequestContextOnIOThread() {
           command_line,
           quick_check_enabled_.GetValue()));
 
-  net::HttpNetworkSession::Params system_params;
-  InitializeNetworkSessionParams(&system_params);
-  system_params.net_log = net_log_;
-  system_params.proxy_service = globals_->system_proxy_service.get();
-
-  globals_->system_http_transaction_factory.reset(
-      new net::HttpNetworkLayer(
-          new net::HttpNetworkSession(system_params)));
-  globals_->system_url_request_job_factory.reset(
-      new net::URLRequestJobFactoryImpl());
   globals_->system_request_context.reset(
       ConstructSystemRequestContext(globals_, net_log_));
-  globals_->system_request_context->set_ssl_config_service(
-      globals_->ssl_config_service.get());
-  globals_->system_request_context->set_http_server_properties(
-      globals_->http_server_properties->GetWeakPtr());
 }
 
 void IOThread::UpdateDnsClientEnabled() {
@@ -1600,3 +1472,140 @@ net::QuicVersion IOThread::ParseQuicVersion(const std::string& quic_version) {
 
   return net::QUIC_VERSION_UNSUPPORTED;
 }
+
+// static
+net::URLRequestContext*
+IOThread::ConstructSystemRequestContext(IOThread::Globals* globals,
+                              net::NetLog* net_log) {
+  net::URLRequestContext* context = new SystemURLRequestContext;
+  context->set_net_log(net_log);
+  context->set_host_resolver(globals->host_resolver.get());
+  context->set_cert_verifier(globals->cert_verifier.get());
+  context->set_transport_security_state(
+      globals->transport_security_state.get());
+  context->set_cert_transparency_verifier(
+      globals->cert_transparency_verifier.get());
+  context->set_ssl_config_service(globals->ssl_config_service.get());
+  context->set_http_auth_handler_factory(
+      globals->http_auth_handler_factory.get());
+  context->set_proxy_service(globals->system_proxy_service.get());
+
+  globals->system_url_request_job_factory.reset(
+      new net::URLRequestJobFactoryImpl());
+  context->set_job_factory(globals->system_url_request_job_factory.get());
+
+  context->set_cookie_store(globals->system_cookie_store.get());
+  context->set_channel_id_service(
+      globals->system_channel_id_service.get());
+  context->set_network_delegate(globals->system_network_delegate.get());
+  context->set_http_user_agent_settings(
+      globals->http_user_agent_settings.get());
+  context->set_network_quality_estimator(
+      globals->network_quality_estimator.get());
+  context->set_backoff_manager(globals->url_request_backoff_manager.get());
+
+  context->set_ssl_config_service(
+      globals->ssl_config_service.get());
+  context->set_http_server_properties(
+      globals->http_server_properties->GetWeakPtr());
+
+  net::HttpNetworkSession::Params system_params;
+  InitializeNetworkSessionParamsFromGlobals(*globals, &system_params);
+  net::URLRequestContextBuilder::SetHttpNetworkSessionComponents(&system_params,
+                                                                 context);
+
+  globals->system_http_transaction_factory.reset(
+      new net::HttpNetworkLayer(new net::HttpNetworkSession(system_params)));
+  context->set_http_transaction_factory(
+      globals->system_http_transaction_factory.get());
+
+  return context;
+}
+
+// TODO(willchan): Remove proxy script fetcher context since it's not necessary
+// now that I got rid of refcounting URLRequestContexts.

[mmenke, 2015/08/17 20:13:04]

Hrm...Wonder how doable this is.  Not suggesting you take it on, but if I could
get this done myself, make some of the future refactors a little simpler.

[wjmaclean, 2015/08/18 14:17:23]

Acknowledged.

+// See IOThread::Globals for details.
+// static
+net::URLRequestContext* IOThread::ConstructProxyScriptFetcherContext(
+    IOThread::Globals* globals,
+    net::NetLog* net_log) {
+  net::URLRequestContext* context = new net::URLRequestContext;
+  context->set_net_log(net_log);
+  context->set_host_resolver(globals->host_resolver.get());
+  context->set_cert_verifier(globals->cert_verifier.get());
+  context->set_transport_security_state(
+      globals->transport_security_state.get());
+  context->set_cert_transparency_verifier(
+      globals->cert_transparency_verifier.get());
+  context->set_ssl_config_service(globals->ssl_config_service.get());
+  context->set_http_auth_handler_factory(
+      globals->http_auth_handler_factory.get());
+  context->set_proxy_service(globals->proxy_script_fetcher_proxy_service.get());
+
+  scoped_ptr<net::URLRequestJobFactoryImpl> job_factory(
+      new net::URLRequestJobFactoryImpl());
+
+  // TODO(erikchen): Remove ScopedTracker below once http://crbug.com/466432
+  // is fixed.
+  tracked_objects::ScopedTracker tracking_profile16(
+      FROM_HERE_WITH_EXPLICIT_FUNCTION(
+          "466432 IOThread::InitAsync::SetProtocolHandler"));

[mmenke, 2015/08/17 20:13:04]

This should be renamed "466432 IOThread::ConstructProxyScriptFetcherContext" and
probably moved to the start of this method.

[wjmaclean, 2015/08/18 14:17:22]

Done.

+  job_factory->SetProtocolHandler(url::kDataScheme,
+                                  new net::DataProtocolHandler());
+  job_factory->SetProtocolHandler(
+      url::kFileScheme,
+      new net::FileProtocolHandler(
+          content::BrowserThread::GetBlockingPool()->
+              GetTaskRunnerWithShutdownBehavior(
+                  base::SequencedWorkerPool::SKIP_ON_SHUTDOWN)));
+#if !defined(DISABLE_FTP_SUPPORT)
+  globals->proxy_script_fetcher_ftp_transaction_factory.reset(
+      new net::FtpNetworkLayer(globals->host_resolver.get()));
+  job_factory->SetProtocolHandler(
+      url::kFtpScheme,
+      new net::FtpProtocolHandler(
+          globals->proxy_script_fetcher_ftp_transaction_factory.get()));
+#endif
+  globals->proxy_script_fetcher_url_request_job_factory = job_factory.Pass();
+
+  context->set_job_factory(
+      globals->proxy_script_fetcher_url_request_job_factory.get());

[mmenke, 2015/08/17 20:13:03]

Suggest we put job factory initialization last, since it involves the most code.

[wjmaclean, 2015/08/18 14:17:22]

Done.

+
+  context->set_cookie_store(globals->system_cookie_store.get());
+  context->set_channel_id_service(
+      globals->system_channel_id_service.get());
+  context->set_network_delegate(globals->system_network_delegate.get());
+  context->set_http_user_agent_settings(
+      globals->http_user_agent_settings.get());
+  context->set_http_server_properties(
+      globals->http_server_properties->GetWeakPtr());

[mmenke, 2015/08/17 20:13:04]

This last one wasn't being done on the proxy context before, I believe?  I don't
believe it matters, but want to try and minimize functional changes in this CL,
for now.

[wjmaclean, 2015/08/18 14:17:22]

Adding this was necessary because we need the http_server_properties for the
HttpNetworkSession construction (it has a CHECK requiring it
https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_netw...).

Before this use to get set directly on the params in
IOThread::InitializeNetworkSessionParamsFromGlobals() called from
IOThread::Init(), but since the refactored code pulls this value from the
context, it needs to be set on the context in order to match how other contexts
are built.

If we're really worried it will change something we can clear it again after
line 1586 below. But it would be nice if this context could be consistent with
other contexts.

[mmenke, 2015/08/18 18:30:32]

Ahh...It was being set on the session, but not the context.  Which seems like a
bug, though a harmless one - everything in net accesses them through the
session.

+
+  net::HttpNetworkSession::Params session_params;
+  InitializeNetworkSessionParamsFromGlobals(*globals, &session_params);
+  net::URLRequestContextBuilder::SetHttpNetworkSessionComponents(
+      &session_params, context);
+
+  // TODO(erikchen): Remove ScopedTracker below once http://crbug.com/466432
+  // is fixed.
+  tracked_objects::ScopedTracker tracking_profile14(
+      FROM_HERE_WITH_EXPLICIT_FUNCTION(
+          "466432 IOThread::InitAsync::HttpNetorkSession::Start"));

[mmenke, 2015/08/17 20:13:04]

"466432 IOThread::ConstructProxyScriptFetcherContext2"

[wjmaclean, 2015/08/18 14:17:23]

Done.

+  TRACE_EVENT_BEGIN0("startup", "IOThread::InitAsync:HttpNetworkSession");

[mmenke, 2015/08/17 20:13:04]

These two TRACE_EVENTS are weird.  Suggest moving them back into IOThread::Init,
making them surround this method call, and renaming them to
IOThread::Init::ProxyScriptFetcherRequestContext.

[wjmaclean, 2015/08/18 14:17:22]

Done.

+  scoped_refptr<net::HttpNetworkSession> network_session(
+      new net::HttpNetworkSession(session_params));
+  // TODO(erikchen): Remove ScopedTracker below once http://crbug.com/466432
+  // is fixed.
+  tracked_objects::ScopedTracker tracking_profile15(
+      FROM_HERE_WITH_EXPLICIT_FUNCTION(
+          "466432 IOThread::InitAsync::HttpNetorkSession::End"));
+  globals->proxy_script_fetcher_http_transaction_factory
+      .reset(new net::HttpNetworkLayer(network_session.get()));
+  context->set_http_transaction_factory(
+      globals->proxy_script_fetcher_http_transaction_factory.get());
+  TRACE_EVENT_END0("startup", "IOThread::InitAsync:HttpNetworkSession");
+
+  // TODO(rtenneti): We should probably use HttpServerPropertiesManager for the
+  // system URLRequestContext too. There's no reason this should be tied to a
+  // profile.
+  return context;
+}