Plumb NQP to context and to http_proxy_client_socket_pool

net/http/http_proxy_client_socket_pool.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 "net/http/http_proxy_client_socket_pool.h"
 
 #include <algorithm>
 #include <utility>
 
 #include "base/compiler_specific.h"
@@ skipping 14 matching lines @@
 #include "net/socket/transport_client_socket_pool.h"
 #include "net/spdy/chromium/spdy_proxy_client_socket.h"
 #include "net/spdy/chromium/spdy_session.h"
 #include "net/spdy/chromium/spdy_session_pool.h"
 #include "net/spdy/chromium/spdy_stream.h"
 #include "net/ssl/ssl_cert_request_info.h"
 #include "url/gurl.h"
 
 namespace net {
 
+namespace {
+
+// HttpProxyConnectJobs will time out after this many seconds.  Note this is on
+// top of the timeout for the transport socket.
+// TODO(kundaji): Proxy connect timeout should be independent of platform and be
+// based on proxy. Bug http://crbug.com/407446.
+#if defined(OS_ANDROID) || defined(OS_IOS)
+static const int kHttpProxyConnectJobTimeoutInSeconds = 10;
+#else
+static const int kHttpProxyConnectJobTimeoutInSeconds = 30;
+#endif
+
+// Returns the default proxy connection timeout.
+base::TimeDelta GetDefaultProxyConnectionTimeout(
+    TransportClientSocketPool* transport_pool,
+    SSLClientSocketPool* ssl_pool) {
+  base::TimeDelta max_pool_timeout = base::TimeDelta();
+
+#if (!defined(OS_ANDROID) && !defined(OS_IOS))
+  if (transport_pool)
+    max_pool_timeout = transport_pool->ConnectionTimeout();
+  if (ssl_pool) {
+    max_pool_timeout =
+        std::max(max_pool_timeout, ssl_pool->ConnectionTimeout());
+  }
+#endif  // !defined(OS_ANDROID) && !defined(OS_IOS)
+
+  return max_pool_timeout +
+         base::TimeDelta::FromSeconds(kHttpProxyConnectJobTimeoutInSeconds);
+}
+
+}  // namespace
+
 HttpProxySocketParams::HttpProxySocketParams(
     const scoped_refptr<TransportSocketParams>& transport_params,
     const scoped_refptr<SSLSocketParams>& ssl_params,
     const std::string& user_agent,
     const HostPortPair& endpoint,
     HttpAuthCache* http_auth_cache,
     HttpAuthHandlerFactory* http_auth_handler_factory,
     SpdySessionPool* spdy_session_pool,
     bool tunnel,
     ProxyDelegate* proxy_delegate)
@@ skipping 13 matching lines @@
 const HostResolver::RequestInfo& HttpProxySocketParams::destination() const {
   if (transport_params_.get() == NULL) {
     return ssl_params_->GetDirectConnectionParams()->destination();
   } else {
     return transport_params_->destination();
   }
 }
 
 HttpProxySocketParams::~HttpProxySocketParams() {}
 
-// HttpProxyConnectJobs will time out after this many seconds.  Note this is on
-// top of the timeout for the transport socket.
-// TODO(kundaji): Proxy connect timeout should be independent of platform and be
-// based on proxy. Bug http://crbug.com/407446.
-#if defined(OS_ANDROID) || defined(OS_IOS)
-static const int kHttpProxyConnectJobTimeoutInSeconds = 10;
-#else
-static const int kHttpProxyConnectJobTimeoutInSeconds = 30;
-#endif
-
 HttpProxyConnectJob::HttpProxyConnectJob(
     const std::string& group_name,
     RequestPriority priority,
     ClientSocketPool::RespectLimits respect_limits,
     const scoped_refptr<HttpProxySocketParams>& params,
     const base::TimeDelta& timeout_duration,
     TransportClientSocketPool* transport_pool,
     SSLClientSocketPool* ssl_pool,
     Delegate* delegate,
     NetLog* net_log)
@@ skipping 54 matching lines @@
   if (result == ERR_SSL_CLIENT_AUTH_CERT_NEEDED)
     error_response_info_ = client_socket_->GetAdditionalErrorState();
 
   if (result == OK || result == ERR_PROXY_AUTH_REQUESTED ||
       result == ERR_HTTPS_PROXY_TUNNEL_RESPONSE) {
     SetSocket(std::move(client_socket_));
   }
   return result;
 }
 
-HttpProxyClientSocketPool::
-HttpProxyConnectJobFactory::HttpProxyConnectJobFactory(
-    TransportClientSocketPool* transport_pool,
-    SSLClientSocketPool* ssl_pool,
-    NetLog* net_log)
+HttpProxyClientSocketPool::HttpProxyConnectJobFactory::
+    HttpProxyConnectJobFactory(TransportClientSocketPool* transport_pool,
+                               SSLClientSocketPool* ssl_pool,
+                               NetworkQualityProvider* network_quality_provider,
+                               NetLog* net_log)
     : transport_pool_(transport_pool),
       ssl_pool_(ssl_pool),
-      net_log_(net_log) {
-  base::TimeDelta max_pool_timeout = base::TimeDelta();
-
-// TODO(kundaji): Proxy connect timeout should be independent of platform and be
-// based on proxy. Bug http://crbug.com/407446.
-#if (defined(OS_ANDROID) || defined(OS_IOS))
-#else
-  if (transport_pool_)
-    max_pool_timeout = transport_pool_->ConnectionTimeout();
-  if (ssl_pool_)
-    max_pool_timeout = std::max(max_pool_timeout,
-                                ssl_pool_->ConnectionTimeout());
-#endif
-  timeout_ = max_pool_timeout +
-    base::TimeDelta::FromSeconds(kHttpProxyConnectJobTimeoutInSeconds);
-}
+      network_quality_provider_(network_quality_provider),
+      net_log_(net_log) {}
 
 std::unique_ptr<ConnectJob>
 HttpProxyClientSocketPool::HttpProxyConnectJobFactory::NewConnectJob(
     const std::string& group_name,
     const PoolBase::Request& request,
     ConnectJob::Delegate* delegate) const {
   return std::unique_ptr<ConnectJob>(new HttpProxyConnectJob(
       group_name, request.priority(), request.respect_limits(),
       request.params(), ConnectionTimeout(), transport_pool_, ssl_pool_,
       delegate, net_log_));
 }
 
 base::TimeDelta
 HttpProxyClientSocketPool::HttpProxyConnectJobFactory::ConnectionTimeout(
     ) const {
-  return timeout_;
+  // TODO(tbansal): https://crbug.com/704339. Use |network_quality_provider_|
+  // and field trial to determine the connection timeout.
+  // Silence unused variable warning.

[mmenke, 2017/06/08 17:53:02]

Could we just pass it in to GetDefaultProxyConnectionTimeout instead, and rename
GetDefaultProxyConnectionTimeout to GetProxyConnectionTimeout?  I don't think we
still need a default/  (I mean, we still need behavior when we don't have enough
data to estimate the network performance, but seems like that logic should just
be in the same method as everything else)

[tbansal1, 2017/06/08 19:59:14]

Ok, then I am keeping everything here, and removing the method from the
anonymous namespace. It does not seem useful to have 2 methods where the class
method is simply calling the anonymous method.

[mmenke, 2017/06/08 20:02:10]

SGTM.

+  (void)network_quality_provider_;
+  return GetDefaultProxyConnectionTimeout(transport_pool_, ssl_pool_);
 }
 
 HttpProxyClientSocketPool::HttpProxyClientSocketPool(
     int max_sockets,
     int max_sockets_per_group,
     TransportClientSocketPool* transport_pool,
     SSLClientSocketPool* ssl_pool,
+    NetworkQualityProvider* network_quality_provider,
     NetLog* net_log)
     : transport_pool_(transport_pool),
       ssl_pool_(ssl_pool),
       base_(this,
             max_sockets,
             max_sockets_per_group,
             ClientSocketPool::unused_idle_socket_timeout(),
             ClientSocketPool::used_idle_socket_timeout(),
-            new HttpProxyConnectJobFactory(transport_pool, ssl_pool, net_log)) {
+            new HttpProxyConnectJobFactory(transport_pool,
+                                           ssl_pool,
+                                           network_quality_provider,
+                                           net_log)) {
   // We should always have a |transport_pool_| except in unit tests.
   if (transport_pool_)
     base_.AddLowerLayeredPool(transport_pool_);
   if (ssl_pool_)
     base_.AddLowerLayeredPool(ssl_pool_);
 }
 
 HttpProxyClientSocketPool::~HttpProxyClientSocketPool() {
 }
 
@@ skipping 108 matching lines @@
   base_.RemoveHigherLayeredPool(higher_pool);
 }
 
 bool HttpProxyClientSocketPool::CloseOneIdleConnection() {
   if (base_.CloseOneIdleSocket())
     return true;
   return base_.CloseOneIdleConnectionInHigherLayeredPool();
 }
 
 }  // namespace net