Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in

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

Can't Edit
Can't Publish+Mail
Start Review
8 years, 1 month ago by eroman
6 years, 3 months ago
CC:, darin (slow to review), willchan no longer on Chromium


Clean-up, to remove dependency on SingleThreadedProxyResolver. This was a TODO generated in <>; This simplifies the test code since everything is running on one thread. BUG=, Committed:

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) Patch
M net/proxy/ View 1 2 3 4 22 chunks +606 lines, -743 lines 0 comments Download
Commit queue not available (can’t edit this change).


Total messages: 3 (0 generated)
8 years, 1 month ago (2009-07-27 06:49:12 UTC) #1
willchan no longer on Chromium
LGTM File net/proxy/ (right): Line 5: #include "googleurl/src/gurl.h" You need base/logging.h and <vector> ...
8 years, 1 month ago (2009-07-28 04:19:46 UTC) #2
8 years, 1 month ago (2009-07-28 05:45:45 UTC) #3
thanks for the review!
File net/proxy/ (right):
Line 5: #include "googleurl/src/gurl.h"
On 2009/07/28 04:19:46, willchan wrote:
> You need base/logging.h and <vector>

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.
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
>  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.

Line 231: EXPECT_EQ(1u, resolver->pending_requests().size());
On 2009/07/28 04:19:46, willchan wrote:


Did a search replace for all "EXPECT_EQ([0-9]u,"
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.
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.

Removed the comment.
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/

Good spot, wasn't even part of the CL deltas!
Line 1005:
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 b40b6558b