Chromium Code Reviews
Help | Chromium Project | Sign in
(351)

Issue 160155: Clean-up proxy_service_unittest.cc, to remove dependency on SingleThreadedPro... (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 9 months ago by eroman
Modified:
2 years, 10 months ago
Reviewers:
willchan
CC:
chromium-reviews_googlegroups.com, darin, willchan
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : Add checks using cancelled_requests() #

Patch Set 3 : Fix unused variable #

Total comments: 34

Patch Set 4 : Address willchan's comments #

Patch Set 5 : Fixe header order #

Unified diffs Side-by-side diffs Delta from patch set Stats (+606 lines, -743 lines) Lint Patch
M net/proxy/proxy_service_unittest.cc View 1 2 3 4 22 chunks +606 lines, -743 lines 0 comments 0 errors Download
Commit:

Messages

Total messages: 3
eroman
4 years, 9 months ago #1
willchan
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> ...
4 years, 9 months ago #2
eroman
4 years, 9 months ago #3
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()|
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1280:2d3e6564b7b6