Refactor pooling logic into a helper method

net/quic/quic_client_session.cc

Index: net/quic/quic_client_session.cc
diff --git a/net/quic/quic_client_session.cc b/net/quic/quic_client_session.cc
index c6699f189bace3a856695447723baebb27611843..2f771d346ec428651ae35af7c030867fbb8df7a1 100644
--- a/net/quic/quic_client_session.cc
+++ b/net/quic/quic_client_session.cc
@@ -13,6 +13,8 @@
 #include "base/values.h"
 #include "net/base/io_buffer.h"
 #include "net/base/net_errors.h"
+#include "net/http/http_util.h"
+#include "net/http/transport_security_state.h"
 #include "net/quic/crypto/proof_verifier_chromium.h"
 #include "net/quic/crypto/quic_server_info.h"
 #include "net/quic/quic_connection_helper.h"
@@ -138,6 +140,7 @@ QuicClientSession::QuicClientSession(
     scoped_ptr<QuicDefaultPacketWriter> writer,
     QuicStreamFactory* stream_factory,
     QuicCryptoClientStreamFactory* crypto_client_stream_factory,
+    TransportSecurityState* transport_security_state,
     scoped_ptr<QuicServerInfo> server_info,
     const QuicServerId& server_id,
     const QuicConfig& config,
@@ -151,6 +154,7 @@ QuicClientSession::QuicClientSession(
       socket_(socket.Pass()),
       writer_(writer.Pass()),
       read_buffer_(new IOBufferWithSize(kMaxPacketSize)),
+      transport_security_state_(transport_security_state),
       server_info_(server_info.Pass()),
       read_pending_(false),
       num_total_streams_(0),
@@ -489,28 +493,8 @@ bool QuicClientSession::CanPool(const std::string& hostname) const {
     return true;
   }
 
-  // Disable pooling for secure sessions.
-  // TODO(rch): re-enable this.
-  return false;
-#if 0
-  bool unused = false;
-  // Pooling is prohibited if the server cert is not valid for the new domain,
-  // and for connections on which client certs were sent. It is also prohibited
-  // when channel ID was sent if the hosts are from different eTLDs+1.
-  if (!ssl_info.cert->VerifyNameMatch(hostname, &unused))
-    return false;
-
-  if (ssl_info.client_cert_sent)
-    return false;
-
-  if (ssl_info.channel_id_sent &&
-      ChannelIDService::GetDomainForHost(hostname) !=
-      ChannelIDService::GetDomainForHost(server_host_port_.host())) {
-    return false;
-  }
-
-  return true;
-#endif
+  return HttpUtil::CanPool(transport_security_state_, ssl_info,

[Ryan Sleevi, 2014/08/07 18:49:29]

This seems weird to be called an HTTP util. This isn't really an HTTP-level
concept, but seems like it's more tied with the QUIC/SPDY session pools / client
socket pool?

For example, one way I see this sort of backfiring is with respect to how group
names are treated (for example, privacy mode vs non-privacy mode), and the
caller being confused as to whether/how this treats them.

[Ryan Hamilton, 2014/08/08 19:27:43]

*nod* I've stuffed it into spdy_session.h for now, but happy to put it somewhere
if you have a better idea.
Hm. I'm not sure. There are two steps that, say, the SpdySessionPool has to
undertake in order to send an existing request to an existing session. First it
has to (as you suggest) identify a set of possible candidate session based on
things like IP overlap, privacy mode, secure vs insecure.  Only after a
candidate session has been identified does it perform the this final validation
step on the session. I don't think we can fold the "identification" step into
common code because SpdySessionPool and QuicStreamFactor represent these things
quite differently. I'm totally happy to move this method anywhere you think it
should go; I'm just not sold on folding in privacy mode and friends into it. 

What do you think?

[Ryan Sleevi, 2014/08/11 19:09:17]

Right, I wasn't trying to suggest folding privacy mode into it - merely that
someone would assume in HttpUtil that it did, or that it was safe to use w/o
checking privacy mode.

I think moving it to Spdy is fine. I guess the argument for HttpUtil is that
this is part of the HTTP/2 spec, and so maybe this really is an HTTP utility
now, even though we support SPDY?

Or are our SPDY classes being used for h2 support now?

[Ryan Hamilton, 2014/08/12 14:39:06]

Yeah, that's what I was thinking, but I agree that it felt weird.
Indeed SPDY == HTTP2, which makes SPDY seem like a reasonable place.  Thanks!

+                           server_host_port_.host(), hostname);
 }
 
 QuicDataStream* QuicClientSession::CreateIncomingDataStream(