Change ClientSocketPoolManager::InitSocketHandleForHttpRequest and friends

net/http/http_stream_factory_impl_job.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_stream_factory_impl_job.h"
 
 #include <algorithm>
 #include <string>
 
 #include "base/bind.h"
@@ skipping 610 matching lines @@
   } else {
     server_ = HostPortPair::FromURL(request_info_.url);
   }
   origin_url_ =
       stream_factory_->ApplyHostMappingRules(request_info_.url, &server_);
   alternative_service_url_ = origin_url_;
   // For SPDY via Alt-Svc, set |alternative_service_url_| to
   // https://<alternative host>:<alternative port>/...
   // so the proxy resolution works with the actual destination, and so
   // that the correct socket pool is used.
   // TODO(rch): change the socket pool API to not require a full URL.

[mmenke, 2015/04/07 16:36:58]

You're doing this here, so should remove the TODO (Admittedly, the proxy still
needs the fully URL, but can't change that)

[Ryan Hamilton, 2015/04/07 19:07:30]

Done.

   if (alternative_service_.protocol >= NPN_SPDY_MINIMUM_VERSION &&
       alternative_service_.protocol <= NPN_SPDY_MAXIMUM_VERSION) {
     // TODO(rch):  Figure out how to make QUIC iteract with PAC
     // scripts.  By not re-writing the URL, we will query the PAC script
     // for the proxy to use to reach the original URL via TCP.  But
     // the alternate request will be going via UDP to a different port.
     GURL::Replacements replacements;
     // new_port needs to be in scope here because GURL::Replacements references
     // the memory contained by it directly.
     const std::string new_port = base::IntToString(alternative_service_.port);
@@ skipping 221 matching lines @@
     http_server_properties->MaybeForceHTTP11(server_, &server_ssl_config_);
     if (proxy_info_.is_http() || proxy_info_.is_https()) {
       http_server_properties->MaybeForceHTTP11(
           proxy_info_.proxy_server().host_port_pair(), &proxy_ssl_config_);
     }
   }
 
   if (IsPreconnecting()) {
     DCHECK(!stream_factory_->for_websockets_);
     return PreconnectSocketsForHttpRequest(
-        alternative_service_url_, request_info_.extra_headers,
+        GetSocketGroup(), server_, request_info_.extra_headers,
         request_info_.load_flags, priority_, session_, proxy_info_,
         ShouldForceSpdySSL(), want_spdy_over_npn, 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
   // the hostname is resolved.
   OnHostResolutionCallback resolution_callback = CanUseExistingSpdySession() ?
       base::Bind(&Job::OnHostResolution, session_->spdy_session_pool(),
                  GetSpdySessionKey()) :
       OnHostResolutionCallback();
   if (stream_factory_->for_websockets_) {
     // TODO(ricea): Re-enable NPN when WebSockets over SPDY is supported.
     SSLConfig websocket_server_ssl_config = server_ssl_config_;
     websocket_server_ssl_config.next_protos.clear();
     return InitSocketHandleForWebSocketRequest(
-        origin_url_, request_info_.extra_headers, request_info_.load_flags,
-        priority_, session_, proxy_info_, ShouldForceSpdySSL(),
-        want_spdy_over_npn, websocket_server_ssl_config, proxy_ssl_config_,
-        request_info_.privacy_mode, net_log_,
+        GetSocketGroup(), server_, request_info_.extra_headers,
+        request_info_.load_flags, priority_, session_, proxy_info_,
+        ShouldForceSpdySSL(), want_spdy_over_npn, websocket_server_ssl_config,
+        proxy_ssl_config_, request_info_.privacy_mode, net_log_,
         connection_.get(), resolution_callback, io_callback_);
   }
 
   return InitSocketHandleForHttpRequest(
-      alternative_service_url_, request_info_.extra_headers,
+      GetSocketGroup(), server_, request_info_.extra_headers,
       request_info_.load_flags, priority_, session_, proxy_info_,
       ShouldForceSpdySSL(), want_spdy_over_npn, server_ssl_config_,
       proxy_ssl_config_, request_info_.privacy_mode, net_log_,
       connection_.get(), resolution_callback, io_callback_);
 }
 
 int HttpStreamFactoryImpl::Job::DoInitConnectionComplete(int result) {
   if (IsPreconnecting()) {
     if (using_quic_)
       return result;
@@ skipping 574 matching lines @@
   }
 
   if (job_status_ == STATUS_SUCCEEDED && other_job_status_ == STATUS_BROKEN) {
     HistogramBrokenAlternateProtocolLocation(
         BROKEN_ALTERNATE_PROTOCOL_LOCATION_HTTP_STREAM_FACTORY_IMPL_JOB_MAIN);
     session_->http_server_properties()->MarkAlternativeServiceBroken(
         other_job_alternative_service_);
   }
 }
 
+ClientSocketPoolManager::SocketGroupType
+HttpStreamFactoryImpl::Job::GetSocketGroup() {
+  if (IsAlternate() &&
+      alternative_service_.protocol >= NPN_SPDY_MINIMUM_VERSION &&
+      alternative_service_.protocol <= NPN_SPDY_MAXIMUM_VERSION) {
+    return ClientSocketPoolManager::SSL_GROUP;

[mmenke, 2015/04/07 16:36:58]

This is kinda weird.  In HttpStreamFactoryImpl::Job::DoStart, any alternate
service URL is rewritten as https, but here, only SPDY ones are changed.  Could
this entire method just be:

ClientSocketPoolManager::GroupTypeFromScheme(alternative_service_url_.scheme())?

(I know QUIC doesn't go through this method, but as-is, just seems a bit weird
and redundant)

[Ryan Hamilton, 2015/04/07 19:07:30]

Done. Good point. 

My slightly longer term objective is to remove alternative_service_url_
completely, so I think this method will end up looking like I have it now, but
your suggestion is a good one until then. Thanks.

+  }
+  return ClientSocketPoolManager::GroupTypeFromScheme(origin_url_.scheme());
+}
+
 }  // namespace net