Changed SPDY's ip connection pooling logic to only add the used IP,

Changed SPDY's ip connection pooling logic to only add the used IP,
rather than all the IPs for the hostname, to the SpdyAliasMap.

This is to fix the following problem:

When we establish a new SPDY connection, we add all IP addresses for
that hostname to our alias map. Therefore, if we connect to host foo
which maps to IPs X, Y, and Z, but the actual IP we connect to is X,
then if we try to get a SPDY connection to host bar which maps to
IPs Y and Z, then currently we will share the SPDY connection to host
foo on IP X.

BUG=89094
R=willchan
TEST=spdy unit tests
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=93105

[wtc, 2011-07-14 02:30:40]

http://codereview.chromium.org/7349023/diff/1003/net/spdy/spdy_session_pool.cc
File net/spdy/spdy_session_pool.cc (right):

http://codereview.chromium.org/7349023/diff/1003/net/spdy/spdy_session_pool.c...
net/spdy/spdy_session_pool.cc:166: return error;
Testing error == net::OK is too strict.
(*spdy_session)->InitializeWithSocket() may return
net::ERR_IO_PENDING, which is not a real error.

I think you should do it like this:
  if (g_enable_ip_pooling) {
    // Get connected address from spdy_session.
    AddressList addresses;
    if (connection->socket()->GetPeerAddress(&addresses) == OK)
      AddAliases(addresses, host_port_proxy_pair);
  }

  // Now we can initialize the session with the SSL socket.
  return (*spdy_session)->InitializeWithSocket(connection, is_secure,
                                               certificate_error_code);

[willchan no longer on Chromium, 2011-07-14 12:35:39]

I didn't look at your IP address changes in the tests, but do they add new
coverage to make sure we're not doing what we did previously?

http://codereview.chromium.org/7349023/diff/1010/net/spdy/spdy_session_pool.cc
File net/spdy/spdy_session_pool.cc (right):

http://codereview.chromium.org/7349023/diff/1010/net/spdy/spdy_session_pool.c...
net/spdy/spdy_session_pool.cc:153: // We have a new session.  Lookup the IP
addresses for this session so that
There should only be a single IP address. You should probably delete the
AddAliases() code and just do the AddressList=>IPEndPoint conversion here, and
assert that GetPeerAddress() returns only a single entry.

I need to think about proxies. I don't know if IP connection pooling works in
this case, because this GetPeerAddress() thing will report the proxy's address
instead of the origin server. Please add check to see if this is a direct
connection (it's part of the ProxyServer object in the HostPortProxyPair). The
condition should be added to the g_enable_ip_pooling condition check. This is an
additional bugfix.

[wtc, 2011-07-14 17:03:14]

http://codereview.chromium.org/7349023/diff/1010/net/spdy/spdy_session_pool.cc
File net/spdy/spdy_session_pool.cc (right):

http://codereview.chromium.org/7349023/diff/1010/net/spdy/spdy_session_pool.c...
net/spdy/spdy_session_pool.cc:153: // We have a new session.  Lookup the IP
addresses for this session so that
On 2011/07/14 12:35:39, willchan wrote:
>
> I need to think about proxies. I don't know if IP connection pooling works in
> this case, because this GetPeerAddress() thing will report the proxy's address
> instead of the origin server. Please add check to see if this is a direct
> connection (it's part of the ProxyServer object in the HostPortProxyPair). The
> condition should be added to the g_enable_ip_pooling condition check.

I came to the same conclusion after I went home last night
that this fix won't work if a proxy is used.

> This is an additional bugfix.

I wonder if mbelshe's algorithm also intended to make IP
connection pooling work when proxies are used.  It is bad
that we're still resolving hostnames when a proxy is used.

[willchan no longer on Chromium, 2011-07-14 17:10:51]

http://codereview.chromium.org/7349023/diff/1010/net/spdy/spdy_session_pool.cc
File net/spdy/spdy_session_pool.cc (right):

http://codereview.chromium.org/7349023/diff/1010/net/spdy/spdy_session_pool.c...
net/spdy/spdy_session_pool.cc:153: // We have a new session.  Lookup the IP
addresses for this session so that
On 2011/07/14 17:03:14, wtc wrote:
> On 2011/07/14 12:35:39, willchan wrote:
> >
> > I need to think about proxies. I don't know if IP connection pooling works
in
> > this case, because this GetPeerAddress() thing will report the proxy's
address
> > instead of the origin server. Please add check to see if this is a direct
> > connection (it's part of the ProxyServer object in the HostPortProxyPair).
The
> > condition should be added to the g_enable_ip_pooling condition check.
> 
> I came to the same conclusion after I went home last night
> that this fix won't work if a proxy is used.
> 
> > This is an additional bugfix.
> 
> I wonder if mbelshe's algorithm also intended to make IP
> connection pooling work when proxies are used.  It is bad
> that we're still resolving hostnames when a proxy is used.

We don't resolve the hostnames here. LookupAddresses() only uses cached entries.

[willchan no longer on Chromium, 2011-07-17 01:23:45]

http://codereview.chromium.org/7349023/diff/11001/net/http/http_network_trans...
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/7349023/diff/11001/net/http/http_network_trans...
net/http/http_network_transaction_unittest.cc:8705:
TEST_F(HttpNetworkTransactionTest, DISABLED_UseIPConnectionPooling) {
Is this going to be fixed?

http://codereview.chromium.org/7349023/diff/11001/net/http/http_network_trans...
net/http/http_network_transaction_unittest.cc:8848:
DISABLED_UseIPConnectionPoolingWithHostCacheExpiration) {
Is this going to be fixed?

http://codereview.chromium.org/7349023/diff/11001/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_pool_unittest.cc (left):

http://codereview.chromium.org/7349023/diff/11001/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_pool_unittest.cc:33: namespace {
If you use the SpdySessionPoolPeer, then you can restore this anonymous
namespace.

http://codereview.chromium.org/7349023/diff/11001/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_pool_unittest.cc (right):

http://codereview.chromium.org/7349023/diff/11001/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_pool_unittest.cc:713:
session_->spdy_session_pool()->AddAliases(test_hosts[0].pair);
Rather than doing this, you should probably just use the address that the
MockClientSocket will return, which you can see in socket_test_util.cc's
MockClientSocket::GetPeerAddress() which shows it returns 192.0.2.33. So you
should probably just use that address as an alias for test_hosts[0] and
test_hosts[2].

http://codereview.chromium.org/7349023/diff/11001/net/spdy/spdy_session_pool.cc
File net/spdy/spdy_session_pool.cc (right):

http://codereview.chromium.org/7349023/diff/11001/net/spdy/spdy_session_pool....
net/spdy/spdy_session_pool.cc:162: if (address) {
Should DCHECK() on this, because it must be available since we've got a
connected socket, so there must be *some* peer.

http://codereview.chromium.org/7349023/diff/11001/net/spdy/spdy_session_pool....
net/spdy/spdy_session_pool.cc:165: aliases_[endpoint] = host_port_proxy_pair;
If you take my suggestion on AddAlias() instead of AddAliases(), you can use
AddAlias() here.

http://codereview.chromium.org/7349023/diff/11001/net/spdy/spdy_session_pool.h
File net/spdy/spdy_session_pool.h (right):

http://codereview.chromium.org/7349023/diff/11001/net/spdy/spdy_session_pool....
net/spdy/spdy_session_pool.h:133: friend class SpdySessionTest;  // For testing.
Rather than add all of these friends, can you just use the SpdySessionPoolPeer?
That's the friended class that you can change to expose whatever functionality
you want. That way we don't need to add all these ugly/fragile FRIEND_TEST
macros.

http://codereview.chromium.org/7349023/diff/11001/net/spdy/spdy_session_pool....
net/spdy/spdy_session_pool.h:174: // addresses for |pair|. Used by unit tests
only.
This seems a bit weird, since for LookupAddresses(), that means we'd have to
pre-populate the HostCache in the test. I suggest just allowing AddAlias() to
add whatever value. Perhaps you should rename this to AddAlias(const IPEndPoint&
endpoint, const HostPortProxyPair& pair);

http://codereview.chromium.org/7349023/diff/11001/net/spdy/spdy_session_unitt...
File net/spdy/spdy_session_unittest.cc (right):

http://codereview.chromium.org/7349023/diff/11001/net/spdy/spdy_session_unitt...
net/spdy/spdy_session_unittest.cc:23: // This test has two variants, one for
each style of closing the connection.
Did you just move this to gain the friend access? If so, please just use
SpdySessionPoolPeer.

Is this test changed in any way? Or is it just moved? It'd be easier for me to
see the real diff if it didn't get moved.

http://codereview.chromium.org/7349023/diff/11001/net/spdy/spdy_session_unitt...
net/spdy/spdy_session_unittest.cc:221: spdy_session_pool->AddAliases(pair);
Why is this necessary?

http://codereview.chromium.org/7349023/diff/11001/net/spdy/spdy_session_unitt...
net/spdy/spdy_session_unittest.cc:334: spdy_session_pool->AddAliases(pair);
Why is this necessary?

http://codereview.chromium.org/7349023/diff/11001/net/spdy/spdy_session_unitt...
net/spdy/spdy_session_unittest.cc:421: spdy_session_pool->AddAliases(pair);
Why is this necessary?

http://codereview.chromium.org/7349023/diff/11001/net/spdy/spdy_session_unitt...
net/spdy/spdy_session_unittest.cc:522: spdy_session_pool->AddAliases(pair);
Ditto

[ramant (doing other things), 2011-07-17 08:12:30]

Hi willchan,
  Made the changes you have suggested (except using
MockClientSocket::GetPeerAddress. It was returning port 0 for the address and
LookupAddress which calls HostResolver was looking in the aliases_ with port 80.
Added a small routine to spdy_test_util.h to add host_port pair for port 80 with
Resolved addresses).

  Would appreciate your comments.

thanks much,
raman

http://codereview.chromium.org/7349023/diff/11001/net/http/http_network_trans...
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/7349023/diff/11001/net/http/http_network_trans...
net/http/http_network_transaction_unittest.cc:8705:
TEST_F(HttpNetworkTransactionTest, DISABLED_UseIPConnectionPooling) {
On 2011/07/17 01:23:45, willchan wrote:
> Is this going to be fixed?
Working on finding the problem. Will fix asap (top priority).

http://codereview.chromium.org/7349023/diff/11001/net/http/http_network_trans...
net/http/http_network_transaction_unittest.cc:8848:
DISABLED_UseIPConnectionPoolingWithHostCacheExpiration) {
On 2011/07/17 01:23:45, willchan wrote:
> Is this going to be fixed?

Working on finding the problem. Will fix asap (top priority).

http://codereview.chromium.org/7349023/diff/11001/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_pool_unittest.cc (left):

http://codereview.chromium.org/7349023/diff/11001/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_pool_unittest.cc:33: namespace {
On 2011/07/17 01:23:45, willchan wrote:
> If you use the SpdySessionPoolPeer, then you can restore this anonymous
> namespace.

Done.

http://codereview.chromium.org/7349023/diff/11001/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_pool_unittest.cc (right):

http://codereview.chromium.org/7349023/diff/11001/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_pool_unittest.cc:713:
session_->spdy_session_pool()->AddAliases(test_hosts[0].pair);
On 2011/07/17 01:23:45, willchan wrote:
> Rather than doing this, you should probably just use the address that the
> MockClientSocket will return, which you can see in socket_test_util.cc's
> MockClientSocket::GetPeerAddress() which shows it returns 192.0.2.33. So you
> should probably just use that address as an alias for test_hosts[0] and
> test_hosts[2].

Done.

http://codereview.chromium.org/7349023/diff/11001/net/spdy/spdy_session_pool.cc
File net/spdy/spdy_session_pool.cc (right):

http://codereview.chromium.org/7349023/diff/11001/net/spdy/spdy_session_pool....
net/spdy/spdy_session_pool.cc:162: if (address) {
On 2011/07/17 01:23:45, willchan wrote:
> Should DCHECK() on this, because it must be available since we've got a
> connected socket, so there must be *some* peer.

Done.

http://codereview.chromium.org/7349023/diff/11001/net/spdy/spdy_session_pool....
net/spdy/spdy_session_pool.cc:165: aliases_[endpoint] = host_port_proxy_pair;
On 2011/07/17 01:23:45, willchan wrote:
> If you take my suggestion on AddAlias() instead of AddAliases(), you can use
> AddAlias() here.

Done.

http://codereview.chromium.org/7349023/diff/11001/net/spdy/spdy_session_pool.h
File net/spdy/spdy_session_pool.h (right):

http://codereview.chromium.org/7349023/diff/11001/net/spdy/spdy_session_pool....
net/spdy/spdy_session_pool.h:133: friend class SpdySessionTest;  // For testing.
On 2011/07/17 01:23:45, willchan wrote:
> Rather than add all of these friends, can you just use the
SpdySessionPoolPeer?
> That's the friended class that you can change to expose whatever functionality
> you want. That way we don't need to add all these ugly/fragile FRIEND_TEST
> macros.

Done.

http://codereview.chromium.org/7349023/diff/11001/net/spdy/spdy_session_pool....
net/spdy/spdy_session_pool.h:174: // addresses for |pair|. Used by unit tests
only.
On 2011/07/17 01:23:45, willchan wrote:
> This seems a bit weird, since for LookupAddresses(), that means we'd have to
> pre-populate the HostCache in the test. I suggest just allowing AddAlias() to
> add whatever value. Perhaps you should rename this to AddAlias(const
IPEndPoint&
> endpoint, const HostPortProxyPair& pair);

Done.

http://codereview.chromium.org/7349023/diff/11001/net/spdy/spdy_session_unitt...
File net/spdy/spdy_session_unittest.cc (right):

http://codereview.chromium.org/7349023/diff/11001/net/spdy/spdy_session_unitt...
net/spdy/spdy_session_unittest.cc:23: // This test has two variants, one for
each style of closing the connection.
On 2011/07/17 01:23:45, willchan wrote:
> Did you just move this to gain the friend access? If so, please just use
> SpdySessionPoolPeer.
> 
> Is this test changed in any way? Or is it just moved? It'd be easier for me to
> see the real diff if it didn't get moved.

Done.

http://codereview.chromium.org/7349023/diff/11001/net/spdy/spdy_session_unitt...
net/spdy/spdy_session_unittest.cc:221: spdy_session_pool->AddAliases(pair);
On 2011/07/17 01:23:45, willchan wrote:
> Why is this necessary?

Done.

http://codereview.chromium.org/7349023/diff/11001/net/spdy/spdy_session_unitt...
net/spdy/spdy_session_unittest.cc:334: spdy_session_pool->AddAliases(pair);
On 2011/07/17 01:23:45, willchan wrote:
> Why is this necessary?

Done.

http://codereview.chromium.org/7349023/diff/11001/net/spdy/spdy_session_unitt...
net/spdy/spdy_session_unittest.cc:421: spdy_session_pool->AddAliases(pair);
On 2011/07/17 01:23:45, willchan wrote:
> Why is this necessary?

Done.

http://codereview.chromium.org/7349023/diff/11001/net/spdy/spdy_session_unitt...
net/spdy/spdy_session_unittest.cc:522: spdy_session_pool->AddAliases(pair);
On 2011/07/17 01:23:45, willchan wrote:
> Ditto

Done.

[willchan no longer on Chromium, 2011-07-17 12:44:46]

http://codereview.chromium.org/7349023/diff/12012/net/spdy/spdy_session_pool.cc
File net/spdy/spdy_session_pool.cc (right):

http://codereview.chromium.org/7349023/diff/12012/net/spdy/spdy_session_pool....
net/spdy/spdy_session_pool.cc:362: if (!g_enable_ip_pooling)
I think it should be DCHECK(g_enable_ip_pooling). We shouldn't be calling this
unless ip connection pooling is enabled, right?

http://codereview.chromium.org/7349023/diff/12012/net/spdy/spdy_session_unitt...
File net/spdy/spdy_session_unittest.cc (right):

http://codereview.chromium.org/7349023/diff/12012/net/spdy/spdy_session_unitt...
net/spdy/spdy_session_unittest.cc:484: pool_peer.AddAliases(test_hosts[0].pair);
Sorry, I wasn't clear. I'd prefer you just do AddAlias() rather than
AddAliases(). The reason is that this more closely mirrors what the code is
actually doing now. When we create a new SpdySession, we add only one
ipport=>hostname mapping in InitalizeWithSocket(). That's why I think it's wrong
to have an AddAliases() which adds multiple mappings. I think you should only
add the single mapping.

That said, I think we can also fix this by changing
MockClientsocket::GetPeerAddress() to return port 80. That's the more typical
port to return. We shouldn't be returning 0. I don't think anything will break
if you do this, but if something breaks and it looks nontrivial to fix, don't
worry about it and just use pool_peer.AddAlias().

[ramant (doing other things), 2011-07-17 19:45:34]

Hi willchan,
  Many many thanks for the very quick review and very good comments. Made the
changes you have suggested. 

Implemented the first solution you had suggested. Will investigate how to return
port 80 from MockClientsocket::GetPeerAddress().

thanks much,
raman

http://codereview.chromium.org/7349023/diff/12012/net/spdy/spdy_session_pool.cc
File net/spdy/spdy_session_pool.cc (right):

http://codereview.chromium.org/7349023/diff/12012/net/spdy/spdy_session_pool....
net/spdy/spdy_session_pool.cc:362: if (!g_enable_ip_pooling)
On 2011/07/17 12:44:47, willchan wrote:
> I think it should be DCHECK(g_enable_ip_pooling). We shouldn't be calling this
> unless ip connection pooling is enabled, right?

Done.

http://codereview.chromium.org/7349023/diff/12012/net/spdy/spdy_session_unitt...
File net/spdy/spdy_session_unittest.cc (right):

http://codereview.chromium.org/7349023/diff/12012/net/spdy/spdy_session_unitt...
net/spdy/spdy_session_unittest.cc:484: pool_peer.AddAliases(test_hosts[0].pair);
On 2011/07/17 12:44:47, willchan wrote:
> Sorry, I wasn't clear. I'd prefer you just do AddAlias() rather than
> AddAliases(). The reason is that this more closely mirrors what the code is
> actually doing now. When we create a new SpdySession, we add only one
> ipport=>hostname mapping in InitalizeWithSocket(). That's why I think it's
wrong
> to have an AddAliases() which adds multiple mappings. I think you should only
> add the single mapping.
> 
> That said, I think we can also fix this by changing
> MockClientsocket::GetPeerAddress() to return port 80. That's the more typical
> port to return. We shouldn't be returning 0. I don't think anything will break
> if you do this, but if something breaks and it looks nontrivial to fix, don't
> worry about it and just use pool_peer.AddAlias().

Done.

[willchan no longer on Chromium, 2011-07-17 19:52:21]

Cool, LGTM once you fix the DISABLED tests.

[wtc, 2011-07-19 00:11:15]

I only reviewed spdy_session_pool.{h,cc}.  The changes look
correct to me.

http://codereview.chromium.org/7349023/diff/17020/net/spdy/spdy_session_pool.cc
File net/spdy/spdy_session_pool.cc (right):

http://codereview.chromium.org/7349023/diff/17020/net/spdy/spdy_session_pool....
net/spdy/spdy_session_pool.cc:156: // proxy's address instead of the origin
server, added the check to see if
Nit: added the check to see => check

http://codereview.chromium.org/7349023/diff/17020/net/spdy/spdy_session_pool....
net/spdy/spdy_session_pool.cc:160: if
(connection->socket()->GetPeerAddress(&addresses) == net::OK) {
Remove "net::" because this is in the net namespace.

http://codereview.chromium.org/7349023/diff/17020/net/spdy/spdy_session_pool.h
File net/spdy/spdy_session_pool.h (right):

http://codereview.chromium.org/7349023/diff/17020/net/spdy/spdy_session_pool....
net/spdy/spdy_session_pool.h:166: // // Add |endpoint| as IP-equivalent address
for |pair|.
Nit: add "an" before "IP-equivalent".

[ramant (doing other things), 2011-07-19 03:48:46]

hi willchan and wan-teh,
  Made the changes you had suggested. Could you please review these changes?

thanks very much,
raman

http://codereview.chromium.org/7349023/diff/17020/net/spdy/spdy_session_pool.cc
File net/spdy/spdy_session_pool.cc (right):

http://codereview.chromium.org/7349023/diff/17020/net/spdy/spdy_session_pool....
net/spdy/spdy_session_pool.cc:156: // proxy's address instead of the origin
server, added the check to see if
On 2011/07/19 00:11:19, wtc wrote:
> Nit: added the check to see => check

Done.

http://codereview.chromium.org/7349023/diff/17020/net/spdy/spdy_session_pool....
net/spdy/spdy_session_pool.cc:160: if
(connection->socket()->GetPeerAddress(&addresses) == net::OK) {
On 2011/07/19 00:11:19, wtc wrote:
> Remove "net::" because this is in the net namespace.

Done.

http://codereview.chromium.org/7349023/diff/17020/net/spdy/spdy_session_pool.h
File net/spdy/spdy_session_pool.h (right):

http://codereview.chromium.org/7349023/diff/17020/net/spdy/spdy_session_pool....
net/spdy/spdy_session_pool.h:166: // // Add |endpoint| as IP-equivalent address
for |pair|.
On 2011/07/19 00:11:19, wtc wrote:
> Nit: add "an" before "IP-equivalent".

Done.

[willchan no longer on Chromium, 2011-07-19 14:46:58]

LGTM, thanks

[wtc, 2011-07-19 22:12:36]

I only reviewed http_network_transaction_unittest.cc and
spdy_session_pool.{h,cc}.  I suggest some changes below.

http://codereview.chromium.org/7349023/diff/20001/net/http/http_network_trans...
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/7349023/diff/20001/net/http/http_network_trans...
net/http/http_network_transaction_unittest.cc:8713: };
The need to preload this entry, with the magic hostname
www.google.com and magic IP address 127.0.0.1, into the
HostCache is very non-obvious.  Please document why.
Otherwise a future maintainer will need to read the code
review comments to reconstruct the reason.

http://codereview.chromium.org/7349023/diff/20001/net/http/http_network_trans...
net/http/http_network_transaction_unittest.cc:8718: test_hosts[i].iplist, "");
Align this line with the argument on the previous line.

http://codereview.chromium.org/7349023/diff/20001/net/http/http_network_trans...
net/http/http_network_transaction_unittest.cc:8721: // This test requires that
the HostResolver cache be populated.  Normal
By "This test", which test are you referring to?  Did you
copy this function from some unit test?

http://codereview.chromium.org/7349023/diff/20001/net/spdy/spdy_session_pool.cc
File net/spdy/spdy_session_pool.cc (right):

http://codereview.chromium.org/7349023/diff/20001/net/spdy/spdy_session_pool....
net/spdy/spdy_session_pool.cc:162: AddAlias(address, host_port_proxy_pair);
Nit: you can just pass addresses.head() to AddAlias and
remove the curly braces around the inner if statement.

http://codereview.chromium.org/7349023/diff/20001/net/spdy/spdy_session_pool.h
File net/spdy/spdy_session_pool.h (right):

http://codereview.chromium.org/7349023/diff/20001/net/spdy/spdy_session_pool....
net/spdy/spdy_session_pool.h:166: // Add |endpoint| as an IP-equivalent address
for |pair|.
|endpoint| => |address|

[ramant (doing other things), 2011-07-25 23:36:06]

Hi Wan-Teh,
  Added comments and did a minor cleanup of the comments in  
http_network_transaction_unittest.cc and submitted the following CL.

http://codereview.chromium.org/7471056/

thanks,
raman

http://codereview.chromium.org/7349023/diff/20001/net/http/http_network_trans...
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/7349023/diff/20001/net/http/http_network_trans...
net/http/http_network_transaction_unittest.cc:8713: };
On 2011/07/19 22:12:36, wtc wrote:
> The need to preload this entry, with the magic hostname
> http://www.google.com and magic IP address 127.0.0.1, into the
> HostCache is very non-obvious.  Please document why.
> Otherwise a future maintainer will need to read the code
> review comments to reconstruct the reason.

Done.

http://codereview.chromium.org/7349023/diff/20001/net/http/http_network_trans...
net/http/http_network_transaction_unittest.cc:8718: test_hosts[i].iplist, "");
On 2011/07/19 22:12:36, wtc wrote:
> Align this line with the argument on the previous line.

Done.

http://codereview.chromium.org/7349023/diff/20001/net/http/http_network_trans...
net/http/http_network_transaction_unittest.cc:8721: // This test requires that
the HostResolver cache be populated.  Normal
On 2011/07/19 22:12:36, wtc wrote:
> By "This test", which test are you referring to?  Did you
> copy this function from some unit test?

Done.

http://codereview.chromium.org/7349023/diff/20001/net/spdy/spdy_session_pool.cc
File net/spdy/spdy_session_pool.cc (right):

http://codereview.chromium.org/7349023/diff/20001/net/spdy/spdy_session_pool....
net/spdy/spdy_session_pool.cc:162: AddAlias(address, host_port_proxy_pair);
On 2011/07/19 22:12:36, wtc wrote:
> Nit: you can just pass addresses.head() to AddAlias and
> remove the curly braces around the inner if statement.

Done.

http://codereview.chromium.org/7349023/diff/20001/net/spdy/spdy_session_pool.h
File net/spdy/spdy_session_pool.h (right):

http://codereview.chromium.org/7349023/diff/20001/net/spdy/spdy_session_pool....
net/spdy/spdy_session_pool.h:166: // Add |endpoint| as an IP-equivalent address
for |pair|.
On 2011/07/19 22:12:36, wtc wrote:
> |endpoint| => |address|

Done.