Cleanup the preconnect to proxy code and Job controller code

(1) Add a boolean |restrict_to_one_preconnect_for_proxies| to network
session params. This allows us to run multiple experiments using a single
field trial. Next CL will add the variation param
|race_preconnects_to_proxies| to experiment with racing the alternative
and main jobs for proxy preconnects.

(2) Remove an extra parameter in
HttpStreamFactoryImpl::Job::Delegate::OnStreamReady() method.

BUG=667471,671291
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2600943002
Cr-Commit-Position: refs/heads/master@{#440893}

components/network_session_configurator/network_session_configurator.cc

@@ -396,6 +396,14 @@ void ConfigureQuicParams(base::StringPiece quic_trial_group,
   }
 }
 
+void ConfigureOptimizePreconnectsToProxiesParams(
+    const std::map<std::string, std::string>& proxy_preconnects_trial_params,
+    net::HttpNetworkSession::Params* params) {
+  params->restrict_to_one_preconnect_for_proxies =
+      GetVariationParam(proxy_preconnects_trial_params,
+                        "restrict_to_one_preconnect_for_proxies") == "true";
+}
+
 }  // anonymous namespace
 
 namespace network_session_configurator {
@@ -436,6 +444,12 @@ void ParseFieldTrials(bool is_quic_force_disabled,
   const std::string tfo_trial_group =
       base::FieldTrialList::FindFullName(kTCPFastOpenFieldTrialName);
   ConfigureTCPFastOpenParams(tfo_trial_group, params);
+
+  std::map<std::string, std::string> proxy_preconnects_trial_params;
+  variations::GetVariationParams("NetProxyPreconnects",
+                                 &proxy_preconnects_trial_params);
+  ConfigureOptimizePreconnectsToProxiesParams(proxy_preconnects_trial_params,
+                                              params);
 }
 
 }  // namespace network_session_configurator

net/http/http_network_session.cc

@@ -155,7 +155,8 @@ HttpNetworkSession::Params::Params()
       quic_do_not_fragment(false),
       proxy_delegate(NULL),
       enable_token_binding(false),
-      http_09_on_non_default_ports_enabled(false) {
+      http_09_on_non_default_ports_enabled(false),
+      restrict_to_one_preconnect_for_proxies(false) {
   quic_supported_versions.push_back(QUIC_VERSION_35);
 }
 

net/http/http_network_session.h

@@ -203,6 +203,10 @@ class NET_EXPORT HttpNetworkSession
     // Enable HTTP/0.9 for HTTP/HTTPS on ports other than the default one for
     // each protocol.
     bool http_09_on_non_default_ports_enabled;
+
+    // If true, only one pending preconnect is allowed to proxies that support
+    // request priorities.
+    bool restrict_to_one_preconnect_for_proxies;
   };
 
   enum SocketPoolType {

net/http/http_stream_factory_impl.cc

@@ -8,7 +8,6 @@
 
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
-#include "base/metrics/field_trial.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/stl_util.h"
 #include "base/strings/string_util.h"
@@ -89,23 +88,13 @@ class DefaultJobFactory : public HttpStreamFactoryImpl::JobFactory {
   }
 };
 
-// Returns true if only one preconnect to proxy servers is allowed via field
-// trial.
-bool AllowOnlyOnePreconnectToProxyServers() {
-  return base::StartsWith(base::FieldTrialList::FindFullName(
-                              "NetAllowOnlyOnePreconnectToProxyServers"),
-                          "Enabled", base::CompareCase::INSENSITIVE_ASCII);
-}
-
 }  // anonymous namespace
 
 HttpStreamFactoryImpl::HttpStreamFactoryImpl(HttpNetworkSession* session,
                                              bool for_websockets)
     : session_(session),
       job_factory_(new DefaultJobFactory()),
-      for_websockets_(for_websockets),
-      allow_only_one_preconnect_to_proxy_servers_(
-          AllowOnlyOnePreconnectToProxyServers()) {}
+      for_websockets_(for_websockets) {}
 
 HttpStreamFactoryImpl::~HttpStreamFactoryImpl() {
   DCHECK(request_map_.empty());
@@ -261,7 +250,7 @@ bool HttpStreamFactoryImpl::OnInitConnection(const JobController& controller,
     return false;
   }
 
-  if (!allow_only_one_preconnect_to_proxy_servers_ ||
+  if (!session_->params().restrict_to_one_preconnect_for_proxies ||
       !ProxyServerSupportsPriorities(proxy_info)) {
     return false;
   }

net/http/http_stream_factory_impl.h

@@ -168,10 +168,6 @@ class NET_EXPORT_PRIVATE HttpStreamFactoryImpl : public HttpStreamFactory {
 
   const bool for_websockets_;
 
-  // True if only one preconnect is allowed to proxy servers that support
-  // request priorities.
-  const bool allow_only_one_preconnect_to_proxy_servers_;
-
   DISALLOW_COPY_AND_ASSIGN(HttpStreamFactoryImpl);
 };
 

net/http/http_stream_factory_impl_job.cc

@@ -386,7 +386,7 @@ void HttpStreamFactoryImpl::Job::OnStreamReadyCallback() {
 
   MaybeCopyConnectionAttemptsFromSocketOrHandle();
 
-  delegate_->OnStreamReady(this, server_ssl_config_, proxy_info_);
+  delegate_->OnStreamReady(this, server_ssl_config_);
   // |this| may be deleted after this call.
 }
 

net/http/http_stream_factory_impl_job.h

@@ -51,9 +51,7 @@ class HttpStreamFactoryImpl::Job {
     virtual ~Delegate() {}
 
     // Invoked when |job| has an HttpStream ready.
-    virtual void OnStreamReady(Job* job,
-                               const SSLConfig& used_ssl_config,
-                               const ProxyInfo& used_proxy_info) = 0;
+    virtual void OnStreamReady(Job* job, const SSLConfig& used_ssl_config) = 0;
 
     // Invoked when |job| has a BidirectionalStream ready.
     virtual void OnBidirectionalStreamImplReady(

net/http/http_stream_factory_impl_job_controller.cc

@@ -172,12 +172,8 @@ void HttpStreamFactoryImpl::JobController::SetPriority(
 
 void HttpStreamFactoryImpl::JobController::OnStreamReady(
     Job* job,
-    const SSLConfig& used_ssl_config,
-    const ProxyInfo& used_proxy_info) {
+    const SSLConfig& used_ssl_config) {
   DCHECK(job);
-  // TODO(tbansal): Remove |used_proxy_info| from the method arguments.
-  DCHECK(job->proxy_info().is_empty() == used_proxy_info.is_empty() ||
-         job->proxy_info().proxy_server() == used_proxy_info.proxy_server());
 
   factory_->OnStreamReady(job->proxy_info());
 
@@ -197,7 +193,7 @@ void HttpStreamFactoryImpl::JobController::OnStreamReady(
   DCHECK(!factory_->for_websockets_);
   DCHECK_EQ(HttpStreamRequest::HTTP_STREAM, request_->stream_type());
   OnJobSucceeded(job);
-  request_->OnStreamReady(used_ssl_config, used_proxy_info, stream.release());
+  request_->OnStreamReady(used_ssl_config, job->proxy_info(), stream.release());
 }
 
 void HttpStreamFactoryImpl::JobController::OnBidirectionalStreamImplReady(

net/http/http_stream_factory_impl_job_controller.h

@@ -69,9 +69,7 @@ class HttpStreamFactoryImpl::JobController
 
   // From HttpStreamFactoryImpl::Job::Delegate.
   // Invoked when |job| has an HttpStream ready.
-  void OnStreamReady(Job* job,
-                     const SSLConfig& used_ssl_config,
-                     const ProxyInfo& used_proxy_info) override;
+  void OnStreamReady(Job* job, const SSLConfig& used_ssl_config) override;
 
   // Invoked when |job| has a BidirectionalStream ready.
   void OnBidirectionalStreamImplReady(

net/http/http_stream_factory_impl_job_controller_unittest.cc

@@ -239,8 +239,7 @@ TEST_F(HttpStreamFactoryImplJobControllerTest,
 
   EXPECT_CALL(request_delegate_, OnStreamReady(_, _, http_stream))
       .WillOnce(Invoke(DeleteHttpStreamPointer));
-  job_controller_->OnStreamReady(job_factory_.main_job(), SSLConfig(),
-                                 ProxyInfo());
+  job_controller_->OnStreamReady(job_factory_.main_job(), SSLConfig());
 }
 
 // Test we cancel Jobs correctly when the Request is explicitly canceled
@@ -358,8 +357,7 @@ TEST_F(HttpStreamFactoryImplJobControllerTest,
 
   EXPECT_CALL(request_delegate_, OnStreamReady(_, _, http_stream))
       .WillOnce(Invoke(DeleteHttpStreamPointer));
-  job_controller_->OnStreamReady(job_factory_.main_job(), SSLConfig(),
-                                 ProxyInfo());
+  job_controller_->OnStreamReady(job_factory_.main_job(), SSLConfig());
 
   // JobController shouldn't report the status of second job as request
   // is already successfully served.
@@ -413,8 +411,7 @@ TEST_F(HttpStreamFactoryImplJobControllerTest,
 
   EXPECT_CALL(request_delegate_, OnStreamReady(_, _, http_stream))
       .WillOnce(Invoke(DeleteHttpStreamPointer));
-  job_controller_->OnStreamReady(job_factory_.alternative_job(), SSLConfig(),
-                                 ProxyInfo());
+  job_controller_->OnStreamReady(job_factory_.alternative_job(), SSLConfig());
   VerifyBrokenAlternateProtocolMapping(request_info, false);
 }
 
@@ -458,8 +455,7 @@ TEST_F(HttpStreamFactoryImplJobControllerTest,
 
   EXPECT_CALL(request_delegate_, OnStreamReady(_, _, http_stream))
       .WillOnce(Invoke(DeleteHttpStreamPointer));
-  job_controller_->OnStreamReady(job_factory_.main_job(), SSLConfig(),
-                                 ProxyInfo());
+  job_controller_->OnStreamReady(job_factory_.main_job(), SSLConfig());
 
   VerifyBrokenAlternateProtocolMapping(request_info, true);
 }
@@ -509,8 +505,7 @@ TEST_F(HttpStreamFactoryImplJobControllerTest, GetLoadStateAfterMainJobFailed) {
 
   EXPECT_CALL(request_delegate_, OnStreamReady(_, _, http_stream))
       .WillOnce(Invoke(DeleteHttpStreamPointer));
-  job_controller_->OnStreamReady(job_factory_.alternative_job(), SSLConfig(),
-                                 ProxyInfo());
+  job_controller_->OnStreamReady(job_factory_.alternative_job(), SSLConfig());
 }
 
 TEST_F(HttpStreamFactoryImplJobControllerTest, DoNotResumeMainJobBeforeWait) {
@@ -935,8 +930,7 @@ TEST_F(HttpStreamFactoryImplJobControllerTest,
 
   EXPECT_CALL(request_delegate_, OnStreamReady(_, _, http_stream))
       .WillOnce(Invoke(DeleteHttpStreamPointer));
-  job_controller_->OnStreamReady(job_factory_.main_job(), SSLConfig(),
-                                 job_factory_.main_job()->proxy_info());
+  job_controller_->OnStreamReady(job_factory_.main_job(), SSLConfig());
 
   // JobController shouldn't report the status of alternative server job as
   // request is already successfully served.

net/http/http_stream_factory_impl_unittest.cc

@@ -14,7 +14,6 @@
 #include "base/compiler_specific.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
-#include "base/metrics/field_trial.h"
 #include "base/run_loop.h"
 #include "base/test/histogram_tester.h"
 #include "net/base/port_util.h"
@@ -1218,13 +1217,9 @@ TEST_F(HttpStreamFactoryTest, QuicDisablePreConnectIfZeroRtt) {
   }
 }
 
-// Verify that the proxy delegate can disable preconnect jobs to only the proxy
-// servers that support request priorities.
-TEST_F(HttpStreamFactoryTest, ProxyDelegateDisablesPreconnect) {
-  base::FieldTrialList field_trial_list(nullptr);
-  base::FieldTrialList::CreateFieldTrial(
-      "NetAllowOnlyOnePreconnectToProxyServers", "Enabled");
-
+// Verify that only one preconnect job succeeds to a proxy server that supports
+// request priorities.
+TEST_F(HttpStreamFactoryTest, OnlyOnePreconnectToProxyServer) {
   for (bool set_http_server_properties : {false, true}) {
     for (int num_streams = 1; num_streams < 3; ++num_streams) {
       base::HistogramTester histogram_tester;
@@ -1245,6 +1240,7 @@ TEST_F(HttpStreamFactoryTest, ProxyDelegateDisablesPreconnect) {
       params.enable_quic = true;
       params.proxy_service = proxy_service.get();
       params.http_server_properties = &http_server_properties;
+      ASSERT_TRUE(params.restrict_to_one_preconnect_for_proxies);
 
       std::unique_ptr<HttpNetworkSession> session(
           new HttpNetworkSession(params));
@@ -1610,10 +1606,6 @@ TEST_F(HttpStreamFactoryTest, RequestHttpStreamOverProxy) {
 // Verifies that once a stream has been created to a proxy server (that supports
 // request priorities) the next preconnect job can again open new sockets.
 TEST_F(HttpStreamFactoryTest, RequestHttpStreamOverProxyWithPreconnects) {
-  base::FieldTrialList field_trial_list(nullptr);
-  base::FieldTrialList::CreateFieldTrial(
-      "NetAllowOnlyOnePreconnectToProxyServers", "Enabled");
-
   SpdySessionDependencies session_deps(
       ProxyService::CreateFixed("https://myproxy.org:443"));
 

net/spdy/spdy_test_util_common.cc

@@ -402,6 +402,7 @@ HttpNetworkSession::Params SpdySessionDependencies::CreateSessionParams(
   params.net_log = session_deps->net_log;
   params.http_09_on_non_default_ports_enabled =
       session_deps->http_09_on_non_default_ports_enabled;
+  params.restrict_to_one_preconnect_for_proxies = true;
   return params;
 }