Implement a 15 connection per proxy server limit.

Implement a 15 connection per proxy server limit.

Previously, we had a limit of 6 connections per proxy server, which was horrifically low.
Connections to all endpoint hosts remains at 6.
Basically, we have a map of socket pools, keyed by the proxy server.
Each of these proxy socket pools has a socket limit of 15 and a connection limit per host of 6.
The main TCP socket pool (non-proxied) has the standard 256 socket limit, and connection limit per host of 6.
There are two maps currently, one for HTTP proxies and one for SOCKS proxies.
Note that SSL tunnels over HTTP CONNECTs are still located within the standard http proxy socket pools.
We depend on the fact that our code never returns a non-SSL connected socket to the pool for a |connection_group|
that is to a HTTPS endpoint.  A newly connected socket from the pool will only have the TCP connection done.  An
idle socket will have both the TCP and the HTTP CONNECT and the SSL handshake done for it.

TODO(willchan): Remove the extra constructor overload for the old deprecated parameters.  Switch to using HostPortPair everywhere.
TODO(willchan): Finish SSL socket pools.
TODO(willchan): Rip out the DoInitConnection() code so it can be shared by other caller (TCP pre-warming!).
TODO(willchan): Flush out the socket pool maps when the proxy configuration changes.
TODO(willchan): Fix up global socket limits.  They're slightly broken in this case, since each pool instance has its own "global" limit, so obviously different pools don't share the same "global" limit.  This is such a minor deal since the global limits are so small for proxy servers compared to the system global limits (256 vs 15), that it doesn't have a big effect.
TODO(willchan): Drink moar b33r.

BUG=12066
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=45896

[willchan no longer on Chromium, 2010-04-28 00:00:14]

Not quite ready for review.  But it's probably worth it to take a quick
overview.

wtc: Please examine the new |connection_group_| logic in
http_network_transaction.cc.  Read the changelist description and state if
you're ok with the new initial limits (I think we should experiment with them in
Chrome 6).
vandebo: Check to see if you're cool with my proposal for how we use socket
pools for proxies (the map of proxy servers to socket pool instances).  In
particular, note the changes in http_network_session.cc.
eroman: Please review the changelist description and skim over changes.

I want to clean up the tests more before I submit this change (I want to add
tests to verify that requests go to the correct pool) and finish up part of my
TODO to replace with HostPortPair.

[vandebo (ex-Chrome), 2010-04-28 00:58:18]

Seems good.

Not sure about all your TODO's

Enforcing a global connection limit is probably easy - All connections must have
a TCP connection at base, so adding a class global counter in the TCP pool would
work.  If you want to be able to queue requests when the connection limit is
hit, you'll need a bit more logic to queue and dispatch to TCP pool instances.

[willchan no longer on Chromium, 2010-04-28 01:10:00]

On 2010/04/28 00:58:18, vandebo wrote:
> Seems good.
> 
> Not sure about all your TODO's
> 
> Enforcing a global connection limit is probably easy - All connections must
have
> a TCP connection at base, so adding a class global counter in the TCP pool
would
> work.  If you want to be able to queue requests when the connection limit is
> hit, you'll need a bit more logic to queue and dispatch to TCP pool instances.

Yeah, this is the part.  What if you have multiple stalled pools?  How do you do
prioritization across multiple pool instances?  The need for a static counter
would also mean that you have to have a static vector of all TCPClientSocketPool
instances so you can call each to see if they can be unstalled.  And you have to
iterate through them all to do the highest priority search.  And then there's
the possibility of running multiple instances in multiple threads.  browser sync
is currently running some network stack code in an alternate thread (although
not the socket pools).  This gets weird quickly and requires locking and what
not.

[vandebo (ex-Chrome), 2010-04-28 01:22:26]

Oops, forgot to include my comments...

On Tue, Apr 27, 2010 at 6:10 PM, <willchan@chromium.org> wrote:
> Yeah, this is the part.  What if you have multiple stalled pools?  How do you
do
> prioritization across multiple pool instances?  The need for a static counter
> would also mean that you have to have a static vector of all
TCPClientSocketPool
> instances so you can call each to see if they can be unstalled.  And you have
to
> iterate through them all to do the highest priority search.  And then there's
> the possibility of running multiple instances in multiple threads.  browser
sync
> is currently running some network stack code in an alternate thread (although
> not the socket pools).  This gets weird quickly and requires locking and what
> not.

There's probably no easy way around dealing with this messiness.  Though, you
could prioritize up front instead of when a slot is available.  Maybe a static
priority queue with locking.  When the connection count decreases, dispatch the
top of the queue.  To deal with multiple threads, the priority queue could be of
tasks to dispatch.

http://codereview.chromium.org/1808001/diff/1/2
File net/http/http_network_session.cc (right):

http://codereview.chromium.org/1808001/diff/1/2#newcode68
net/http/http_network_session.cc:68: StringPrintf("HTTPProxy[%s]",
http_proxy.ToString().c_str()),
I don't think UMA supports processing arbitrary statistic names and we probably
don't care about UMA stats per proxy server, so a static name should be fine.

http://codereview.chromium.org/1808001/diff/1/2#newcode82
net/http/http_network_session.cc:82: StringPrintf("SOCKS[%s]",
socks_proxy.ToString().c_str()),
ditto.

http://codereview.chromium.org/1808001/diff/1/4
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/1808001/diff/1/4#newcode718
net/http/http_network_transaction.cc:718: HostPortPair proxy_host_port_pair;
I've gone back and forth about having two different HostPortPair's here.  The
connection->Init logic might be a bit simpler with just one.  But if you like
two it seems ok too.

[eroman, 2010-04-28 01:34:00]

Overall I like the increase of the HTTP proxy limit from 6 to 15 (you should
mention that explicitly in the description).

My concern is, it looks like this hange breaks HTTP proxy tunneling and SOCKS.

 * In the case of SOCKS and (HTTP+CONNECT), the target hostname was part of the
connection group. It doesn't look like that is the case anymore, in which case
we may will re-use incorrect sockets from the pool.

* The socks pool needs to be keyed by socks version number, not just
hostandport. Otherwise if the same socks server supports both v4 and v5
protocols, we may re-use the wrong sockets.

http://codereview.chromium.org/1808001/diff/1/2
File net/http/http_network_session.cc (right):

http://codereview.chromium.org/1808001/diff/1/2#newcode18
net/http/http_network_session.cc:18: // Total limit of sockets. Not a constant
to allow experiments.
I don't find the "Not a constant to allow experiments" comment useful.

http://codereview.chromium.org/1808001/diff/1/2#newcode24
net/http/http_network_session.cc:24: int g_max_sockets_per_group = 6;
I suggest adding some additional comments explaining how we arrived at 6, and
how it compares to other browsers. Or linking to a bug.

http://codereview.chromium.org/1808001/diff/1/2#newcode26
net/http/http_network_session.cc:26: // The max number of sockets to allow per
proxy server.  Not a constant to
Ditto, I don't think the "Not a constant" comment is useful.

http://codereview.chromium.org/1808001/diff/1/2#newcode28
net/http/http_network_session.cc:28: int g_max_sockets_per_proxy_server = 15;
Can you give some extra comments about this? For example, how this compares to
other browsers (this is what Firefox uses), and that this limit applies to both
SOCKS and HTTP proxies.

http://codereview.chromium.org/1808001/diff/1/2#newcode64
net/http/http_network_session.cc:64: if (!ContainsKey(http_proxy_socket_pool_,
http_proxy)) {
nit: you could avoid doing two lookups in the case where pool already exists, by
using find().

http://codereview.chromium.org/1808001/diff/1/2#newcode72
net/http/http_network_session.cc:72: return http_proxy_socket_pool_[http_proxy];
nit: I noticed that HostPortPair's operator< tests |host| and then |port|.
Reversing the ordering might lead to better performance.

http://codereview.chromium.org/1808001/diff/1/2#newcode82
net/http/http_network_session.cc:82: StringPrintf("SOCKS[%s]",
socks_proxy.ToString().c_str()),
nit: I suggest using something other than square brackets (might look weird if
the hostname was an IPv6 literal).

http://codereview.chromium.org/1808001/diff/1/2#newcode92
net/http/http_network_session.cc:92: return socks_socket_pool_[socks_proxy];
Shouldn't the SOCKS version be a part of the key?

http://codereview.chromium.org/1808001/diff/1/3
File net/http/http_network_session.h (right):

http://codereview.chromium.org/1808001/diff/1/3#newcode66
net/http/http_network_session.h:66: const scoped_refptr<SOCKSClientSocketPool>&
GetSocketPoolForSOCKSProxy(
Have you considered combining GetSocketPoolForSocksProxy() and
GetSocketPoolForProxy() into just 1 method?

GetSocketPoolForProxy(const ProxyServer& proxy_server);

(ProxyServer is a proxy scheme + host and port).

http://codereview.chromium.org/1808001/diff/1/3#newcode69
net/http/http_network_session.h:69: const scoped_refptr<TCPClientSocketPool>&
GetSocketPoolForProxy(
I suggest a better name pair to avoid confusion with GetSocketPoolForSOCKSProxy.
What about GetSocketPoolForHTPProxy() to match the member variable?

http://codereview.chromium.org/1808001/diff/1/4
File net/http/http_network_transaction.cc (left):

http://codereview.chromium.org/1808001/diff/1/4#oldcode766
net/http/http_network_transaction.cc:766:
connection_group.append(endpoint.ToString());
Haven't you broken tunneling + SOCKS with this change?

http://codereview.chromium.org/1808001/diff/1/4
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/1808001/diff/1/4#newcode715
net/http/http_network_transaction.cc:715: endpoint_.host =
request_->url.HostNoBrackets();
nit: I think this would be cleaner as:
   endpoint_ = HostAndPort(...., ....);

struct-style initialization always scares me since it is more fragile to
changes.

http://codereview.chromium.org/1808001/diff/1/4#newcode722
net/http/http_network_transaction.cc:722: proxy_host_port_pair.host =
proxy_server.HostNoBrackets();
Can you instead a method to ProxyServer that directly returns a HostAndPort?

In fact, ProxyServer currently has the method:
   string host_and_port() const;

So just refactor it into:
  const HostAndPort& host_and_port() const;

http://codereview.chromium.org/1808001/diff/1/7
File net/socket/socks_client_socket_pool.h (right):

http://codereview.chromium.org/1808001/diff/1/7#newcode32
net/socket/socks_client_socket_pool.h:32: destination_(host_port_pair.host,
host_port_pair.port),
Seems like destination_ should be constructed directly from host_port_pair.

I suppose this is a follow-up task, to convert everything to using HostAndPort.

[willchan no longer on Chromium, 2010-04-28 02:54:40]

I agreed with the risk of breaking SOCKS if versions 4 and 5 were both sent to
the same SOCKS server.  I didn't account for that.  I have now.

I am not sure how the HTTP tunneling is broken.  Can you explain that?

vandebo: Yeah, for the reasons listed above, I think that it's not worth the
effort to shave all those yaks to implement the TODO.  Thanks for catching the
UMA stuff.  I totally forget about that sometimes.

http://codereview.chromium.org/1808001/diff/1/2
File net/http/http_network_session.cc (right):

http://codereview.chromium.org/1808001/diff/1/2#newcode18
net/http/http_network_session.cc:18: // Total limit of sockets. Not a constant
to allow experiments.
On 2010/04/28 01:34:00, eroman wrote:
> I don't find the "Not a constant to allow experiments" comment useful.

Done.

http://codereview.chromium.org/1808001/diff/1/2#newcode24
net/http/http_network_session.cc:24: int g_max_sockets_per_group = 6;
On 2010/04/28 01:34:00, eroman wrote:
> I suggest adding some additional comments explaining how we arrived at 6, and
> how it compares to other browsers. Or linking to a bug.

I've linked to 12066.  Do you have any other bugs I should link to?

http://codereview.chromium.org/1808001/diff/1/2#newcode26
net/http/http_network_session.cc:26: // The max number of sockets to allow per
proxy server.  Not a constant to
On 2010/04/28 01:34:00, eroman wrote:
> Ditto, I don't think the "Not a constant" comment is useful.

Done.

http://codereview.chromium.org/1808001/diff/1/2#newcode28
net/http/http_network_session.cc:28: int g_max_sockets_per_proxy_server = 15;
On 2010/04/28 01:34:00, eroman wrote:
> Can you give some extra comments about this? For example, how this compares to
> other browsers (this is what Firefox uses), and that this limit applies to
both
> SOCKS and HTTP proxies.

I've linked to crbug.com/12066.

http://codereview.chromium.org/1808001/diff/1/2#newcode64
net/http/http_network_session.cc:64: if (!ContainsKey(http_proxy_socket_pool_,
http_proxy)) {
On 2010/04/28 01:34:00, eroman wrote:
> nit: you could avoid doing two lookups in the case where pool already exists,
by
> using find().

Done.

http://codereview.chromium.org/1808001/diff/1/2#newcode68
net/http/http_network_session.cc:68: StringPrintf("HTTPProxy[%s]",
http_proxy.ToString().c_str()),
On 2010/04/28 01:22:26, vandebo wrote:
> I don't think UMA supports processing arbitrary statistic names and we
probably
> don't care about UMA stats per proxy server, so a static name should be fine.

Oops, I totally forgot about UMA, thanks for catching this!

http://codereview.chromium.org/1808001/diff/1/2#newcode72
net/http/http_network_session.cc:72: return http_proxy_socket_pool_[http_proxy];
On 2010/04/28 01:34:00, eroman wrote:
> nit: I noticed that HostPortPair's operator< tests |host| and then |port|.
> Reversing the ordering might lead to better performance.

Done.

http://codereview.chromium.org/1808001/diff/1/2#newcode82
net/http/http_network_session.cc:82: StringPrintf("SOCKS[%s]",
socks_proxy.ToString().c_str()),
On 2010/04/28 01:22:26, vandebo wrote:
> ditto.

Done.

http://codereview.chromium.org/1808001/diff/1/2#newcode82
net/http/http_network_session.cc:82: StringPrintf("SOCKS[%s]",
socks_proxy.ToString().c_str()),
On 2010/04/28 01:34:00, eroman wrote:
> nit: I suggest using something other than square brackets (might look weird if
> the hostname was an IPv6 literal).

Not relevant anymore given vandebo's comment.

http://codereview.chromium.org/1808001/diff/1/2#newcode92
net/http/http_network_session.cc:92: return socks_socket_pool_[socks_proxy];
On 2010/04/28 01:34:00, eroman wrote:
> Shouldn't the SOCKS version be a part of the key?

Hm, looks like it should not.  Instead, the scheme should be part of the
|connection_group|.  That way, we can still have our limit be per proxy server. 
If a proxy server at a given host port pair accepts both socks 4 and socks 5,
then we shouldn't split the limit for both version.

http://codereview.chromium.org/1808001/diff/1/3
File net/http/http_network_session.h (right):

http://codereview.chromium.org/1808001/diff/1/3#newcode66
net/http/http_network_session.h:66: const scoped_refptr<SOCKSClientSocketPool>&
GetSocketPoolForSOCKSProxy(
On 2010/04/28 01:34:00, eroman wrote:
> Have you considered combining GetSocketPoolForSocksProxy() and
> GetSocketPoolForProxy() into just 1 method?
> 
> GetSocketPoolForProxy(const ProxyServer& proxy_server);
> 
> (ProxyServer is a proxy scheme + host and port).

Doesn't work, since we need the concrete subtype for the template checks I do
later.

http://codereview.chromium.org/1808001/diff/1/3#newcode69
net/http/http_network_session.h:69: const scoped_refptr<TCPClientSocketPool>&
GetSocketPoolForProxy(
On 2010/04/28 01:34:00, eroman wrote:
> I suggest a better name pair to avoid confusion with
GetSocketPoolForSOCKSProxy.
> What about GetSocketPoolForHTPProxy() to match the member variable?

Ok.

http://codereview.chromium.org/1808001/diff/1/4
File net/http/http_network_transaction.cc (left):

http://codereview.chromium.org/1808001/diff/1/4#oldcode766
net/http/http_network_transaction.cc:766:
connection_group.append(endpoint.ToString());
On 2010/04/28 01:34:00, eroman wrote:
> Haven't you broken tunneling + SOCKS with this change?

I see how SOCKS might be broken since I don't key on the SOCKS version.  How is
tunneling broken?

http://codereview.chromium.org/1808001/diff/1/4
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/1808001/diff/1/4#newcode715
net/http/http_network_transaction.cc:715: endpoint_.host =
request_->url.HostNoBrackets();
On 2010/04/28 01:34:00, eroman wrote:
> nit: I think this would be cleaner as:
>    endpoint_ = HostAndPort(...., ....);
> 
> struct-style initialization always scares me since it is more fragile to
> changes.

Done.

http://codereview.chromium.org/1808001/diff/1/4#newcode718
net/http/http_network_transaction.cc:718: HostPortPair proxy_host_port_pair;
On 2010/04/28 01:22:26, vandebo wrote:
> I've gone back and forth about having two different HostPortPair's here.  The
> connection->Init logic might be a bit simpler with just one.  But if you like
> two it seems ok too.

Well, there has to be one to differentiate between endpoint_ and the other one
right?  Since we connect directly to the proxy server with a connection_group
that is derived from endpoint_.  And endpoint_ has to be cached for use later on
in constructing the CONNECT request line.  Also it gets used for SPDY later on
too.  I guess you mean that I could just StringPrintf() from within the if
statements for the proxies below.  I guess that's true.  How about I move this
to be scoped within there instead then?

http://codereview.chromium.org/1808001/diff/1/4#newcode722
net/http/http_network_transaction.cc:722: proxy_host_port_pair.host =
proxy_server.HostNoBrackets();
On 2010/04/28 01:34:00, eroman wrote:
> Can you instead a method to ProxyServer that directly returns a HostAndPort?
> 
> In fact, ProxyServer currently has the method:
>    string host_and_port() const;
> 
> So just refactor it into:
>   const HostAndPort& host_and_port() const;
> 

It looks like there are a few other uses of this.  I'm going to leave a TODO to
clean them up later.

http://codereview.chromium.org/1808001/diff/1/7
File net/socket/socks_client_socket_pool.h (right):

http://codereview.chromium.org/1808001/diff/1/7#newcode32
net/socket/socks_client_socket_pool.h:32: destination_(host_port_pair.host,
host_port_pair.port),
On 2010/04/28 01:34:00, eroman wrote:
> Seems like destination_ should be constructed directly from host_port_pair.
> 
> I suppose this is a follow-up task, to convert everything to using
HostAndPort.

Yeah, this is a TODO.  Too much needs to be cleaned up for that.  I'm
incrementally cleaning things up.

[eroman, 2010-04-28 03:14:29]

> I am not sure how the HTTP tunneling is broken.  Can you explain that?

I was wrong; upon re-reading this, looks like HTTP tunneling should be fine.

Somehow I thought the proxy host/port pair was being used as the connection
group, but I realize now you are using end_point_ which is actually the URL's
host/port pair.

Thanks for clarifying this.

[willchan no longer on Chromium, 2010-04-28 03:23:02]

On 2010/04/28 03:14:29, eroman wrote:
> > I am not sure how the HTTP tunneling is broken.  Can you explain that?
> 
> I was wrong; upon re-reading this, looks like HTTP tunneling should be fine.
> 
> Somehow I thought the proxy host/port pair was being used as the connection
> group, but I realize now you are using end_point_ which is actually the URL's
> host/port pair.
> 
> Thanks for clarifying this.

Np, I'm glad you're looking at that code.  It's hard to notice breakages, so
definitely take a close look.  I've added some more tests to cover socks4 and
sock5 cases too.

[vandebo (ex-Chrome), 2010-04-28 21:18:09]

LGTM with comments.

http://codereview.chromium.org/1808001/diff/10002/15004
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/1808001/diff/10002/15004#newcode779
net/http/http_network_transaction.cc:779: StringPrintf("socks%s/%s",
socks_version, connection_group.c_str());
Why not use proxy_server.ToURI()?  It's simpler and having both the proxy server
and destination in the connection group might help debugging with net log
output.

http://codereview.chromium.org/1808001/diff/10002/15006
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/1808001/diff/10002/15006#newcode3737
net/http/http_network_transaction_unittest.cc:3737: for (size_t i = 0; i <
ARRAYSIZE_UNSAFE(tests); ++i) {
Maybe this code can get de-dupped a bit?

http://codereview.chromium.org/1808001/diff/10002/15006#newcode3764
net/http/http_network_transaction_unittest.cc:3764: std::string allgroups =
tcp_conn_pool->last_group_name_received();
This isn't allgroups any more.

[willchan no longer on Chromium, 2010-04-28 21:49:53]

Thanks for the review Steve.

Eric, have anything else?  Wan-Teh, want to take a look??

http://codereview.chromium.org/1808001/diff/10002/15004
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/1808001/diff/10002/15004#newcode779
net/http/http_network_transaction.cc:779: StringPrintf("socks%s/%s",
socks_version, connection_group.c_str());
On 2010/04/28 21:18:09, vandebo wrote:
> Why not use proxy_server.ToURI()?  It's simpler and having both the proxy
server
> and destination in the connection group might help debugging with net log
> output.

Is NetLog using |connection_group| already somewhere?  If not, then I don't see
why it's important that it be in |connection_group|.  We can easily add logging
that adds the proxy server info.  I don't think it needs to be part of the key. 
It just makes our key sizes larger.

http://codereview.chromium.org/1808001/diff/10002/15006
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/1808001/diff/10002/15006#newcode3737
net/http/http_network_transaction_unittest.cc:3737: for (size_t i = 0; i <
ARRAYSIZE_UNSAFE(tests); ++i) {
On 2010/04/28 21:18:09, vandebo wrote:
> Maybe this code can get de-dupped a bit?

Done.

http://codereview.chromium.org/1808001/diff/10002/15006#newcode3764
net/http/http_network_transaction_unittest.cc:3764: std::string allgroups =
tcp_conn_pool->last_group_name_received();
On 2010/04/28 21:18:09, vandebo wrote:
> This isn't allgroups any more.

Done.

[vandebo (ex-Chrome), 2010-04-28 22:07:24]

http://codereview.chromium.org/1808001/diff/10002/15004
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/1808001/diff/10002/15004#newcode779
net/http/http_network_transaction.cc:779: StringPrintf("socks%s/%s",
socks_version, connection_group.c_str());
On 2010/04/28 21:49:54, willchan wrote:
> Is NetLog using |connection_group| already somewhere?  If not, then I don't
see
> why it's important that it be in |connection_group|.  We can easily add
logging
> that adds the proxy server info.  I don't think it needs to be part of the
key. 
> It just makes our key sizes larger.

Yes, the SOCKET_POOL_CONNECT_JOB event stores the group. And it would be
simpler; assign socks_v5 as before and update connection_group. Connection_group
currently uses ToURI() without performance problems. But do as you like, this
isn't important to me.

[eroman, 2010-04-29 00:45:36]

lgtm

http://codereview.chromium.org/1808001/diff/25001/26002
File net/http/http_network_session.cc (right):

http://codereview.chromium.org/1808001/diff/25001/26002#newcode90
net/http/http_network_session.cc:90: 
nit: extra line

http://codereview.chromium.org/1808001/diff/25001/26004
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/1808001/diff/25001/26004#newcode760
net/http/http_network_transaction.cc:760: if
(proxy_info_.proxy_server().scheme() == ProxyServer::SCHEME_SOCKS5) {
Would be nice if you could re-use the stringizing code in ProxyServer, which
outputs things like 'socks5://' and 'socks://'.

If/else on the version is fairly fragile, especially if a new version gets
introduced. (Hmm, looks like the Socks pool takes a bool which isnt great
either).

Or if you don't care that these are human readable, then "%d",
static_cast<int>(version) should work.

[willchan no longer on Chromium, 2010-04-29 01:03:50]

Skipping wtc's review and landing now.  wtc, let me know if there's anything
else you want me to change here.

http://codereview.chromium.org/1808001/diff/25001/26002
File net/http/http_network_session.cc (right):

http://codereview.chromium.org/1808001/diff/25001/26002#newcode90
net/http/http_network_session.cc:90: 
On 2010/04/29 00:45:36, eroman wrote:
> nit: extra line

Done.

http://codereview.chromium.org/1808001/diff/25001/26004
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/1808001/diff/25001/26004#newcode760
net/http/http_network_transaction.cc:760: if
(proxy_info_.proxy_server().scheme() == ProxyServer::SCHEME_SOCKS5) {
On 2010/04/29 00:45:36, eroman wrote:
> Would be nice if you could re-use the stringizing code in ProxyServer, which
> outputs things like 'socks5://' and 'socks://'.
> 
> If/else on the version is fairly fragile, especially if a new version gets
> introduced. (Hmm, looks like the Socks pool takes a bool which isnt great
> either).
> 
> Or if you don't care that these are human readable, then "%d",
> static_cast<int>(version) should work.

As discussed offline, leaving this as is (there's no good way to get the
version, and the proxy server itself is redundant).

[wtc, 2010-05-03 19:01:53]

Will,

Sorry about the late review.  I'm sure vandebo and eroman
have done a good review of your CL.

I reviewed the code and the CL's description that you asked
me to look at.  They look good to me.

The first half of HttpNetworkTransaction::DoInitConnection
becomes simpler, but the second half becomes more complicated
because of the SOCKS code.  I wonder if we can hide it in
a method of the ProxyServer class.  Feel free to ignore
this nit.

http://codereview.chromium.org/1808001/diff/23002/13003
File net/http/http_network_session.cc (right):

http://codereview.chromium.org/1808001/diff/23002/13003#newcode31
net/http/http_network_session.cc:31: int g_max_sockets_per_proxy_server = 15;
I would use 16, since 15 is so close to a power of 2.

http://codereview.chromium.org/1808001/diff/23002/13005
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/1808001/diff/23002/13005#newcode749
net/http/http_network_transaction.cc:749: if (!proxy_info_.is_direct()) {
Nit: I prefer
    if (condition) {
      A;
    } else {
      B;
    }
over
    if (!condition) {
      B;
    } else {
      A;
    }

This is just a personal preference.

http://codereview.chromium.org/1808001/diff/23002/13005#newcode769
net/http/http_network_transaction.cc:769: StringPrintf("socks%s/%s",
socks_version, connection_group.c_str());
Should we also add a "http/" prefix to the connection_group
for normal (HTTP) proxies?