Remove the concept of threading from ProxyService, and move it into the ProxyResolver dependency.
ProxyResolver may now complete requests asynchronously, and is defined to handle multiple requests.
The code from ProxyService that queued requests onto the single PAC thread has moved into SingleThreadedProxyResolver.
This refactor lays the groundwork for:
(1) http://crbug.com/11746 -- Run PAC proxy resolving out of process.
(Can inject an IPC bridge implementation of ProxyResolver)
(2) http://crbug.com/11079 -- Run PAC proxy resolving on multiple threads.
(Can implement a MultithreadedProxyResolver type class; still complications around v8 threadsafety though).
BUG=http://crbug.com/11746, http://crbug.com/11079
TEST=existing unit-tests.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=21631

http://codereview.chromium.org/149525/diff/89/1075
File net/proxy/proxy_resolver.h (right):
http://codereview.chromium.org/149525/diff/89/1075#newcode35
Line 35: // for |url|. If the request will complete complete asynchronously
returns
On 2009/07/14 18:34:46, willchan wrote:
> Too many "complete"s. Also, the grammar's off. Perhaps you need an "it" as
the
> subject for the next clause.
Done.
Simplified this comment.
http://codereview.chromium.org/149525/diff/89/1075#newcode42
Line 42: RequestHandle* request) = 0;
On 2009/07/14 18:34:46, willchan wrote:
> You need to explain the use of |request| in your documentation. I presume
it's
> only set in the asynchronous case?
Done.
http://codereview.chromium.org/149525/diff/89/1075#newcode42
Line 42: RequestHandle* request) = 0;
On 2009/07/14 22:10:04, willchan wrote:
> On 2009/07/14 18:34:46, willchan wrote:
> > You need to explain the use of |request| in your documentation. I presume
> it's
> > only set in the asynchronous case?
>
> I noticed that in SingleThreadProxyResolver there is a check to see if this is
> NULL. If that's valid, perhaps it should be documented as well.
That is a higher-level check. SingleThreadedProxyResolver always calls its
wrapped ProxyResolver with NULL callback and request. This way if the wrapped
ProxyResolver tries to complete asynchronously you will get a crash, since it
doesn't make sense to wrap an already async ProxyResolver implementation.
http://codereview.chromium.org/149525/diff/89/1075#newcode50
Line 50: // |expects_pac_bytes| was set to false, than |bytes| willalways be the
empty
On 2009/07/14 18:34:46, willchan wrote:
> s/willalways/will always/
Done.
http://codereview.chromium.org/149525/diff/89/1075#newcode51
Line 51: // string. Otherwise if |expects_pac_bytes| was set to true true, the
On 2009/07/14 18:34:46, willchan wrote:
> You have an extra "true"
True dat.
(Done).
http://codereview.chromium.org/149525/diff/89/1075#newcode56
Line 56: virtual void SetPacScript(const GURL& pac_url, const std::string&
bytes) = 0;
On 2009/07/14 18:34:46, willchan wrote:
> I wonder if you should split this into two member functions.
>
> public:
> void SetPacScript(const GURL& pac_url, const std::string& bytes) {
> DCHECK(expect_pac_bytes() && !bytes.empty() ||
> !expect_pac_bytes() && bytes.empty());
> SetPacScriptInternal(pac_url, bytes);
> }
>
> private:
> virtual void SetPacScriptInternal(const GURL& pac_url, const std::string&
> bytes) = 0;
>
> This way the assertion is always done correctly for all derived classes.
Done.
This was indeed a mess -- I should have realized that if I need that much
comments to explain it, it was bad design.
I have split it up into four methods:
void SetPacScriptByUrl(const GURL& pac_url) {
DCHECK(!expects_pac_bytes());
SetPacScriptByUrlInternal(pac_url);
}
void SetPacScriptByData(const std::string& bytes) {
DCHECK(expects_pac_bytes());
SetPacScriptByDataInternal(bytes);
}
private:
// Called to set the PAC script backend to use. If |pac_url| is invalid,
// this is a request to use WPAD (auto detect).
virtual void SetPacScriptByUrlInternal(const GURL& pac_url) {
NOTREACHED();
}
// Called to set the PAC script backend to use. |bytes| may be empty if the
// fetch failed, or if the fetch returned no content.
virtual void SetPacScriptByDataInternal(const std::string& bytes) {
NOTREACHED();
}
http://codereview.chromium.org/149525/diff/89/1075#newcode62
Line 62: bool expects_pac_bytes_;
On 2009/07/14 18:34:46, willchan wrote:
> Can this be const? Also, it should be private:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Access....
Done.
http://codereview.chromium.org/149525/diff/89/1075#newcode63
Line 63: };
On 2009/07/14 18:34:46, willchan wrote:
> DISALLOW_COPY_AND_ASSIGN
Done.
http://codereview.chromium.org/149525/diff/89/1069
File net/proxy/proxy_resolver_v8.cc (right):
http://codereview.chromium.org/149525/diff/89/1069#newcode355
Line 355: CompletionCallback* callback,
On 2009/07/14 18:34:46, willchan wrote:
> Perhaps you should comment out |callback| and |request| so it's clear that
> they're unused?
Done.
http://codereview.chromium.org/149525/diff/89/1068
File net/proxy/proxy_resolver_winhttp.cc (right):
http://codereview.chromium.org/149525/diff/89/1068#newcode57
Line 57: CompletionCallback* callback,
On 2009/07/14 18:34:46, willchan wrote:
> Comment out |callback| and |request| to indicate they are unused?
Done.
http://codereview.chromium.org/149525/diff/89/1064
File net/proxy/proxy_service.cc (right):
http://codereview.chromium.org/149525/diff/89/1064#newcode47
Line 47: ProxyResolverNull() : ProxyResolver(false /*expects_pac_bytes*/) {}
On 2009/07/14 18:34:46, willchan wrote:
> This comment can be confusing. I understand you mean that the variable is
> expects_pac_bytes, but when someone reads it (if they haven't
> memorized/remembered that the constructor variable is called
expects_pac_bytes,
> they might think that false means that pac bytes are expected.
I like the style of writing only variable names in c-style argument comments,
and writing bigger picture comments as a regular comment block above the call.
I find this to be the most consistent style, since it works the same for
commenting out un-used parameters, or for labeling boolean parameters.
http://codereview.chromium.org/149525/diff/89/1064#newcode78
Line 78: class ProxyService::PacRequest
On 2009/07/14 22:10:04, willchan wrote:
> There seems to be tight coupling between ProxyService::PacRequest and
> ProxyService. Most of this seems to come from QueryDidComplete() and
> QueryComplete. My personal inclination is that we can provide more
> encapsulation if the request object is agnostic of the requester. He simply
> runs a callback/delegate on completion.
>
> This is somewhat of a large suggestion, so I'm fine with you disagreeing or
> postponing.
The main issue is that since ProxyResolver is multi-request, a single
OnIOComplete(int rv) style method doesn't work.
So I use this helper object to bind a per-request callback object (notice that
it actually extends CompletionCallback). So by design, it needs to ping back to
the ProxyService.
http://codereview.chromium.org/149525/diff/89/1064#newcode111
Line 111: bool is_started() const { return !!resolve_job_; }
On 2009/07/14 22:10:04, willchan wrote:
> I assume you didn't mean to have two !'s.
Actually this is intentional.
Conceptually this is the same as:
static_cast<bool>(resolve_job);
The problem was VS poohs all over me when using the static-cast or implicit cast
versions. It gives me:
forcing value to bool 'true' or 'false' (performance warning)
Added a comment for time being, if you know how to work around this in another
way let me know.
http://codereview.chromium.org/149525/diff/89/1064#newcode144
Line 144: // Reset the state associated with in-progress-resolve.
On 2009/07/14 22:10:04, willchan wrote:
> Is resetting the state necessary? I'm not necessarily against it, I'm just
> trying to understand what its purpose is.
This is just book-keeping, to help catch problems (like trying to cancel the job
after it has already completed).
http://codereview.chromium.org/149525/diff/89/1064#newcode272
Line 272: pending_requests_.push_back(req);
On 2009/07/14 22:10:04, willchan wrote:
> I wonder if it's worth an assertion here that the request does not already
exist
> in |pending_requests_|.
Done.
http://codereview.chromium.org/149525/diff/89/1064#newcode369
Line 369: PacRequest* req = (*it).get();
On 2009/07/14 22:10:04, willchan wrote:
> any reason you don't just do it->get()?
Probably because I was dropped on my head as a baby.
Done.
http://codereview.chromium.org/149525/diff/89/1064#newcode375
Line 375: bool ProxyService::IsResolverReadyForRequests() {
On 2009/07/14 22:10:04, willchan wrote:
> My personal _preference_ (I'll leave the choice to you) is that
> Foo::IsSomethingTrue() should be a const member function. It doesn't seem
> intuitive to me that a member function named like this would have side
effects.
Done.
I have changed it to:
bool PrepareResolverForRequests();
Note that I will be changing this soon anyways, as part of http://crbug.com/9985
(Since I need more complicated fallback logic, will probably end up making with
a concept of "canary" requests coupled with a state-machine).
http://codereview.chromium.org/149525/diff/89/1065
File net/proxy/proxy_service.h (right):
http://codereview.chromium.org/149525/diff/89/1065#newcode225
Line 225: typedef std::vector<scoped_refptr<PacRequest> > PendingRequests;
On 2009/07/14 22:10:04, willchan wrote:
> Why did you change it not to be FIFO? If a set's what you want, why not
> std::set?
I have put a TODO(eroman) about using std::set.
The issue I need to address is that some of ProxyService's unit-tests assume
requests will complete in the order that they were requested. This is wrong,
since that is now a concept of SingleThreadedProxyResolver and NOT ProxyService.
I need to do a general cleanup of ProxyService's unit-tests, and I will address
this issue then.
http://codereview.chromium.org/149525/diff/89/1066
File net/proxy/single_threaded_proxy_resolver.cc (right):
http://codereview.chromium.org/149525/diff/89/1066#newcode133
Line 133: // Completion will be notifed through |callback|, unless the caller
cancels
On 2009/07/14 22:10:04, willchan wrote:
> s/notifed/notified/
Done.
http://codereview.chromium.org/149525/diff/89/1066#newcode148
Line 148: void SingleThreadedProxyResolver::InitThread() {
On 2009/07/14 22:10:04, willchan wrote:
> Perhaps this should be named EnsureThreadInit() or something, since it doesn't
> necessarily initialize the thread each time.
Done -- EnsureThreadStarted()
http://codereview.chromium.org/149525/diff/89/1066#newcode155
Line 155: // TODO(eroman): Move this higher up to match delcaration order.
On 2009/07/14 22:10:04, willchan wrote:
> s/delcaration/declaration/
Done -- fixed both occurrences of this typo.
http://codereview.chromium.org/149525/diff/89/1066#newcode184
Line 184: DCHECK(pending_jobs_.front().get() == expected_job);
On 2009/07/14 22:10:04, willchan wrote:
> DCHECK_EQ
Done.
http://codereview.chromium.org/149525/diff/89/1066#newcode191
Line 191: // TODO(eroman): Move this higher up to match delcaration order.
On 2009/07/14 22:10:04, willchan wrote:
> s/delcaration/declaration/
Done.
http://codereview.chromium.org/149525/diff/89/1066#newcode195
Line 195: // (1) Not started (just sitting in the queue).
On 2009/07/14 22:10:04, willchan wrote:
> It looks like CancelRequest() does not actually prevent the job from getting
> executed in case (1). There's also a case (1.5) where the job's task is in
the
> MessageLoop, but it's not executed yet.
>
> Are there any plans to address this case? Perhaps it's too minor to worry
> about? If so, can we document that?
Both these cases should currently be handled.
Case (1) is handled by the call to:
pending_jobs_.erase(it);
By removing the job from the queue, it will never reach the front of the queue
and therefore never be started on the resolver.
This case (1) is covered by the unit-test:
SingleThreadedProxyResolverTest.CancelRequest
As for "case (1.5)", this is handled by the code for "case (2)" -- once it
reaches the front of the message loop, case (1.5) becomes case (2).
During Thread's destructor, the outstanding tasks are first completed, so we
don't have an extra case to consider here.

http://codereview.chromium.org/149525/diff/89/1066
File net/proxy/single_threaded_proxy_resolver.cc (right):
http://codereview.chromium.org/149525/diff/89/1066#newcode216
Line 216: if (it != pending_jobs_.end()) {
On 2009/07/17 17:00:04, willchan wrote:
> Is it legal to cancel a request that doesn't exist? If not, should there be a
> DCHECK?
Done.
http://codereview.chromium.org/149525/diff/160/1106
File net/proxy/proxy_resolver_mac.cc (right):
http://codereview.chromium.org/149525/diff/160/1106#newcode247
Line 247: CompletionCallback* callback,
On 2009/07/17 17:00:04, willchan wrote:
> Comment out these unused params?
Done.
http://codereview.chromium.org/149525/diff/160/1104
File net/proxy/proxy_resolver_v8.h (right):
http://codereview.chromium.org/149525/diff/160/1104#newcode54
Line 54: virtual void SetPacScriptByDataInternal(const std::string& bytes);
On 2009/07/17 17:00:04, willchan wrote:
> This should be private.
I struggled with this decision -- by splitting it makes it harder to read, since
the "ProxyResolver implementation" block is split between non-contiguous
sections. Which seems more error prone in missing updates if the interface
changes (especially since SetPacScriptByDataInternal() is not a required =0).
How would you feel about me keeping it in this position by adding a middle
private section?
Something like:
public:
...
// ProxyResolver implementation:
virtual int GetProxyForURL(const GURL& url,
ProxyInfo* results,
CompletionCallback* callback,
RequestHandle* request);
virtual void CancelRequest(RequestHandle request);
virtual void SetPacScriptByUrlInternal(const GURL& pac_url);
private:
virtual void SetPacScriptByDataInternal(const std::string& bytes);
public:
...
http://codereview.chromium.org/149525/diff/160/1098
File net/proxy/proxy_service.cc (right):
http://codereview.chromium.org/149525/diff/160/1098#newcode96
Line 96: virtual void RunWithParams(const Tuple1<int>& params) {
On 2009/07/17 17:00:04, willchan wrote:
> My personal preference is to have a CompletionCallbackImpl member instead of
> inheriting from CompletionCallback. It simplifies the inheritance hierarchy
> (it'll look weird if you have to inherit off something else later on), and
using
> an OnIOComplete(int result) method is more readable than RunWithParams(const
> Tuple1<int>& params).
Done. Agreed.
http://codereview.chromium.org/149525/diff/160/1098#newcode369
Line 369: // of one of the requests.
On 2009/07/17 17:00:04, willchan wrote:
> I haven't wrapped my head around this completely. Can you explain how it's
safe
> to continue processing the PendingRequests set after |this| is deleted? It
> looks like PacRequest::QueryComplete() and PacRequest::QueryDidComplete() both
> call back to service_, which would be deleted memory then, right?
If |this| is deleted, then all of the PacRequest instances are Cancel()-ed.
Since PacRequest is reference counted, |pending_copy| holds a reference to these
same request objects, which will survive the deletion of |this|.
When iterating through them now, the check for |!req->was_cancelled()| prevents
processing requests from the deleted ProxyService.
(in other words, the deletion case generalizes to cancellation case).
http://codereview.chromium.org/149525/diff/160/1100
File net/proxy/single_threaded_proxy_resolver.cc (right):
http://codereview.chromium.org/149525/diff/160/1100#newcode158
Line 158: thread()->message_loop()->PostTask(FROM_HERE,
On 2009/07/17 17:00:04, willchan wrote:
> your formatting is off here, the arguments should be lined up. you should
> probably move FROM_HERE to the next line as well.
Done.
http://codereview.chromium.org/149525/diff/160/1101
File net/proxy/single_threaded_proxy_resolver.h (right):
http://codereview.chromium.org/149525/diff/160/1101#newcode12
Line 12: #include "base/thread.h"
On 2009/07/17 17:00:04, willchan wrote:
> Can base::Thread be forward declared instead?
Done.
http://codereview.chromium.org/149525/diff/160/1101#newcode49
Line 49: typedef std::deque<scoped_refptr<Job> > PendingJobsQueue;
On 2009/07/17 17:00:04, willchan wrote:
> You should #include "base/ref_counted.h" for scoped_refptr.
Done.
http://codereview.chromium.org/149525/diff/160/1108
File net/proxy/single_threaded_proxy_resolver_unittest.cc (right):
http://codereview.chromium.org/149525/diff/160/1108#newcode14
Line 14:
On 2009/07/17 17:00:04, willchan wrote:
> Can this go into an anonymous namespace?
Done.
http://codereview.chromium.org/149525/diff/160/1108#newcode295
Line 295: mock->SetResolveLatency(100);
On 2009/07/17 17:00:04, willchan wrote:
> It seems like you could remove the need for the resolve latency by posting a
> task to another thread (perhaps the WorkerPool) that calls mock->Unblock()
after
> you do resolve.reset(). Would save some time and guarantee coverage for the
> case you are testing.
It seems to me that moving the unblocking to another thread still suffers from
timing races. The timing race now, is that the posted task must not run
immediately (a guarantee not made by PostTask).
I believe your suggestion is to do:
1: other_thread->PostTask(<call mock->UnBlock()>)
2: resolver.reset();
However, it may still be the task scheduled in line (1) completes before line
(2) is run.

LGTM
http://codereview.chromium.org/149525/diff/160/1104
File net/proxy/proxy_resolver_v8.h (right):
http://codereview.chromium.org/149525/diff/160/1104#newcode54
Line 54: virtual void SetPacScriptByDataInternal(const std::string& bytes);
On 2009/07/20 21:22:40, eroman wrote:
> On 2009/07/17 17:00:04, willchan wrote:
> > This should be private.
>
> I struggled with this decision -- by splitting it makes it harder to read,
since
> the "ProxyResolver implementation" block is split between non-contiguous
> sections. Which seems more error prone in missing updates if the interface
> changes (especially since SetPacScriptByDataInternal() is not a required =0).
>
> How would you feel about me keeping it in this position by adding a middle
> private section?
>
> Something like:
>
> public:
>
> ...
>
> // ProxyResolver implementation:
> virtual int GetProxyForURL(const GURL& url,
> ProxyInfo* results,
> CompletionCallback* callback,
> RequestHandle* request);
> virtual void CancelRequest(RequestHandle request);
> virtual void SetPacScriptByUrlInternal(const GURL& pac_url);
> private:
> virtual void SetPacScriptByDataInternal(const std::string& bytes);
>
> public:
>
> ...
This breaks the ordering of access controls sections as specified by the style
guide.
My preference is still just to keep it in the private section. The way I think
of it, it's clear enough that it's part of the interface since it's virtual. If
you're worried about missing updates, then I think pure virtual is the way to
go.
http://codereview.chromium.org/149525/diff/160/1098
File net/proxy/proxy_service.cc (right):
http://codereview.chromium.org/149525/diff/160/1098#newcode369
Line 369: // of one of the requests.
On 2009/07/20 21:22:40, eroman wrote:
> On 2009/07/17 17:00:04, willchan wrote:
> > I haven't wrapped my head around this completely. Can you explain how it's
> safe
> > to continue processing the PendingRequests set after |this| is deleted? It
> > looks like PacRequest::QueryComplete() and PacRequest::QueryDidComplete()
both
> > call back to service_, which would be deleted memory then, right?
>
> If |this| is deleted, then all of the PacRequest instances are Cancel()-ed.
>
> Since PacRequest is reference counted, |pending_copy| holds a reference to
these
> same request objects, which will survive the deletion of |this|.
>
> When iterating through them now, the check for |!req->was_cancelled()|
prevents
> processing requests from the deleted ProxyService.
>
> (in other words, the deletion case generalizes to cancellation case).
Ah, that makes sense now. Can you add the "If |this| is deleted, then all of
the PacRequest instances are Cancel()-ed." to the above comment?
http://codereview.chromium.org/149525/diff/160/1108
File net/proxy/single_threaded_proxy_resolver_unittest.cc (right):
http://codereview.chromium.org/149525/diff/160/1108#newcode295
Line 295: mock->SetResolveLatency(100);
On 2009/07/20 21:22:40, eroman wrote:
> On 2009/07/17 17:00:04, willchan wrote:
> > It seems like you could remove the need for the resolve latency by posting a
> > task to another thread (perhaps the WorkerPool) that calls mock->Unblock()
> after
> > you do resolve.reset(). Would save some time and guarantee coverage for the
> > case you are testing.
>
> It seems to me that moving the unblocking to another thread still suffers from
> timing races. The timing race now, is that the posted task must not run
> immediately (a guarantee not made by PostTask).
>
> I believe your suggestion is to do:
>
> 1: other_thread->PostTask(<call mock->UnBlock()>)
> 2: resolver.reset();
>
> However, it may still be the task scheduled in line (1) completes before line
> (2) is run.
Ah yes, you're right. Hm, it sounds like what you want is at the end of the
destructor body (before the compiler generated code for destructing the member
variables gets invoked), to call mock->Unblock(). I guess this is fine then.
http://codereview.chromium.org/149525/diff/2001/1126
File net/proxy/single_threaded_proxy_resolver.cc (right):
http://codereview.chromium.org/149525/diff/2001/1126#newcode63
Line 63: coordinator_->thread()->message_loop()->PostTask(FROM_HERE,
The indentation of the arguments is off. FROM_HERE probably should move to the
next line.
http://codereview.chromium.org/149525/diff/2001/1126#newcode84
Line 84: DCHECK(rv != ERR_IO_PENDING);
DCHECK_NE
http://codereview.chromium.org/149525/diff/2001/1126#newcode171
Line 171: // It is here to make diffs against proxy_service.cc cleaner.
You can do this now if you want since I've already read through it.