Change ClientSocketPoolManager::InitSocketHandleForHttpRequest and friends

Change ClientSocketPoolManager::InitSocketHandleForHttpRequest and friends
to not take a request_url, since this is confusing for Alternate-Protocol
and Alt-Svc requests. Instead, take an enum which describes the type of
socket to be requested (FTP, SSL, or normal) and endpoint to which the
socket is to be connected.

Committed: https://crrev.com/c5c07de291701d5e055d20a2a22f7065baeabdc0
Cr-Commit-Position: refs/heads/master@{#324196}

[Ryan Hamilton, 2015-04-06 15:16:03]

rch@chromium.org changed reviewers:
+ mmenke@chromium.org

[Ryan Hamilton, 2015-04-06 15:16:03]

mmenke: Hopefully this CL is small enough that you can take a look. If not, I
can get someone else to review. However, there aren't a lot of other reviewers
who grok the socket pools as well as you do. But totally push back if you're too
busy.

[mmenke, 2015-04-06 15:57:05]

On 2015/04/06 15:16:03, Ryan Hamilton wrote:
> mmenke: Hopefully this CL is small enough that you can take a look. If not, I
> can get someone else to review. However, there aren't a lot of other reviewers
> who grok the socket pools as well as you do. But totally push back if you're
too
> busy.

My review load has finally eased off the past month - lots of little ones, but
few enough big ones that I certainly have time for this.  May need to refresh my
memory a bit, but should be able to get back to you today or tomorrow.

[Ryan Hamilton, 2015-04-06 16:08:14]

On 2015/04/06 15:57:05, mmenke wrote:
> On 2015/04/06 15:16:03, Ryan Hamilton wrote:
> > mmenke: Hopefully this CL is small enough that you can take a look. If not,
I
> > can get someone else to review. However, there aren't a lot of other
reviewers
> > who grok the socket pools as well as you do. But totally push back if you're
> too
> > busy.
> 
> My review load has finally eased off the past month - lots of little ones, but
> few enough big ones that I certainly have time for this.  May need to refresh
my
> memory a bit, but should be able to get back to you today or tomorrow.

Great! Thanks.

[mmenke, 2015-04-07 16:36:59]

This seems really confusing, particularly as it interacts with the
force_spdy_over_ssl, want_spdy_over_npn flags, and alternate services.

Some preliminary suggestions, but I want to think a bit more about how to make
all this SPDY/QUIC logic easier to follow.  Currently, I think this whole area
of the code is in desperate need of cleanup.

https://codereview.chromium.org/1051023003/diff/1/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job.cc (right):

https://codereview.chromium.org/1051023003/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:631: // TODO(rch): change the socket
pool API to not require a full URL.
You're doing this here, so should remove the TODO (Admittedly, the proxy still
needs the fully URL, but can't change that)

https://codereview.chromium.org/1051023003/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:1498: return
ClientSocketPoolManager::SSL_GROUP;
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)

https://codereview.chromium.org/1051023003/diff/1/net/socket/client_socket_po...
File net/socket/client_socket_pool_manager.cc (right):

https://codereview.chromium.org/1051023003/diff/1/net/socket/client_socket_po...
net/socket/client_socket_pool_manager.cc:93: group_type ==
ClientSocketPoolManager::SSL_GROUP || force_spdy_over_ssl;
This seems kinda weird.  We use the group type the caller passed to us...Unless
the caller also told us to override the group type?  Think it would make more
sense to pass in the real group type instead.

And then force_spdy_over_ssl would only be used for SPDY negotiation histograms,
I guess?  Do we even care about histograms in that case?

https://codereview.chromium.org/1051023003/diff/1/net/socket/client_socket_po...
File net/socket/client_socket_pool_manager.h (right):

https://codereview.chromium.org/1051023003/diff/1/net/socket/client_socket_po...
net/socket/client_socket_pool_manager.h:47: enum SocketGroupType { NORMAL_GROUP,
SSL_GROUP, FTP_GROUP };
It's not clear what a group type is, particular with force_spdy_over_ssl
muddying the waters.  Should document it.

https://codereview.chromium.org/1051023003/diff/1/net/socket/client_socket_po...
net/socket/client_socket_pool_manager.h:106: bool want_spdy_over_npn,
Could you document these two parameters, while you're here?  I have no idea what
they mean.  Looks like if we "want_spdy_over_npn", but fail to negotiate SPDY,
we consider it a failed request?

[Ryan Hamilton, 2015-04-07 19:07:31]

Thanks!

https://codereview.chromium.org/1051023003/diff/1/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job.cc (right):

https://codereview.chromium.org/1051023003/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:631: // TODO(rch): change the socket
pool API to not require a full URL.
On 2015/04/07 16:36:58, mmenke wrote:
> You're doing this here, so should remove the TODO (Admittedly, the proxy still
> needs the fully URL, but can't change that)

Done.

https://codereview.chromium.org/1051023003/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job.cc:1498: return
ClientSocketPoolManager::SSL_GROUP;
On 2015/04/07 16:36:58, mmenke wrote:
> 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)

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.

https://codereview.chromium.org/1051023003/diff/1/net/socket/client_socket_po...
File net/socket/client_socket_pool_manager.cc (right):

https://codereview.chromium.org/1051023003/diff/1/net/socket/client_socket_po...
net/socket/client_socket_pool_manager.cc:93: group_type ==
ClientSocketPoolManager::SSL_GROUP || force_spdy_over_ssl;
On 2015/04/07 16:36:58, mmenke wrote:
> This seems kinda weird.  We use the group type the caller passed to
us...Unless
> the caller also told us to override the group type?  Think it would make more
> sense to pass in the real group type instead.

Oh goodness, this is hideous. You're right. I've pushed this logic up the stack.

> And then force_spdy_over_ssl would only be used for SPDY negotiation
histograms,
> I guess?  Do we even care about histograms in that case?

If it's ok with you, I'm going to keep the logic down in the SSLClientSocketPool
for now, but I'll remove this whole crazy force_spdy logic completely in a
follow up CL (perhaps pending some stats). We added this stuff back in the
SPDY/1 days but that was like 5 year ago (before I started). It's LONG since
been useful.

https://codereview.chromium.org/1051023003/diff/1/net/socket/client_socket_po...
File net/socket/client_socket_pool_manager.h (right):

https://codereview.chromium.org/1051023003/diff/1/net/socket/client_socket_po...
net/socket/client_socket_pool_manager.h:47: enum SocketGroupType { NORMAL_GROUP,
SSL_GROUP, FTP_GROUP };
On 2015/04/07 16:36:58, mmenke wrote:
> It's not clear what a group type is, particular with force_spdy_over_ssl
> muddying the waters.  Should document it.

Done. Hopefully this helps. now that the force_spdy wart has been removed, this
should hopefully be self explanatory?

https://codereview.chromium.org/1051023003/diff/1/net/socket/client_socket_po...
net/socket/client_socket_pool_manager.h:106: bool want_spdy_over_npn,
On 2015/04/07 16:36:58, mmenke wrote:
> Could you document these two parameters, while you're here?  I have no idea
what
> they mean.  Looks like if we "want_spdy_over_npn", but fail to negotiate SPDY,
> we consider it a failed request?

Done. I'm not a fan of "want_spdy_over_npn" as a name. How about
"expect_spdy_over_npn" or "requires_" or "needs_"?

[mmenke, 2015-04-07 19:24:16]

Quick responses.  Think I'll have time to do a full (Hopefully final) pass
today, but just in case I don't.

https://codereview.chromium.org/1051023003/diff/1/net/socket/client_socket_po...
File net/socket/client_socket_pool_manager.cc (right):

https://codereview.chromium.org/1051023003/diff/1/net/socket/client_socket_po...
net/socket/client_socket_pool_manager.cc:93: group_type ==
ClientSocketPoolManager::SSL_GROUP || force_spdy_over_ssl;
On 2015/04/07 19:07:30, Ryan Hamilton wrote:
> On 2015/04/07 16:36:58, mmenke wrote:
> > This seems kinda weird.  We use the group type the caller passed to
> us...Unless
> > the caller also told us to override the group type?  Think it would make
more
> > sense to pass in the real group type instead.
> 
> Oh goodness, this is hideous. You're right. I've pushed this logic up the
stack.
> 
> > And then force_spdy_over_ssl would only be used for SPDY negotiation
> histograms,
> > I guess?  Do we even care about histograms in that case?
> 
> If it's ok with you, I'm going to keep the logic down in the
SSLClientSocketPool
> for now, but I'll remove this whole crazy force_spdy logic completely in a
> follow up CL (perhaps pending some stats). We added this stuff back in the
> SPDY/1 days but that was like 5 year ago (before I started). It's LONG since
> been useful.

I'm fine with keeping it for now, think we should remove this stuff sooner
rather than later - I've scratched my head over all that spdy stuff multiple
times in the past, so there's a real cost in terms of hair loss over unused code
(Particularly weird legacy code).

https://codereview.chromium.org/1051023003/diff/1/net/socket/client_socket_po...
File net/socket/client_socket_pool_manager.h (right):

https://codereview.chromium.org/1051023003/diff/1/net/socket/client_socket_po...
net/socket/client_socket_pool_manager.h:106: bool want_spdy_over_npn,
On 2015/04/07 19:07:30, Ryan Hamilton wrote:
> On 2015/04/07 16:36:58, mmenke wrote:
> > Could you document these two parameters, while you're here?  I have no idea
> what
> > they mean.  Looks like if we "want_spdy_over_npn", but fail to negotiate
SPDY,
> > we consider it a failed request?
> 
> Done. I'm not a fan of "want_spdy_over_npn" as a name. How about
> "expect_spdy_over_npn" or "requires_" or "needs_"?

Maybe require_spdy_over_npn?  (The imperative seems a little clearer, and I
don't really know what "expect" means)

https://codereview.chromium.org/1051023003/diff/20001/net/socket/client_socke...
File net/socket/client_socket_pool_manager.h (right):

https://codereview.chromium.org/1051023003/diff/20001/net/socket/client_socke...
net/socket/client_socket_pool_manager.h:50: FTP_GROUP      // For FTP sockets.
optional:  Maybe "For FTP sockets" -> "For FTP sockets (over an HTTP proxy)."

https://codereview.chromium.org/1051023003/diff/20001/net/socket/client_socke...
net/socket/client_socket_pool_manager.h:102: // SPDY must have been
negotiationed or else it will be considered an error.
negotiationed -> negotiated

[mmenke, 2015-04-07 20:27:33]

LGTM, modulo comments.  Once you remove the force_spdy_over_ssl flag, could also
consider just passing in the scheme (Think it's a little strange how the logic
to figure out SocketGroupType is split across the Job and the
SocketPoolManager).

https://codereview.chromium.org/1051023003/diff/20001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_job.h (right):

https://codereview.chromium.org/1051023003/diff/20001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job.h:239:
ClientSocketPoolManager::SocketGroupType GetSocketGroup();
nit:  const

https://codereview.chromium.org/1051023003/diff/20001/net/socket/client_socke...
File net/socket/client_socket_pool_manager.cc (right):

https://codereview.chromium.org/1051023003/diff/20001/net/socket/client_socke...
net/socket/client_socket_pool_manager.cc:70: const HostPortPair endpoint,
Should either pass this by reference, or just rename it origin_host_port and
make it non-const (And get rid of the old origin_host_port, of course).

[Ryan Hamilton, 2015-04-08 03:41:19]

Patchset #3 (id:40001) has been deleted

[Ryan Hamilton, 2015-04-08 03:44:46]

Thanks!

re. passing in scheme: I'm not thrilled with that idea because in the alt-svc
case, the URL's scheme will be "http", but we need an SSL socket. So the if we
pass in a scheme, we'd have to pass in "https", but since that's not the actual
scheme it feels sketchy. But it's not *that* bad, so perhaps it's a better idea?

https://codereview.chromium.org/1051023003/diff/20001/net/http/http_stream_fa...
File net/http/http_stream_factory_impl_job.h (right):

https://codereview.chromium.org/1051023003/diff/20001/net/http/http_stream_fa...
net/http/http_stream_factory_impl_job.h:239:
ClientSocketPoolManager::SocketGroupType GetSocketGroup();
On 2015/04/07 20:27:32, mmenke wrote:
> nit:  const

Done.

https://codereview.chromium.org/1051023003/diff/20001/net/socket/client_socke...
File net/socket/client_socket_pool_manager.cc (right):

https://codereview.chromium.org/1051023003/diff/20001/net/socket/client_socke...
net/socket/client_socket_pool_manager.cc:70: const HostPortPair endpoint,
On 2015/04/07 20:27:32, mmenke wrote:
> Should either pass this by reference, or just rename it origin_host_port and
> make it non-const (And get rid of the old origin_host_port, of course).

Done.

https://codereview.chromium.org/1051023003/diff/20001/net/socket/client_socke...
File net/socket/client_socket_pool_manager.h (right):

https://codereview.chromium.org/1051023003/diff/20001/net/socket/client_socke...
net/socket/client_socket_pool_manager.h:50: FTP_GROUP      // For FTP sockets.
On 2015/04/07 19:24:16, mmenke wrote:
> optional:  Maybe "For FTP sockets" -> "For FTP sockets (over an HTTP proxy)."

Done.

https://codereview.chromium.org/1051023003/diff/20001/net/socket/client_socke...
net/socket/client_socket_pool_manager.h:102: // SPDY must have been
negotiationed or else it will be considered an error.
On 2015/04/07 19:24:16, mmenke wrote:
> negotiationed -> negotiated

Done.

[Ryan Hamilton, 2015-04-08 03:44:50]

The CQ bit was checked by rch@chromium.org

[Ryan Hamilton, 2015-04-08 03:44:50]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/1051023003/#ps60001
(title: "fix comments")

[commit-bot: I haz the power, 2015-04-08 03:45:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1051023003/60001

[mmenke, 2015-04-08 03:52:30]

On 2015/04/08 03:44:46, Ryan Hamilton wrote:
> Thanks!
> 
> re. passing in scheme: I'm not thrilled with that idea because in the alt-svc
> case, the URL's scheme will be "http", but we need an SSL socket. So the if we
> pass in a scheme, we'd have to pass in "https", but since that's not the
actual
> scheme it feels sketchy. But it's not *that* bad, so perhaps it's a better
idea?

Another option would be to just inline
ClientSocketPoolManager::GroupTypeFromScheme...  If it's not the scheme of the
actual request that determines scheme, and it's only the Job that knows how to
determine type, maybe all the logic should go there?

Anyhow, still LGTM.

[commit-bot: I haz the power, 2015-04-08 07:28:29]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2015-04-08 07:29:15]

Patchset 3 (id:??) landed as
https://crrev.com/c5c07de291701d5e055d20a2a22f7065baeabdc0
Cr-Commit-Position: refs/heads/master@{#324196}

[Ryan Hamilton, 2015-04-08 14:17:11]

On 2015/04/08 03:52:30, mmenke wrote:
> Another option would be to just inline
> ClientSocketPoolManager::GroupTypeFromScheme...  If it's not the scheme of the
> actual request that determines scheme, and it's only the Job that knows how to
> determine type, maybe all the logic should go there?
> 
> Anyhow, still LGTM.

Oh, good idea. URL: https://codereview.chromium.org/1069413002