Clean up socket backup job when there are no requests.

Clean up socket backup job when there are no requests.
BUG=56532
TEST=ClientSocketPoolBaseTest.CancelBackupSocketAfterFinishingAllRequests

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

[Mike Belshe, 2010-09-27 19:00:49]

LGTM

http://codereview.chromium.org/3441034/diff/1/4
File net/socket/client_socket_pool_base_unittest.cc (right):

http://codereview.chromium.org/3441034/diff/1/4#newcode2070
net/socket/client_socket_pool_base_unittest.cc:2070:
connect_job_factory_->set_job_type(TestConnectJob::kMockPendingJob);
nit: line 2070 is unnecessary due to line 2065.

http://codereview.chromium.org/3441034/diff/1/4#newcode2084
net/socket/client_socket_pool_base_unittest.cc:2084:
MessageLoop::current()->RunAllPending();
nit: We're not actually verifying that the backup socket didn't fire here?

[willchan no longer on Chromium, 2010-09-27 19:05:04]

http://codereview.chromium.org/3441034/diff/1/4
File net/socket/client_socket_pool_base_unittest.cc (right):

http://codereview.chromium.org/3441034/diff/1/4#newcode2070
net/socket/client_socket_pool_base_unittest.cc:2070:
connect_job_factory_->set_job_type(TestConnectJob::kMockPendingJob);
On 2010/09/27 19:00:49, Mike Belshe wrote:
> nit: line 2070 is unnecessary due to line 2065.

It's necessary.  Waiting != Pending.  We need a job to be pending so that
handle2.WaitForResult() will complete.  If they're both Waiting, then it'll
never finish.

http://codereview.chromium.org/3441034/diff/1/4#newcode2084
net/socket/client_socket_pool_base_unittest.cc:2084:
MessageLoop::current()->RunAllPending();
On 2010/09/27 19:00:49, Mike Belshe wrote:
> nit: We're not actually verifying that the backup socket didn't fire here?

Well, previously we would hit a LOG(DFATAL) saying that the socket timer fired
even though we had no requests.  So, by doing RunAllPending(), we can verify
that the runnable method was revoked properly.

[Paweł Hajdan Jr., 2010-09-28 08:08:34]

Drive-by with a test comment.

http://codereview.chromium.org/3441034/diff/1/4
File net/socket/client_socket_pool_base_unittest.cc (right):

http://codereview.chromium.org/3441034/diff/1/4#newcode2083
net/socket/client_socket_pool_base_unittest.cc:2083:
PlatformThread::Sleep(ClientSocketPool::kMaxConnectRetryIntervalMs / 2 * 3);
Can we use some smarter method to wait than Sleep?

[Mike Belshe, 2010-09-28 15:13:55]

http://codereview.chromium.org/3441034/diff/1/4
File net/socket/client_socket_pool_base_unittest.cc (right):

http://codereview.chromium.org/3441034/diff/1/4#newcode2070
net/socket/client_socket_pool_base_unittest.cc:2070:
connect_job_factory_->set_job_type(TestConnectJob::kMockPendingJob);
On 2010/09/27 19:05:04, willchan wrote:
> On 2010/09/27 19:00:49, Mike Belshe wrote:
> > nit: line 2070 is unnecessary due to line 2065.
> 
> It's necessary.  Waiting != Pending.  We need a job to be pending so that
> handle2.WaitForResult() will complete.  If they're both Waiting, then it'll
> never finish.

Doh - I misread.  Somehow I read them as the same state.