Clean-up proxy_service_unittest.cc, to remove dependency on SingleThreadedPro...

Clean-up proxy_service_unittest.cc, to remove dependency on SingleThreadedProxyResolver.

This was a TODO generated in <http://codereview.chromium.org/149525>;

This simplifies the test code since everything is running on one thread.

BUG=http://crbug.com/11746, http://crbug.com/11079


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

[willchan no longer on Chromium, 2009-07-28 04:19:46]

LGTM

http://codereview.chromium.org/160155/diff/1004/1005
File net/proxy/proxy_service_unittest.cc (right):

http://codereview.chromium.org/160155/diff/1004/1005#newcode5
Line 5: #include "googleurl/src/gurl.h"
You need base/logging.h and <vector>

http://codereview.chromium.org/160155/diff/1004/1005#newcode54
Line 54: net::CompletionCallback* callback = callback_;
Should you set callback_ to NULL?

http://codereview.chromium.org/160155/diff/1004/1005#newcode101
Line 101: RequestsList& pending_requests() {
I think you can make this a const member function and return const RequestsList.
 You don't seem to ever modify RequestsList, although you do call non-const
member functions on Requests within the list, which I believe should be fine
even if you have a const RequestsList, since you can only get const
scoped_refptrs, but -> is a const operator in scoped_refptr.

http://codereview.chromium.org/160155/diff/1004/1005#newcode231
Line 231: EXPECT_EQ(1u, resolver->pending_requests().size());
ASSERT_EQ

http://codereview.chromium.org/160155/diff/1004/1005#newcode258
Line 258: EXPECT_EQ(1u, resolver->pending_requests().size());
ASSERT_EQ

http://codereview.chromium.org/160155/diff/1004/1005#newcode294
Line 294: EXPECT_EQ(1u, resolver->pending_requests().size());
ASSERT_EQ

http://codereview.chromium.org/160155/diff/1004/1005#newcode297
Line 297: // Fail the first resolve request in MockAsyncProxyResolver.
Rather than calling CompleteNow(), shouldn't you be failing the PAC download?

http://codereview.chromium.org/160155/diff/1004/1005#newcode316
Line 316: EXPECT_EQ(1u, resolver->pending_requests().size());
ASSERT_EQ

http://codereview.chromium.org/160155/diff/1004/1005#newcode348
Line 348: EXPECT_EQ(1u, resolver->pending_requests().size());
ASSERT_EQ

http://codereview.chromium.org/160155/diff/1004/1005#newcode373
Line 373: EXPECT_EQ(1u, resolver->pending_requests().size());
ASSERT_EQ

http://codereview.chromium.org/160155/diff/1004/1005#newcode401
Line 401: EXPECT_EQ(net::ERR_FAILED, rv);  // We try direct.
This comment seems wrong.  We already tried direct.

http://codereview.chromium.org/160155/diff/1004/1005#newcode425
Line 425: EXPECT_EQ(1u, resolver->pending_requests().size());
ASSERT_EQ

http://codereview.chromium.org/160155/diff/1004/1005#newcode447
Line 447: EXPECT_EQ(1u, resolver->pending_requests().size());
ASSERT_EQ

http://codereview.chromium.org/160155/diff/1004/1005#newcode468
Line 468: // We fake anothe error. It should go back to the first proxy.
s/anothe/another/

http://codereview.chromium.org/160155/diff/1004/1005#newcode474
Line 474: EXPECT_EQ(1u, resolver->pending_requests().size());
ASSERT_EQ

http://codereview.chromium.org/160155/diff/1004/1005#newcode504
Line 504: EXPECT_EQ(1u, resolver->pending_requests().size());
ASSERT_EQ

http://codereview.chromium.org/160155/diff/1004/1005#newcode531
Line 531: EXPECT_EQ(1u, resolver->pending_requests().size());
ASSERT_EQ

http://codereview.chromium.org/160155/diff/1004/1005#newcode558
Line 558: EXPECT_EQ(1u, resolver->pending_requests().size());
ASSERT_EQ

http://codereview.chromium.org/160155/diff/1004/1005#newcode897
Line 897: EXPECT_EQ(1u, resolver->pending_requests().size());
ASSERT_EQ

http://codereview.chromium.org/160155/diff/1004/1005#newcode906
Line 906: EXPECT_EQ(2u, resolver->pending_requests().size());
ASSERT_EQ

http://codereview.chromium.org/160155/diff/1004/1005#newcode914
Line 914: EXPECT_EQ(3u, resolver->pending_requests().size());
ASSERT_EQ

http://codereview.chromium.org/160155/diff/1004/1005#newcode920
Line 920: EXPECT_EQ(2u, resolver->pending_requests().size());
ASSERT_EQ

http://codereview.chromium.org/160155/diff/1004/1005#newcode937
Line 937: EXPECT_EQ(1u, resolver->cancelled_requests().size());
ASSERT_EQ

http://codereview.chromium.org/160155/diff/1004/1005#newcode992
Line 992: EXPECT_EQ(3u, resolver->pending_requests().size());
ASSERT_EQ

http://codereview.chromium.org/160155/diff/1004/1005#newcode1005
Line 1005:
resolver->pending_requests()[0]->results()->UseNamedProxy("request2:80");
What happened to pending request 1?

http://codereview.chromium.org/160155/diff/1004/1005#newcode1072
Line 1072: EXPECT_EQ(1u, resolver->pending_requests().size());
ASSERT_EQ

[eroman, 2009-07-28 05:45:45]

thanks for the review!

http://codereview.chromium.org/160155/diff/1004/1005
File net/proxy/proxy_service_unittest.cc (right):

http://codereview.chromium.org/160155/diff/1004/1005#newcode5
Line 5: #include "googleurl/src/gurl.h"
On 2009/07/28 04:19:46, willchan wrote:
> You need base/logging.h and <vector>

Done.

http://codereview.chromium.org/160155/diff/1004/1005#newcode54
Line 54: net::CompletionCallback* callback = callback_;
On 2009/07/28 04:19:46, willchan wrote:
> Should you set callback_ to NULL?

|this| is deleted in the very next line (before the callback is run), so I don't
think it is worthwhile NULLing it.

http://codereview.chromium.org/160155/diff/1004/1005#newcode101
Line 101: RequestsList& pending_requests() {
On 2009/07/28 04:19:46, willchan wrote:
> I think you can make this a const member function and return const
RequestsList.
>  You don't seem to ever modify RequestsList, although you do call non-const
> member functions on Requests within the list, which I believe should be fine
> even if you have a const RequestsList, since you can only get const
> scoped_refptrs, but -> is a const operator in scoped_refptr.

Done.

http://codereview.chromium.org/160155/diff/1004/1005#newcode231
Line 231: EXPECT_EQ(1u, resolver->pending_requests().size());
On 2009/07/28 04:19:46, willchan wrote:
> ASSERT_EQ

Done.

Did a search replace for all "EXPECT_EQ([0-9]u,"

http://codereview.chromium.org/160155/diff/1004/1005#newcode297
Line 297: // Fail the first resolve request in MockAsyncProxyResolver.
On 2009/07/28 04:19:46, willchan wrote:
> Rather than calling CompleteNow(), shouldn't you be failing the PAC download?

This test was poorly named.  It predates when ProxyService knew how to actually
download a PAC, so the title makes little sense today.

In the old days, the winhttp based proxy resolver was the only ProxyResolver
implementation, and there was no concept of a ProxyResolver that
|expects_pac_bytes|. Hence the PAC script would be downloaded internally by the
ProxyResolver, and a failure from the download would cause a corresponding
failure in ProxyResolver::GetProxyForURL.

I have renamed this test and its comment to make sense in the new world.

http://codereview.chromium.org/160155/diff/1004/1005#newcode401
Line 401: EXPECT_EQ(net::ERR_FAILED, rv);  // We try direct.
On 2009/07/28 04:19:46, willchan wrote:
> This comment seems wrong.  We already tried direct.

Done.
Removed the comment.

http://codereview.chromium.org/160155/diff/1004/1005#newcode468
Line 468: // We fake anothe error. It should go back to the first proxy.
On 2009/07/28 04:19:46, willchan wrote:
> s/anothe/another/

Done.
Good spot, wasn't even part of the CL deltas!

http://codereview.chromium.org/160155/diff/1004/1005#newcode1005
Line 1005:
resolver->pending_requests()[0]->results()->UseNamedProxy("request2:80");
On 2009/07/28 04:19:46, willchan wrote:
> What happened to pending request 1?

After the pending request 0 is completed in the line above, it is removed from
the vector.

So when we reach this line, the third request is the only item left in the
pending list.

Added a comment to de-mystify this suspicious looking line :)

// Note that as we complete requests, they shift up in |pending_requests()|