Refactor pooled socket request code to prepare for implementation of pretend-to-preconnect.

net/socket/client_socket_pool_manager.cc

Index: net/socket/client_socket_pool_manager.cc
diff --git a/net/socket/client_socket_pool_manager.cc b/net/socket/client_socket_pool_manager.cc
index 71b9d6d6f5895230a0d7bae8083c930f3372e911..9e88dced9f6edd37fdd6a3251011ae33059f756e 100644
--- a/net/socket/client_socket_pool_manager.cc
+++ b/net/socket/client_socket_pool_manager.cc
@@ -64,61 +64,18 @@ COMPILE_ASSERT(arraysize(g_max_sockets_per_proxy_server) ==
                    HttpNetworkSession::NUM_SOCKET_POOL_TYPES,
                max_sockets_per_proxy_server_length_mismatch);
 
-// The meat of the implementation for the InitSocketHandleForHttpRequest,
-// InitSocketHandleForRawConnect and PreconnectSocketsForHttpRequest methods.
-int InitSocketPoolHelper(const GURL& request_url,
-                         const HttpRequestHeaders& request_extra_headers,
-                         int request_load_flags,
-                         RequestPriority request_priority,
-                         HttpNetworkSession* session,
-                         const ProxyInfo& proxy_info,
-                         bool force_spdy_over_ssl,
-                         bool want_spdy_over_npn,
-                         const SSLConfig& ssl_config_for_origin,
-                         const SSLConfig& ssl_config_for_proxy,
-                         bool force_tunnel,
-                         const BoundNetLog& net_log,
-                         int num_preconnect_streams,
-                         ClientSocketHandle* socket_handle,
-                         const OnHostResolutionCallback& resolution_callback,
-                         const CompletionCallback& callback) {
-  scoped_refptr<TransportSocketParams> tcp_params;
-  scoped_refptr<HttpProxySocketParams> http_proxy_params;
-  scoped_refptr<SOCKSSocketParams> socks_params;
-  scoped_ptr<HostPortPair> proxy_host_port;
-
-  bool using_ssl = request_url.SchemeIs("https") || force_spdy_over_ssl;
-
-  HostPortPair origin_host_port =
-      HostPortPair(request_url.HostNoBrackets(),
-                   request_url.EffectiveIntPort());
-
-  if (!using_ssl && session->params().testing_fixed_http_port != 0) {
-    origin_host_port.set_port(session->params().testing_fixed_http_port);
-  } else if (using_ssl && session->params().testing_fixed_https_port != 0) {
-    origin_host_port.set_port(session->params().testing_fixed_https_port);
-  }
-
-  bool disable_resolver_cache =
-      request_load_flags & LOAD_BYPASS_CACHE ||
-      request_load_flags & LOAD_VALIDATE_CACHE ||
-      request_load_flags & LOAD_DISABLE_CACHE;
-
-  int load_flags = request_load_flags;
-  if (session->params().ignore_certificate_errors)
-    load_flags |= LOAD_IGNORE_ALL_CERT_ERRORS;
-
+std::string ConnectionGroupForUrl(const GURL& request_url,
+                                  bool force_ssl,
+                                  const HostPortPair& host_port,
+                                  uint16 ssl_version_max,
+                                  ProxyServer::Scheme proxy_scheme) {
   // Build the string used to uniquely identify connections of this type.
   // Determine the host and port to connect to.
-  std::string connection_group = origin_host_port.ToString();
+  std::string connection_group = host_port.ToString();
   DCHECK(!connection_group.empty());
   if (request_url.SchemeIs("ftp")) {
-    // Combining FTP with forced SPDY over SSL would be a "path to madness".
-    // Make sure we never do that.
-    DCHECK(!using_ssl);
     connection_group = "ftp/" + connection_group;
-  }
-  if (using_ssl) {
+  } else if (force_ssl || request_url.SchemeIs("https")) {
     // All connections in a group should use the same SSLConfig settings.
     // Encode version_max in the connection group's name, unless it's the
     // default version_max. (We want the common case to use the shortest
@@ -129,9 +86,9 @@ int InitSocketPoolHelper(const GURL& request_url,
     // should be the same for all connections, whereas version_max may
     // change for version fallbacks.
     std::string prefix = "ssl/";
-    if (ssl_config_for_origin.version_max !=
+    if (ssl_version_max !=
         SSLConfigService::default_version_max()) {
-      switch (ssl_config_for_origin.version_max) {
+      switch (ssl_version_max) {
         case SSL_PROTOCOL_VERSION_TLS1_2:
           prefix = "ssl(max:3.3)/";
           break;
@@ -152,150 +109,251 @@ int InitSocketPoolHelper(const GURL& request_url,
     connection_group = prefix + connection_group;
   }
 
-  bool ignore_limits = (request_load_flags & LOAD_IGNORE_LIMITS) != 0;
+  char socks_version = 0;
+  if (proxy_scheme == ProxyServer::SCHEME_SOCKS4)
+    socks_version = '4';
+  else if (proxy_scheme == ProxyServer::SCHEME_SOCKS5)
+    socks_version = '5';
+
+  if (socks_version)
+    connection_group = base::StringPrintf("socks%c/%s", socks_version,
+                                          connection_group.c_str());
+
+  return connection_group;
+}
+
+HostPortPair HostPortPairFromUrlForSession(const GURL& request_url,
+                                           bool using_ssl,
+                                           HttpNetworkSession* session) {
+  HostPortPair host_port(request_url.HostNoBrackets(),
+                         request_url.EffectiveIntPort());
+
+  if (!using_ssl && session->params().testing_fixed_http_port != 0) {
+    host_port.set_port(session->params().testing_fixed_http_port);
+  } else if (using_ssl && session->params().testing_fixed_https_port != 0) {
+    host_port.set_port(session->params().testing_fixed_https_port);
+  }
+
+  return host_port;
+}
+
+template<typename Callback>
+int
+InvokeWithSocketParamsAndPool(
+    const GURL& request_url,
+    const HttpRequestHeaders& request_extra_headers,
+    int request_load_flags,
+    RequestPriority request_priority,
+    HttpNetworkSession* session,
+    const ProxyInfo& proxy_info,
+    bool force_spdy_over_ssl,
+    bool want_spdy_over_npn,
+    const SSLConfig& ssl_config_for_origin,
+    const SSLConfig& ssl_config_for_proxy,
+    bool force_tunnel,
+    const OnHostResolutionCallback& resolution_callback,
+    const Callback& callback) {
+  bool using_ssl = request_url.SchemeIs("https") || force_spdy_over_ssl;
+  // Combining FTP with forced SPDY over SSL would be a "path to madness".
+  // Make sure we never do that.
+  DCHECK(!(using_ssl && request_url.SchemeIs("ftp")));
+
+  HostPortPair origin_host_port =
+      HostPortPairFromUrlForSession(request_url, using_ssl, session);
+
+  ProxyServer::Scheme proxy_scheme = proxy_info.proxy_server().scheme();
+  std::string connection_group =
+      ConnectionGroupForUrl(request_url, force_spdy_over_ssl, origin_host_port,
+                            using_ssl ? ssl_config_for_origin.version_max : 0,
+                            proxy_scheme);
+
+  scoped_ptr<HostPortPair> proxy_host_port;
+  HostPortPair* target_host_port = NULL;
   if (proxy_info.is_direct()) {
-    tcp_params = new TransportSocketParams(origin_host_port,
-                                           request_priority,
-                                           disable_resolver_cache,
-                                           ignore_limits,
-                                           resolution_callback);
+    target_host_port = &origin_host_port;
   } else {
-    ProxyServer proxy_server = proxy_info.proxy_server();
-    proxy_host_port.reset(new HostPortPair(proxy_server.host_port_pair()));
-    scoped_refptr<TransportSocketParams> proxy_tcp_params(
-        new TransportSocketParams(*proxy_host_port,
-                                  request_priority,
-                                  disable_resolver_cache,
-                                  ignore_limits,
-                                  resolution_callback));
-
-    if (proxy_info.is_http() || proxy_info.is_https()) {
-      std::string user_agent;
-      request_extra_headers.GetHeader(HttpRequestHeaders::kUserAgent,
-                                      &user_agent);
-      scoped_refptr<SSLSocketParams> ssl_params;
-      if (proxy_info.is_https()) {
-        // Set ssl_params, and unset proxy_tcp_params
-        ssl_params = new SSLSocketParams(proxy_tcp_params,
-                                         NULL,
-                                         NULL,
-                                         ProxyServer::SCHEME_DIRECT,
-                                         *proxy_host_port.get(),
-                                         ssl_config_for_proxy,
-                                         load_flags,
-                                         force_spdy_over_ssl,
-                                         want_spdy_over_npn);
-        proxy_tcp_params = NULL;
-      }
-
-      http_proxy_params =
-          new HttpProxySocketParams(proxy_tcp_params,
-                                    ssl_params,
-                                    request_url,
-                                    user_agent,
-                                    origin_host_port,
-                                    session->http_auth_cache(),
-                                    session->http_auth_handler_factory(),
-                                    session->spdy_session_pool(),
-                                    force_tunnel || using_ssl);
-    } else {
-      DCHECK(proxy_info.is_socks());
-      char socks_version;
-      if (proxy_server.scheme() == ProxyServer::SCHEME_SOCKS5)
-        socks_version = '5';
-      else
-        socks_version = '4';
-      connection_group = base::StringPrintf(
-          "socks%c/%s", socks_version, connection_group.c_str());
-
-      socks_params = new SOCKSSocketParams(proxy_tcp_params,
-                                           socks_version == '5',
-                                           origin_host_port,
-                                           request_priority);
-    }
+    proxy_host_port.reset(
+        new HostPortPair(proxy_info.proxy_server().host_port_pair()));
+    target_host_port = proxy_host_port.get();
   }
 
-  // Deal with SSL - which layers on top of any given proxy.
+  SSLClientSocketPool* ssl_pool = NULL;
   if (using_ssl) {
-    scoped_refptr<SSLSocketParams> ssl_params =
-        new SSLSocketParams(tcp_params,
-                            socks_params,
-                            http_proxy_params,
-                            proxy_info.proxy_server().scheme(),
-                            origin_host_port,
-                            ssl_config_for_origin,
-                            load_flags,
-                            force_spdy_over_ssl,
-                            want_spdy_over_npn);
-    SSLClientSocketPool* ssl_pool = NULL;
-    if (proxy_info.is_direct()) {
+    if (proxy_info.is_direct())
       ssl_pool = session->GetSSLSocketPool(
           HttpNetworkSession::NORMAL_SOCKET_POOL);
-    } else {
+    else
       ssl_pool = session->GetSocketPoolForSSLWithProxy(
           HttpNetworkSession::NORMAL_SOCKET_POOL,
           *proxy_host_port);
-    }
+  }
 
-    if (num_preconnect_streams) {
-      RequestSocketsForPool(ssl_pool, connection_group, ssl_params,
-                            num_preconnect_streams, net_log);
-      return OK;
-    }
+  int load_flags = request_load_flags;
+  if (session->params().ignore_certificate_errors)
+    load_flags |= LOAD_IGNORE_ALL_CERT_ERRORS;
 
-    return socket_handle->Init(connection_group, ssl_params,
-                               request_priority, callback, ssl_pool,
-                               net_log);
+  scoped_refptr<TransportSocketParams> tcp_params;
+  {
+    bool disable_resolver_cache =
+        request_load_flags & LOAD_BYPASS_CACHE ||
+        request_load_flags & LOAD_VALIDATE_CACHE ||
+        request_load_flags & LOAD_DISABLE_CACHE;
+    bool ignore_limits = (request_load_flags & LOAD_IGNORE_LIMITS) != 0;
+    tcp_params = new TransportSocketParams(*target_host_port,
+                                           request_priority,
+                                           disable_resolver_cache,
+                                           ignore_limits,
+                                           resolution_callback);
   }
 
-  // Finally, get the connection started.
-
-  if (proxy_info.is_http() || proxy_info.is_https()) {
-    HttpProxyClientSocketPool* pool =
-        session->GetSocketPoolForHTTPProxy(
-            HttpNetworkSession::NORMAL_SOCKET_POOL,
-            *proxy_host_port);
-    if (num_preconnect_streams) {
-      RequestSocketsForPool(pool, connection_group, http_proxy_params,
-                            num_preconnect_streams, net_log);
-      return OK;
+  if (proxy_info.is_direct()) {
+    if (using_ssl) {
+      scoped_refptr<SSLSocketParams> ssl_params =
+          SSLSocketParams::CreateForDirectConnection(
+              tcp_params,
+              origin_host_port,
+              ssl_config_for_origin,
+              load_flags,
+              force_spdy_over_ssl,
+              want_spdy_over_npn);
+
+      return callback(ssl_pool, connection_group, ssl_params, request_priority);
+    } else {
+      TransportClientSocketPool* pool = session->GetTransportSocketPool(
+          HttpNetworkSession::NORMAL_SOCKET_POOL);
+
+      return callback(pool, connection_group, tcp_params, request_priority);
+    }
+  } else if (proxy_info.is_http() || proxy_info.is_https()) {
+    std::string user_agent;
+    request_extra_headers.GetHeader(HttpRequestHeaders::kUserAgent,
+                                    &user_agent);
+    scoped_refptr<SSLSocketParams> ssl_params;
+    if (proxy_info.is_https()) {
+      ssl_params = SSLSocketParams::CreateForDirectConnection(
+          tcp_params,
+          *proxy_host_port.get(),
+          ssl_config_for_proxy,
+          load_flags,
+          force_spdy_over_ssl,
+          want_spdy_over_npn);
     }
 
-    return socket_handle->Init(connection_group, http_proxy_params,
-                               request_priority, callback,
-                               pool, net_log);
-  }
+    scoped_refptr<HttpProxySocketParams> http_proxy_params =
+        new HttpProxySocketParams(proxy_info.is_https() ? NULL : tcp_params,
+                                  ssl_params,
+                                  request_url,
+                                  user_agent,
+                                  origin_host_port,
+                                  session->http_auth_cache(),
+                                  session->http_auth_handler_factory(),
+                                  session->spdy_session_pool(),
+                                  force_tunnel || using_ssl);
+
+    if (using_ssl) {
+      scoped_refptr<SSLSocketParams> ssl_params =
+          SSLSocketParams::CreateForHttpProxy(
+              http_proxy_params,
+              proxy_scheme,
+              origin_host_port,
+              ssl_config_for_origin,
+              load_flags,
+              force_spdy_over_ssl,
+              want_spdy_over_npn);
+
+      return callback(ssl_pool, connection_group, ssl_params, request_priority);
+    } else {
+      HttpProxyClientSocketPool* pool =
+          session->GetSocketPoolForHTTPProxy(
+              HttpNetworkSession::NORMAL_SOCKET_POOL,
+              *proxy_host_port);
+
+      return callback(pool, connection_group,
+                      http_proxy_params, request_priority);
+    }
+  } else {
+    DCHECK(proxy_info.is_socks());
+
+    scoped_refptr<SOCKSSocketParams> socks_params =
+        new SOCKSSocketParams(tcp_params,
+                              proxy_scheme == ProxyServer::SCHEME_SOCKS5,
+                              origin_host_port,
+                              request_priority);
+
+    if (using_ssl) {
+      scoped_refptr<SSLSocketParams> ssl_params =
+          SSLSocketParams::CreateForSocksProxy(
+              socks_params,
+              proxy_scheme,
+              origin_host_port,
+              ssl_config_for_origin,
+              load_flags,
+              force_spdy_over_ssl,
+              want_spdy_over_npn);
+
+      return callback(ssl_pool, connection_group, ssl_params, request_priority);
+    } else {
+      SOCKSClientSocketPool* pool = session->GetSocketPoolForSOCKSProxy(
+          HttpNetworkSession::NORMAL_SOCKET_POOL,
+          *proxy_host_port);
 
-  if (proxy_info.is_socks()) {
-    SOCKSClientSocketPool* pool =
-        session->GetSocketPoolForSOCKSProxy(
-            HttpNetworkSession::NORMAL_SOCKET_POOL,
-            *proxy_host_port);
-    if (num_preconnect_streams) {
-      RequestSocketsForPool(pool, connection_group, socks_params,
-                            num_preconnect_streams, net_log);
-      return OK;
+      return callback(pool, connection_group, socks_params, request_priority);
     }
+  }
+
+  NOTREACHED();
+  return ERR_UNEXPECTED;
+}
 
-    return socket_handle->Init(connection_group, socks_params,
-                               request_priority, callback, pool,
-                               net_log);
+class RequestSocketForPool {

[mmenke, 2013/05/01 19:31:12]

I think that these are sufficiently ugly that we should just get rid of them. 
Creating a single templated function that takes both sets of parameters, and
figures out which to use, works.  It isn't pretty, but the nested templates like
this just seems to ugly to me.

In the other CL, I suggested another way of doing this that avoids the need for
the callback, and is less prone to regress.

I'm also not convinced that we gain a whole lot by measuring at the network
level instead of the predictor level.  I think that histograming there would get
us much more important / useful data (If the potentially preconnected site was
visited).  I don't think the number of preconnects that we would have used for
each motivation is at all useful without that knowledge, and I don't think it
adds a whole lot of value on top of that knowledge, either.

I also think it wouldn't be unreasonable to improve our histograms a bit and
just enable preconnecting in the new cases on Canary, individually, and keep an
eye on the histograms.  The network histograms I suggest (May or may not already
be present):  How often a request can be served by an existing socket (Either
one already connected, or one currently being connected), how long requests have
to wait for idle sockets, and how many sockets we connect but never use.  Higher
level histograms, like time per request, may also be worth looking at, but tend
to be very noisy.

If you disagree, I'm fine with a temporary load flag, though I don't want to
keep it around too long.

+public:
+  RequestSocketForPool(ClientSocketHandle* socket_handle,
+                      const CompletionCallback& callback,
+                      const BoundNetLog& net_log)
+    : socket_handle_(socket_handle),
+      callback_(callback),
+      net_log_(net_log) {}
+
+  template <typename PoolType, typename SocketParams>
+  int operator()(PoolType* pool,
+                 const std::string& group_name,
+                 const scoped_refptr<SocketParams>& params,
+                 RequestPriority request_priority) const {
+    CheckIsValidSocketParamsForPool<PoolType, SocketParams>();
+
+    return socket_handle_->Init(group_name, params, request_priority,
+                                callback_, pool, net_log_);
   }
 
-  DCHECK(proxy_info.is_direct());
+private:
+  ClientSocketHandle* socket_handle_;
+  const CompletionCallback& callback_;
+  const BoundNetLog& net_log_;
+};
 
-  TransportClientSocketPool* pool =
-      session->GetTransportSocketPool(HttpNetworkSession::NORMAL_SOCKET_POOL);
-  if (num_preconnect_streams) {
-    RequestSocketsForPool(pool, connection_group, tcp_params,
-                          num_preconnect_streams, net_log);
+class RequestSocketsForPool {
+public:
+  RequestSocketsForPool(int num_preconnect_streams,
+                        const BoundNetLog& net_log)
+    : num_preconnect_streams_(num_preconnect_streams),
+      net_log_(net_log) {}
+
+  template <typename PoolType, typename SocketParams>
+  int operator()(PoolType* pool,
+                 const std::string& group_name,
+                 const scoped_refptr<SocketParams>& params,
+                 RequestPriority request_priority) const {
+    CheckIsValidSocketParamsForPool<PoolType, SocketParams>();
+
+    pool->RequestSockets(
+        group_name, &params, num_preconnect_streams_, net_log_);
     return OK;
   }
 
-  return socket_handle->Init(connection_group, tcp_params,
-                             request_priority, callback,
-                             pool, net_log);
-}
+private:
+  int num_preconnect_streams_;
+  const BoundNetLog& net_log_;
+};
 
 }  // namespace
 
@@ -380,11 +438,14 @@ int InitSocketHandleForHttpRequest(
     const OnHostResolutionCallback& resolution_callback,
     const CompletionCallback& callback) {
   DCHECK(socket_handle);
-  return InitSocketPoolHelper(
+
+  RequestSocketForPool socketRequest(socket_handle, callback, net_log);
+
+  return InvokeWithSocketParamsAndPool(
       request_url, request_extra_headers, request_load_flags, request_priority,
       session, proxy_info, force_spdy_over_ssl, want_spdy_over_npn,
-      ssl_config_for_origin, ssl_config_for_proxy, false, net_log, 0,
-      socket_handle, resolution_callback, callback);
+      ssl_config_for_origin, ssl_config_for_proxy, false,
+      resolution_callback, socketRequest);
 }
 
 int InitSocketHandleForRawConnect(
@@ -397,17 +458,20 @@ int InitSocketHandleForRawConnect(
     ClientSocketHandle* socket_handle,
     const CompletionCallback& callback) {
   DCHECK(socket_handle);
+
+  RequestSocketForPool socketRequest(socket_handle, callback, net_log);
+
   // Synthesize an HttpRequestInfo.
   GURL request_url = GURL("http://" + host_port_pair.ToString());
   HttpRequestHeaders request_extra_headers;
   int request_load_flags = 0;
   RequestPriority request_priority = MEDIUM;
 
-  return InitSocketPoolHelper(
+  return InvokeWithSocketParamsAndPool(
       request_url, request_extra_headers, request_load_flags, request_priority,
-      session, proxy_info, false, false, ssl_config_for_origin,
-      ssl_config_for_proxy, true, net_log, 0, socket_handle,
-      OnHostResolutionCallback(), callback);
+      session, proxy_info, false, false,
+      ssl_config_for_origin, ssl_config_for_proxy, true,
+      OnHostResolutionCallback(), socketRequest);
 }
 
 int PreconnectSocketsForHttpRequest(
@@ -423,12 +487,13 @@ int PreconnectSocketsForHttpRequest(
     const SSLConfig& ssl_config_for_proxy,
     const BoundNetLog& net_log,
     int num_preconnect_streams) {
-  return InitSocketPoolHelper(
+  RequestSocketsForPool socketsRequest(num_preconnect_streams, net_log);
+
+  return InvokeWithSocketParamsAndPool(
       request_url, request_extra_headers, request_load_flags, request_priority,
       session, proxy_info, force_spdy_over_ssl, want_spdy_over_npn,
-      ssl_config_for_origin, ssl_config_for_proxy, false, net_log,
-      num_preconnect_streams, NULL, OnHostResolutionCallback(),
-      CompletionCallback());
+      ssl_config_for_origin, ssl_config_for_proxy, false,
+      OnHostResolutionCallback(), socketsRequest);
 }
 
 }  // namespace net