HttpStreamFactoryImpl::Job cleanup.

net/http/http_stream_factory_impl_job.cc

Index: net/http/http_stream_factory_impl_job.cc
diff --git a/net/http/http_stream_factory_impl_job.cc b/net/http/http_stream_factory_impl_job.cc
index c97829f54020485fc1c5c6008383ab3a1e2759d9..029536b77bbc19d3b013ad7bfcb4b3e556f96e9f 100644
--- a/net/http/http_stream_factory_impl_job.cc
+++ b/net/http/http_stream_factory_impl_job.cc
@@ -125,7 +125,8 @@ std::unique_ptr<base::Value> NetLogHttpStreamJobCallback(
     const NetLogSource& source,
     const GURL* original_url,
     const GURL* url,
-    const AlternativeService* alternative_service,
+    bool expect_spdy,
+    bool using_quic,
     RequestPriority priority,
     NetLogCaptureMode /* capture_mode */) {
   auto dict = base::MakeUnique<base::DictionaryValue>();
@@ -133,7 +134,8 @@ std::unique_ptr<base::Value> NetLogHttpStreamJobCallback(
     source.AddToEventParameters(dict.get());
   dict->SetString("original_url", original_url->GetOrigin().spec());
   dict->SetString("url", url->GetOrigin().spec());
-  dict->SetString("alternative_service", alternative_service->ToString());
+  dict->SetString("expect_spdy", expect_spdy ? "true" : "false");
+  dict->SetString("using_quic", using_quic ? "true" : "false");
   dict->SetString("priority", RequestPriorityToString(priority));
   return std::move(dict);
 }
@@ -159,34 +161,7 @@ HttpStreamFactoryImpl::Job::Job(Delegate* delegate,
                                 const SSLConfig& proxy_ssl_config,
                                 HostPortPair destination,
                                 GURL origin_url,
-                                bool enable_ip_based_pooling,
-                                NetLog* net_log)
-    : Job(delegate,
-          job_type,
-          session,
-          request_info,
-          priority,
-          proxy_info,
-          server_ssl_config,
-          proxy_ssl_config,
-          destination,
-          origin_url,
-          AlternativeService(),
-          ProxyServer(),
-          enable_ip_based_pooling,
-          net_log) {}
-
-HttpStreamFactoryImpl::Job::Job(Delegate* delegate,
-                                JobType job_type,
-                                HttpNetworkSession* session,
-                                const HttpRequestInfo& request_info,
-                                RequestPriority priority,
-                                const ProxyInfo& proxy_info,
-                                const SSLConfig& server_ssl_config,
-                                const SSLConfig& proxy_ssl_config,
-                                HostPortPair destination,
-                                GURL origin_url,
-                                AlternativeService alternative_service,
+                                NextProto alternative_protocol,
                                 const ProxyServer& alternative_proxy_server,
                                 bool enable_ip_based_pooling,
                                 NetLog* net_log)
@@ -204,15 +179,17 @@ HttpStreamFactoryImpl::Job::Job(Delegate* delegate,
       next_state_(STATE_NONE),
       destination_(destination),
       origin_url_(origin_url),
-      alternative_service_(alternative_service),
       alternative_proxy_server_(alternative_proxy_server),
       enable_ip_based_pooling_(enable_ip_based_pooling),
       delegate_(delegate),
       job_type_(job_type),
       using_ssl_(origin_url_.SchemeIs(url::kHttpsScheme) ||
                  origin_url_.SchemeIs(url::kWssScheme)),
+      using_quic_(
+          alternative_protocol == kProtoQUIC ||
+          ShouldForceQuic(session, destination, origin_url, proxy_info)),
+      expect_spdy_(alternative_protocol == kProtoHTTP2 && !using_quic_),
       using_spdy_(false),
-      using_quic_(false),
       should_reconsider_proxy_(false),
       quic_request_(session_->quic_stream_factory()),
       using_existing_quic_session_(false),
@@ -222,34 +199,34 @@ HttpStreamFactoryImpl::Job::Job(Delegate* delegate,
       num_streams_(0),
       spdy_session_direct_(
           !(proxy_info.is_https() && origin_url_.SchemeIs(url::kHttpScheme))),
-      spdy_session_key_(GetSpdySessionKey()),
+      spdy_session_key_(GetSpdySessionKey(spdy_session_direct_,
+                                          proxy_info_.proxy_server(),
+                                          origin_url_,
+                                          request_info_.privacy_mode)),
       stream_type_(HttpStreamRequest::BIDIRECTIONAL_STREAM),
       ptr_factory_(this) {
   DCHECK(session);
-  // The job can't have alternative service and alternative proxy server set at
-  // the same time since alternative services are used for requests that are
-  // fetched directly, while the alternative proxy server is used for requests
-  // that should be fetched using proxy.
-  DCHECK(alternative_service_.protocol == kProtoUnknown ||
-         !alternative_proxy_server_.is_valid());
-  DCHECK(!alternative_proxy_server_.is_valid() ||
-         !(IsSpdyAlternative() || IsQuicAlternative()));
-  // If either the alternative service protocol is specified or if the
-  // alternative proxy server is valid, then the job type must be set to
-  // either ALTERNATIVE or PRECONNECT.
-  DCHECK((alternative_service_.protocol == kProtoUnknown &&
-          !alternative_proxy_server_.is_valid()) ||
-         (job_type_ == ALTERNATIVE || job_type_ == PRECONNECT));
-  // If the alternative proxy server is valid, then the job type must be
-  // set to ALTERNATIVE.
-  DCHECK(!alternative_proxy_server_.is_valid() || job_type_ == ALTERNATIVE);
-
-  if (IsSpdyAlternative()) {
+  if (alternative_protocol != kProtoUnknown) {
+    // The job cannot have protocol requirements dictated by alternative service
+    // and have an alternative proxy server set at the same time, since
+    // alternative services are used for requests that are fetched directly,
+    // while the alternative proxy server is used for requests that should be
+    // fetched using proxy.
+    DCHECK(!alternative_proxy_server_.is_valid());
+    // If the alternative service protocol is specified, then the job type must
+    // be either ALTERNATIVE or PRECONNECT.
+    DCHECK(job_type_ == ALTERNATIVE || job_type_ == PRECONNECT);
+  }
+  // If the alternative proxy server is set, then the job must be ALTERNATIVE.
+  if (alternative_proxy_server_.is_valid()) {
+    DCHECK(job_type_ == ALTERNATIVE);
+  }
+
+  if (expect_spdy_) {
     DCHECK(origin_url_.SchemeIs(url::kHttpsScheme));
   }
-  if (IsQuicAlternative()) {
+  if (using_quic_) {
     DCHECK(session_->IsQuicEnabled());
-    using_quic_ = true;
   }
 }
 
@@ -396,16 +373,38 @@ void HttpStreamFactoryImpl::Job::GetSSLInfo(SSLInfo* ssl_info) {
   ssl_socket->GetSSLInfo(ssl_info);
 }
 
-SpdySessionKey HttpStreamFactoryImpl::Job::GetSpdySessionKey() const {
+// static
+bool HttpStreamFactoryImpl::Job::ShouldForceQuic(
+    HttpNetworkSession* session,
+    const HostPortPair& destination,
+    GURL origin_url,
+    const ProxyInfo& proxy_info) {
+  if (!session->IsQuicEnabled())
+    return false;
+  if (proxy_info.is_quic())
+    return true;
+  return (base::ContainsKey(session->params().origins_to_force_quic_on,
+                            HostPortPair()) ||
+          base::ContainsKey(session->params().origins_to_force_quic_on,
+                            destination)) &&
+         proxy_info.is_direct() && origin_url.SchemeIs(url::kHttpsScheme);
+}
+
+// static
+SpdySessionKey HttpStreamFactoryImpl::Job::GetSpdySessionKey(
+    bool spdy_session_direct,
+    const ProxyServer& proxy_server,
+    const GURL& origin_url,
+    PrivacyMode privacy_mode) {
   // In the case that we're using an HTTPS proxy for an HTTP url,
   // we look for a SPDY session *to* the proxy, instead of to the
   // origin server.
-  if (!spdy_session_direct_) {
-    return SpdySessionKey(proxy_info_.proxy_server().host_port_pair(),
-                          ProxyServer::Direct(), PRIVACY_MODE_DISABLED);
+  if (!spdy_session_direct) {
+    return SpdySessionKey(proxy_server.host_port_pair(), ProxyServer::Direct(),
+                          PRIVACY_MODE_DISABLED);
   }
-  return SpdySessionKey(HostPortPair::FromURL(origin_url_),
-                        proxy_info_.proxy_server(), request_info_.privacy_mode);
+  return SpdySessionKey(HostPortPair::FromURL(origin_url), proxy_server,
+                        privacy_mode);
 }
 
 bool HttpStreamFactoryImpl::Job::CanUseExistingSpdySession() const {
@@ -459,7 +458,7 @@ void HttpStreamFactoryImpl::Job::OnBidirectionalStreamImplReadyCallback() {
 void HttpStreamFactoryImpl::Job::OnNewSpdySessionReadyCallback() {
   DCHECK(stream_.get() || bidirectional_stream_impl_.get());
   DCHECK_NE(job_type_, PRECONNECT);
-  DCHECK(using_spdy());
+  DCHECK(using_spdy_);
   // Note: an event loop iteration has passed, so |new_spdy_session_| may be
   // NULL at this point if the SpdySession closed immediately after creation.
   base::WeakPtr<SpdySession> spdy_session = new_spdy_session_;
@@ -733,7 +732,7 @@ int HttpStreamFactoryImpl::Job::DoStart() {
     net_log_.BeginEvent(
         NetLogEventType::HTTP_STREAM_JOB,
         base::Bind(&NetLogHttpStreamJobCallback, net_log->source(),
-                   &request_info_.url, &origin_url_, &alternative_service_,
+                   &request_info_.url, &origin_url_, expect_spdy_, using_quic_,
                    priority_));
     net_log->AddEvent(NetLogEventType::HTTP_STREAM_REQUEST_STARTED_JOB,
                       net_log_.source().ToEventParametersCallback());
@@ -749,15 +748,6 @@ int HttpStreamFactoryImpl::Job::DoStart() {
   return OK;
 }
 
-bool HttpStreamFactoryImpl::Job::ShouldForceQuic() const {
-  return session_->IsQuicEnabled() &&
-         (base::ContainsKey(session_->params().origins_to_force_quic_on,
-                            HostPortPair()) ||
-          base::ContainsKey(session_->params().origins_to_force_quic_on,
-                            destination_)) &&
-         proxy_info_.is_direct() && origin_url_.SchemeIs(url::kHttpsScheme);
-}
-
 int HttpStreamFactoryImpl::Job::DoWait() {
   next_state_ = STATE_WAIT_COMPLETE;
   bool should_wait = delegate_->ShouldWait(this);
@@ -806,18 +796,6 @@ int HttpStreamFactoryImpl::Job::DoInitConnectionImpl() {
     return OK;
   }
 
-  using_spdy_ = false;

[Bence, 2017/06/14 17:29:10]

This is redundant with the default value in the initializer list in the
constructor.

-
-  if (ShouldForceQuic())

[Bence, 2017/06/14 17:29:10]

This logic has been moved to the initializer list.

-    using_quic_ = true;
-
-  DCHECK(!using_quic_ || session_->IsQuicEnabled());

[Bence, 2017/06/14 17:29:10]

This DCHECK is in the constructor body.

-
-  if (proxy_info_.is_quic()) {

[Bence, 2017/06/14 17:29:10]

This logic has been moved to ShouldForceQuic().

-    using_quic_ = true;
-    DCHECK(session_->IsQuicEnabled());
-  }
-
   if (proxy_info_.is_https() || proxy_info_.is_quic()) {
     InitSSLConfig(&proxy_ssl_config_, /*is_proxy=*/true);
     // Disable revocation checking for HTTPS proxies since the revocation
@@ -904,8 +882,6 @@ int HttpStreamFactoryImpl::Job::DoInitConnectionImpl() {
   if (proxy_info_.is_http() || proxy_info_.is_https())
     establishing_tunnel_ = using_ssl_;
 
-  const bool expect_spdy = IsSpdyAlternative();
-
   HttpServerProperties* http_server_properties =
       session_->http_server_properties();
   if (http_server_properties) {
@@ -920,9 +896,9 @@ int HttpStreamFactoryImpl::Job::DoInitConnectionImpl() {
     DCHECK(!delegate_->for_websockets());
     return PreconnectSocketsForHttpRequest(
         GetSocketGroup(), destination_, request_info_.extra_headers,
-        request_info_.load_flags, priority_, session_, proxy_info_, expect_spdy,
-        server_ssl_config_, proxy_ssl_config_, request_info_.privacy_mode,
-        net_log_, num_streams_);
+        request_info_.load_flags, priority_, session_, proxy_info_,
+        expect_spdy_, server_ssl_config_, proxy_ssl_config_,
+        request_info_.privacy_mode, net_log_, num_streams_);
   }
 
   // If we can't use a SPDY session, don't bother checking for one after
@@ -937,15 +913,15 @@ int HttpStreamFactoryImpl::Job::DoInitConnectionImpl() {
     websocket_server_ssl_config.alpn_protos.clear();
     return InitSocketHandleForWebSocketRequest(
         GetSocketGroup(), destination_, request_info_.extra_headers,
-        request_info_.load_flags, priority_, session_, proxy_info_, expect_spdy,
-        websocket_server_ssl_config, proxy_ssl_config_,
+        request_info_.load_flags, priority_, session_, proxy_info_,
+        expect_spdy_, websocket_server_ssl_config, proxy_ssl_config_,
         request_info_.privacy_mode, net_log_, connection_.get(),
         resolution_callback, io_callback_);
   }
 
   return InitSocketHandleForHttpRequest(
       GetSocketGroup(), destination_, request_info_.extra_headers,
-      request_info_.load_flags, priority_, session_, proxy_info_, expect_spdy,
+      request_info_.load_flags, priority_, session_, proxy_info_, expect_spdy_,
       server_ssl_config_, proxy_ssl_config_, request_info_.privacy_mode,
       net_log_, connection_.get(), resolution_callback, io_callback_);
 }
@@ -1030,16 +1006,13 @@ int HttpStreamFactoryImpl::Job::DoInitConnectionComplete(int result) {
     return result;
   }
 
-  if (proxy_info_.is_quic() && using_quic_ && result < 0) {
-    using_quic_ = false;

[Bence, 2017/06/14 17:29:10]

I believe that at this point the Job fails and will not do any more work, so
this assignment is unnecessary.

+  if (proxy_info_.is_quic() && using_quic_ && result < 0)
     return ReconsiderProxyAfterError(result);
-  }
 
-  if (IsSpdyAlternative() && !using_spdy_)
+  if (expect_spdy_ && !using_spdy_)
     return ERR_ALPN_NEGOTIATION_FAILED;
 
-  if (!ssl_started && result < 0 &&
-      (IsSpdyAlternative() || IsQuicAlternative()))
+  if (!ssl_started && result < 0 && (expect_spdy_ || using_quic_))
     return result;
 
   if (using_quic_) {
@@ -1126,7 +1099,7 @@ int HttpStreamFactoryImpl::Job::DoCreateStream() {
       FROM_HERE_WITH_EXPLICIT_FUNCTION(
           "462811 HttpStreamFactoryImpl::Job::DoCreateStream"));
   DCHECK(connection_->socket() || existing_spdy_session_.get() || using_quic_);
-  DCHECK(!IsQuicAlternative());
+  DCHECK(!using_quic_);
 
   next_state_ = STATE_CREATE_STREAM_COMPLETE;
 
@@ -1144,7 +1117,7 @@ int HttpStreamFactoryImpl::Job::DoCreateStream() {
     SetSocketMotivation();
 
   if (!using_spdy_) {
-    DCHECK(!IsSpdyAlternative());
+    DCHECK(!expect_spdy_);
     // We may get ftp scheme when fetching ftp resources through proxy.
     bool using_proxy = (proxy_info_.is_http() || proxy_info_.is_https()) &&
                        (request_info_.url.SchemeIs(url::kHttpScheme) ||
@@ -1272,14 +1245,6 @@ void HttpStreamFactoryImpl::Job::SetSocketMotivation() {
   // TODO(mbelshe): Add other motivations (like EARLY_LOAD_MOTIVATED).
 }
 
-bool HttpStreamFactoryImpl::Job::IsSpdyAlternative() const {
-  return alternative_service_.protocol == kProtoHTTP2;
-}
-
-bool HttpStreamFactoryImpl::Job::IsQuicAlternative() const {
-  return alternative_service_.protocol == kProtoQUIC;
-}
-
 void HttpStreamFactoryImpl::Job::InitSSLConfig(SSLConfig* ssl_config,
                                                bool is_proxy) const {
   if (!is_proxy) {
@@ -1446,7 +1411,7 @@ HttpStreamFactoryImpl::JobFactory::CreateMainJob(
   return base::MakeUnique<HttpStreamFactoryImpl::Job>(
       delegate, job_type, session, request_info, priority, proxy_info,
       server_ssl_config, proxy_ssl_config, destination, origin_url,
-      enable_ip_based_pooling, net_log);
+      kProtoUnknown, ProxyServer(), enable_ip_based_pooling, net_log);
 }
 
 std::unique_ptr<HttpStreamFactoryImpl::Job>
@@ -1461,13 +1426,13 @@ HttpStreamFactoryImpl::JobFactory::CreateAltSvcJob(
     const SSLConfig& proxy_ssl_config,
     HostPortPair destination,
     GURL origin_url,
-    AlternativeService alternative_service,
+    NextProto alternative_protocol,
     bool enable_ip_based_pooling,
     NetLog* net_log) {
   return base::MakeUnique<HttpStreamFactoryImpl::Job>(
       delegate, job_type, session, request_info, priority, proxy_info,
       server_ssl_config, proxy_ssl_config, destination, origin_url,
-      alternative_service, ProxyServer(), enable_ip_based_pooling, net_log);
+      alternative_protocol, ProxyServer(), enable_ip_based_pooling, net_log);
 }
 
 std::unique_ptr<HttpStreamFactoryImpl::Job>
@@ -1488,7 +1453,7 @@ HttpStreamFactoryImpl::JobFactory::CreateAltProxyJob(
   return base::MakeUnique<HttpStreamFactoryImpl::Job>(
       delegate, job_type, session, request_info, priority, proxy_info,
       server_ssl_config, proxy_ssl_config, destination, origin_url,
-      AlternativeService(), alternative_proxy_server, enable_ip_based_pooling,
+      kProtoUnknown, alternative_proxy_server, enable_ip_based_pooling,
       net_log);
 }