Make TCPClientSocketPool own the ConnectingSockets.

Make TCPClientSocketPool own the ConnectingSockets.
Re-enable the disabled URLRequest tests.
BUG=http://crbug.com/13952
TEST=covered by existing tests

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

[eroman, 2009-06-14 06:41:59]

lgtm.

sorry i couldnt get to this review on friday (was trying to finish the webkit
update).

http://codereview.chromium.org/126065/diff/1/2
File net/base/tcp_client_socket_pool.cc (right):

http://codereview.chromium.org/126065/diff/1/2#newcode54
Line 54: // We don't worry about cancelling the TCP connect since ~ClientSocket
will
This comment is duplicated.

http://codereview.chromium.org/126065/diff/1/2#newcode78
Line 78: NOTREACHED();
If this is a logic failure, then why not just DCHECK(group_it !=
pool_->group_map_.end()) ?

http://codereview.chromium.org/126065/diff/1/2#newcode88
Line 88: NOTREACHED();
Same question as above. It is strange to have logic after a NOTREACHED(). If
anything seems better to make that a CHECK if you suspect it can be reached.

http://codereview.chromium.org/126065/diff/1/2#newcode421
Line 421: delete connecting_socket_map_[handle];
how about doing a find once, and then using that iterator to do the erase
afterwards?

http://codereview.chromium.org/126065/diff/1/4
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/126065/diff/1/4#newcode50
Line 50: // http://crbug.com/13952
Can you also remove the TODO(eroman): hacks that turn off caching? (search for
13952 in this file, and url_request_unittest.h)

[willchan no longer on Chromium, 2009-06-15 18:01:48]

http://codereview.chromium.org/126065/diff/1/2
File net/base/tcp_client_socket_pool.cc (right):

http://codereview.chromium.org/126065/diff/1/2#newcode54
Line 54: // We don't worry about cancelling the TCP connect since ~ClientSocket
will
On 2009/06/14 06:41:59, eroman wrote:
> This comment is duplicated.

Done.

http://codereview.chromium.org/126065/diff/1/2#newcode78
Line 78: NOTREACHED();
On 2009/06/14 06:41:59, eroman wrote:
> If this is a logic failure, then why not just DCHECK(group_it !=
> pool_->group_map_.end()) ?

Done.

http://codereview.chromium.org/126065/diff/1/2#newcode88
Line 88: NOTREACHED();
On 2009/06/14 06:41:59, eroman wrote:
> Same question as above. It is strange to have logic after a NOTREACHED(). If
> anything seems better to make that a CHECK if you suspect it can be reached.

Done.

http://codereview.chromium.org/126065/diff/1/2#newcode421
Line 421: delete connecting_socket_map_[handle];
On 2009/06/14 06:41:59, eroman wrote:
> how about doing a find once, and then using that iterator to do the erase
> afterwards?

Done.

http://codereview.chromium.org/126065/diff/1/4
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/126065/diff/1/4#newcode50
Line 50: // http://crbug.com/13952
On 2009/06/14 06:41:59, eroman wrote:
> Can you also remove the TODO(eroman): hacks that turn off caching? (search for
> 13952 in this file, and url_request_unittest.h)

Done.