Add support to ProxyService for downloading a PAC script on behalf of the Pro...

Add support to ProxyService for downloading a PAC script on behalf of the ProxyResolver. A ProxyResolver can select this new behavior by subclassing ProxyResolver with |does_fetch = false|. A consequence of this change is that proxy resolve requests are maintained in a queue by ProxyService (rather than implicitly in a queue on the PAC thread's message loop). This simplifies cancellation.This solves issue 7461, and is work-in-progress towards {2764, 74}BUG=7461
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=10197

[eroman, 2009-02-13 00:51:05]

Still need to add some new unit tests, but figured I would start this ball
rolling.

[eroman, 2009-02-13 00:59:23]

Oh yeah, and in case it is confusing, ProxyResolverWithoutFetch is intended to
be used as follows to define ProxyResolverV8:

class ProxyResolverV8 : public ProxyResolverWithoutFetch {
 public:
  // ProxyResolver implementation:
  virtual int GetProxyForURL(
      const GURL& query_url,
      const GURL& /*pac_url*/,
      ProxyInfo* results);

  // ProxyScriptFetcher::Delegate implementation:
  virtual void OnScriptFetchCompletion(
      int result, const std::string& bytes);
};

void ProxyResolverV8::OnScriptFetchCompletion(
    int result, const std::string& bytes) {
  /*
   * Reset V8 context; load the script
   * |bytes| into it.
   */
}

int ProxyResolverV8::GetProxyForURL(
    const GURL& query_url,
    const GURL& /*pac_url*/,
    ProxyInfo* results) {
  /*
   * Evaluate "FindProxyForURL" in the current V8 context.
   */
}

[darin (slow to review), 2009-02-13 01:00:40]

it isn't obvious why the Delegate interface is nicer than CompletionCallback. 
couldn't you still use CompletionCallback with a getter on ProxyScriptFetcher to
get at the downloaded data?

CompletionCallback has many benefits, which is why i'd like it to be used more,
not less :)

[eroman, 2009-02-13 02:01:26]

> it isn't obvious why the Delegate interface is nicer than
> CompletionCallback.  couldn't you still use CompletionCallback with a
> getter on ProxyScriptFetcher to get at the downloaded data?
>
> CompletionCallback has many benefits, which is why i'd like it to be
> used more, not less :)

I am not opposed to changing back, but I would like to understand better what
the merits are.

My thinking for moving to delegate was that since ProxyScriptFetcher only
supports a single request at time, and the |callback| is always going to be the
same, the power of completion callback is lost. And it becomes a glorified
delegate plus the tedious boiler plate:

  // Cruft in the header:
  net::CompletionCallbackImpl<Class> callback_;

  // Temporary to receive results of outstanding request.
  Results results_; 

...

// Cruft in the initializer list:

ALLOW_THIS_IN_INITIALIZER_LIST(	
   callback_(this, &Class::OnDone)),

...

// Callback
void Class::OnDone(int result) {
 
  // Ok, now I do what I wanted all along :)
  Foo(result, results_);
}


The other thing I liked with the delegate approach, is how it annotates the
shared behaviors. 
Both ProxyService and ProxyResolverWithoutFetch are responders to fetch
completions, and this shows up in their definitions:

class ProxyService : public ProxyScriptFetcher::Delegate {
  ...
}

class ProxyResolverWithoutFetch : public ProxyResolver,
                                  public ProxyScriptFetcher::Delegate {
  ...
}


Anyhow, these are fairly minor, but open to suggestions.

[darin (slow to review), 2009-02-13 03:00:10]

Yeah, it's true that there is a lot of boilerplate associated with
CompletionCallback.  I would like to find a way to reduce that.

The advantages of CompletionCallback:

1-  Consumer can name the handler function however they please.

2-  CompletionCallback taking only a single 'int' parameter forces the
convention that methods may complete either synchronously or asynchronously. 
the only difference with an asynchronous completion is how the 'rv' gets
delivered.

3-  Consistency throughout the network library.  I find points #1 and #2
compelling enough to make me want to change the rest of the network library over
to CompletionCallback :)

The point about annotating behaviors is interesting, and that does seem like it
could be helpful in some cases.  I had always viewed that from a different
angle....  The thing that has always bugged me about delegate usage is that
often a class is just implementing a delegate as part of its private
implementation.  Google style disallows private inheritance, but here is a case
I think where private inheritance would be nice.  CC allows me to hide the
asynchronous implementation details of my class.  I like to see the most minimal
public section of a class :)

[eroman, 2009-02-13 03:48:55]

ok, thanks for the explanation.

Done.
Removed the delegate in favor of CompletionCallback.

[eroman, 2009-02-14 01:14:39]

Ok, updated with the unit tests... enjoy ;-)

[darin (slow to review), 2009-02-17 20:38:03]

http://codereview.chromium.org/21328/diff/20/1020
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/21328/diff/20/1020#newcode46
Line 46: // HttpUtil::SpecForRequest. Should probably live in net_util.h
+1 for moving this to net_util.h :)

http://codereview.chromium.org/21328/diff/20/1020#newcode207
Line 207: // TODO(eroman): Rename this to ProxyRequest since it isn't specific
to PAC.
hmm... but we only need this object for PAC queries.  the proxy resolver only
handles those, right?

oh, i see...

http://codereview.chromium.org/21328/diff/20/1020#newcode286
Line 286: service_->RemoveFrontOfRequestQueue(this);
this call cannot be combined with DidCompletePacRequest?

http://codereview.chromium.org/21328/diff/20/1020#newcode372
Line 372: pending_requests_.push_back(req);
maybe pending_requests_ should hold  elements of type scoped_refptr<PacRequest>?
 then you would be able to eliminate a lot of the manual AddRef/Release calls.

http://codereview.chromium.org/21328/diff/20/1020#newcode382
Line 382: void ProxyService::UpdateConfigIfOld() {
nit: it might be nice if this function were defined nearby UpdateConfig.

http://codereview.chromium.org/21328/diff/20/1020#newcode488
Line 488: UpdateConfigIfOld();
it might be worth adding a bool parameter to ProcessRequestQueue so that you can
skip this code when called directly from ResolveProxy.

http://codereview.chromium.org/21328/diff/20/1020#newcode489
Line 489: int rv = TryToCompleteSynchronously(req->url_, req->results_);
i'm not sure that it is correct to call TryToCompleteSynchronously twice on the
same results_ object.  note: there is some state managed there (see
config_was_tried_).

maybe instead of trying to update the configuration here, we should instead let
each PacRequest hold a reference to its configuration and ProxyResolver.  if the
configuration ever changes such that we need to change the ProxyResolver, then
we could instead just have the ProxyService release its ProxyResolver and
allocate a new one.  the existing, pending, PacRequests would just continue
using the old one.  WDYT?  that might be simpler.

http://codereview.chromium.org/21328/diff/20/1020#newcode505
Line 505: pac_url, &in_progress_fetch_bytes_, &proxy_script_fetcher_callback_);
btw, if it makes things nicer (avoids a member variable on this class), you
could also make the in_progress_fetch_bytes_ be a member of
proxy_script_fetcher_.  (i remember when we discussed Delegate vs
CompletionCallback that the issue of this result data came up.)

http://codereview.chromium.org/21328/diff/20/1020#newcode532
Line 532: static_cast<ProxyResolverWithoutFetch*>(resolver_.get()),
maybe it would be nice to just add the SetPacScript method to ProxyResolver, and
document that it doesn't make sense to call that method if the ProxyResolver
says "does_fetch".  then, we have one less interface, and no implied casting
from ProxyResolver to ProxyResolverWithoutFetch when !does_fetch, which is a bit
disconnected (ideally, RTTI would be accomplished by asking the resolver if it
"is a"  ProxyResolverWithoutFetch).

http://codereview.chromium.org/21328/diff/20/1021
File net/proxy/proxy_service.h (right):

http://codereview.chromium.org/21328/diff/20/1021#newcode181
Line 181: // Otherwise it fill |result| with the proxy information for |url|.
nit: s/fill/fills/

http://codereview.chromium.org/21328/diff/20/1021#newcode190
Line 190: void ProcessRequestQueue();
nit:  it might be nice to make these names match.  either ProcessRequestQueue ->
ProcessPendingRequests or pending_requests_ -> request_queue_.

http://codereview.chromium.org/21328/diff/20/1021#newcode194
Line 194: void RemoveFrontOfRequestQueue(PacRequest* req);
nit: req -> expected_request

http://codereview.chromium.org/21328/diff/20/1021#newcode242
Line 242: // If no PAC script has been fetched yet, wil be
ProxyConfig::INVALID_ID.
nit: wil -> will

http://codereview.chromium.org/21328/diff/20/1021#newcode247
Line 247: ProxyConfig::ID in_progress_fetch_id_;
nit: maybe ends with "_config_id_" is nice here too for symmetry? 
in_progress_fetch_config_id_

http://codereview.chromium.org/21328/diff/20/1021#newcode391
Line 391: virtual void OnScriptFetchCompletion(int result,
nit: hmm... might also be nice to just call this SetPacScript.  what is the use
case for result != OK?  is it to tell the resolver to drop its configuration? 
maybe that should just be another method?  or one could call
SetPacScript(std::string()) to cause it to clear its PAC script.

http://codereview.chromium.org/21328/diff/20/1022
File net/proxy/proxy_service_unittest.cc (right):

http://codereview.chromium.org/21328/diff/20/1022#newcode341
Line 341: void
RunnableMethodTraits<MockProxyScriptFetcher>::RetainCallee(MockProxyScriptFetcher*
remover) {}
nit: 80 chars

[eroman, 2009-02-18 08:02:59]

Thanks for the review!
feel free to skim/skip some of my longer responses :-)

http://codereview.chromium.org/21328/diff/20/1020
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/21328/diff/20/1020#newcode46
Line 46: // HttpUtil::SpecForRequest. Should probably live in net_util.h
On 2009/02/17 20:38:04, darin wrote:
> +1 for moving this to net_util.h :)

I will do this as a standalone change.
(I'm trying to avoid doing refactorings at the same time.)

http://codereview.chromium.org/21328/diff/20/1020#newcode207
Line 207: // TODO(eroman): Rename this to ProxyRequest since it isn't specific
to PAC.
On 2009/02/17 20:38:04, darin wrote:
> hmm... but we only need this object for PAC queries.  the proxy resolver only
> handles those, right?
> 
> oh, i see... 

I guess it is splitting ends, but technically what might happen is we insert a
"PacRequest" into the queue, and then when we dequeue it we complete it
_without_ going through the resolver (so it wasn't so much a PAC request as a
generic "proxy resolve request").

I describe these scenarios in a later response.

That said, I am fine with keeping this named PacRequest and have removed the
TODO.

http://codereview.chromium.org/21328/diff/20/1020#newcode286
Line 286: service_->RemoveFrontOfRequestQueue(this);
On 2009/02/17 20:38:04, darin wrote:
> this call cannot be combined with DidCompletePacRequest?

RemoveFrontOfRequestQueue() is its own function because it is used by two other
callers. The function  pops the top of the queue after completion and kick off
the next request.

That said, I _could_ call RemoveFrontOfRequestQueue() from inside
DidCompletePacRequest().
However IMO that makes the code a bit trickier to understand. Since what would
happen is the next request gets started (and maybe completes synchronously)
_before_ we invoke the callback of the already-finished request.

The functional behavior ends up being the same -- callbacks are invoked in the
same order as the request was added to the queue. However it is less obvious
then if RemoveFrontOfRequestQueue() were simply called after the callback has
run.

http://codereview.chromium.org/21328/diff/20/1020#newcode372
Line 372: pending_requests_.push_back(req);
On 2009/02/17 20:38:04, darin wrote:
> maybe pending_requests_ should hold  elements of type
scoped_refptr<PacRequest>?
>  then you would be able to eliminate a lot of the manual AddRef/Release calls.

Done.

http://codereview.chromium.org/21328/diff/20/1020#newcode382
Line 382: void ProxyService::UpdateConfigIfOld() {
On 2009/02/17 20:38:04, darin wrote:
> nit: it might be nice if this function were defined nearby UpdateConfig.

Done.

http://codereview.chromium.org/21328/diff/20/1020#newcode488
Line 488: UpdateConfigIfOld();
On 2009/02/17 20:38:04, darin wrote:
> it might be worth adding a bool parameter to ProcessRequestQueue so that you
can
> skip this code when called directly from ResolveProxy.

Done.

But instead of a bool, i used a PacRequest*, since it may be the queue was
non-empty when we added the first request.

http://codereview.chromium.org/21328/diff/20/1020#newcode489
Line 489: int rv = TryToCompleteSynchronously(req->url_, req->results_);
On 2009/02/17 20:38:04, darin wrote:
> i'm not sure that it is correct to call TryToCompleteSynchronously twice on
the
> same results_ object.  note: there is some state managed there (see
> config_was_tried_).
> 
> maybe instead of trying to update the configuration here, we should instead
let
> each PacRequest hold a reference to its configuration and ProxyResolver.  if
the
> configuration ever changes such that we need to change the ProxyResolver, then
> we could instead just have the ProxyService release its ProxyResolver and
> allocate a new one.  the existing, pending, PacRequests would just continue
> using the old one.  WDYT?  that might be simpler.

I believe this is still valid.

The only dependencies for TryToCompleteSynchronously() are:
  1. |ProxyService::config_|    --- the current proxy coniguration
  2. |ProxyService::config_is_bad_|  --- whether we should give up right away
and not even try the ProxyResolver.

|PacRequest::results_->config_was_tried_| is solely an output, so it is safe to
set it to true multiple times. (We know it only gets set to false once, since at
that time we will complete synchronously).

As far as the bigger picture,  the reason I call TryToCompleteSynchronously()
from within ProcessRequestQueue(), is to cover these two scenarios:

--------------
Scenario 1  (config changed while enqueued):
--------------
  1. ProxyService::ResolveProxy(url="http://www.google.com/")
  2. Lets say our system configuration is having us use PAC, so we can't
complete this synchronously as a DIRECT.
  3. So we add a PacRequest(url) to the pending queue. Lets say there are
already a bunch of items in the queue, so we are forced to wait...
  4. Now the system proxy configuration changes. In the new coniguration,
"www.google.com" is on the proxy bypass list.
  5. Finally our pending PacRequest reaches the front of the queue. Given the
new value of our configuration (assume enough time passed for it to re-poll), we
should complete this synchronously as a DIRECT result, and avoid sending this
request to the ProxyResolver.

Scenario 1 can happen with something like dglazkov's "papercuts" test -- the
user notices lots of "resolving proxy..." status messages, and decides to turn
off autoproxy.

--------------
Scenario 2 (config becomes bad while in the queue):
--------------
  1. ProxyService::ResolveProxy(url)
  2. Same as above, we can't complete this resolve synchronously -- push it into
the queue (assume there is already an item in the queue).
  3. Now the resolve that preceded us completes with a failure -- the
ProxyResolver returned some sort of error.
  4. Our request reaches the front of the queue. Given that |config_bad_| is now
true, we should just go ahead and complete synchronously, by setting
|result->config_was_tried_ = false|. Any other request sitting in the queue
should similarly do this, such that we respect the "cooloff" period when proxy
resolving fails.

Scenario 2 might happen if you say get a network error while downloading the PAC
script. While the download is in progress, requests pile up; once the failure
occurs, we expect the first queued request to fail, and then every subsequent
one to give the canned DIRECT result (with config_was_tried_ = false).


> configuration ever changes such that we need to change the ProxyResolver, then
> we could instead just have the ProxyService release its ProxyResolver and

I am not quite following this. AFAIK ProxyResolver never changes, and is never
destroyed. The only thing that changes is the proxy configuration (which may
cause the resolver to stop being used).

http://codereview.chromium.org/21328/diff/20/1020#newcode505
Line 505: pac_url, &in_progress_fetch_bytes_, &proxy_script_fetcher_callback_);
On 2009/02/17 20:38:04, darin wrote:
> btw, if it makes things nicer (avoids a member variable on this class), you
> could also make the in_progress_fetch_bytes_ be a member of
> proxy_script_fetcher_.  (i remember when we discussed Delegate vs
> CompletionCallback that the issue of this result data came up.)

Thanks for the suggestion.

I will avoid this for now, since the semantics might be weird
(To avoid those bytes living past the call to SetPacScript(), I would end up
using some "pop" accessor, or adding a "clear" method)

http://codereview.chromium.org/21328/diff/20/1020#newcode532
Line 532: static_cast<ProxyResolverWithoutFetch*>(resolver_.get()),
On 2009/02/17 20:38:04, darin wrote:
> maybe it would be nice to just add the SetPacScript method to ProxyResolver,
and
> document that it doesn't make sense to call that method if the ProxyResolver
> says "does_fetch".  then, we have one less interface, and no implied casting
> from ProxyResolver to ProxyResolverWithoutFetch when !does_fetch, which is a
bit
> disconnected (ideally, RTTI would be accomplished by asking the resolver if it
> "is a"  ProxyResolverWithoutFetch).

Done.

http://codereview.chromium.org/21328/diff/20/1021
File net/proxy/proxy_service.h (right):

http://codereview.chromium.org/21328/diff/20/1021#newcode181
Line 181: // Otherwise it fill |result| with the proxy information for |url|.
On 2009/02/17 20:38:04, darin wrote:
> nit: s/fill/fills/

Done.

http://codereview.chromium.org/21328/diff/20/1021#newcode190
Line 190: void ProcessRequestQueue();
On 2009/02/17 20:38:04, darin wrote:
> nit:  it might be nice to make these names match.  either ProcessRequestQueue
->
> ProcessPendingRequests or pending_requests_ -> request_queue_.

Done.

Renamed ProcessRequestQueue --> ProcessPendingRequests.

http://codereview.chromium.org/21328/diff/20/1021#newcode194
Line 194: void RemoveFrontOfRequestQueue(PacRequest* req);
On 2009/02/17 20:38:04, darin wrote:
> nit: req -> expected_request

Done.

http://codereview.chromium.org/21328/diff/20/1021#newcode242
Line 242: // If no PAC script has been fetched yet, wil be
ProxyConfig::INVALID_ID.
On 2009/02/17 20:38:04, darin wrote:
> nit: wil -> will

Done.

... I really need a spell checker in Visual Studio or something...

http://codereview.chromium.org/21328/diff/20/1021#newcode247
Line 247: ProxyConfig::ID in_progress_fetch_id_;
On 2009/02/17 20:38:04, darin wrote:
> nit: maybe ends with "_config_id_" is nice here too for symmetry? 
> in_progress_fetch_config_id_

Done.

http://codereview.chromium.org/21328/diff/20/1021#newcode391
Line 391: virtual void OnScriptFetchCompletion(int result,
On 2009/02/17 20:38:04, darin wrote:
> nit: hmm... might also be nice to just call this SetPacScript.  what is the
use

Done.

Renamed to SetPacScript().

> case for result != OK?  is it to tell the resolver to drop its configuration? 
> maybe that should just be another method?  or one could call
> SetPacScript(std::string()) to cause it to clear its PAC script.

Yeah, calling SetPacScript() on the error case means it frees up its v8 context.

It also means that any subsequent requests issued to that ProxyResolver will
echo that network error. (mimicking how the "internal fetch" ProxyResolver
works).

One alternative is to add extra state to ProxyService to track whether the last
download succeeded. And if the last download failed, not dispatch to ProxyServer
but fail right away with the error from the download.

I reckon you are right, and this tracking is best done in the ProxyService. I
can already picture the usefulness of knowing when the download last failed, in
order to implement a "retry failed PAC fetching periodically" policy.

http://codereview.chromium.org/21328/diff/20/1022
File net/proxy/proxy_service_unittest.cc (right):

http://codereview.chromium.org/21328/diff/20/1022#newcode341
Line 341: void
RunnableMethodTraits<MockProxyScriptFetcher>::RetainCallee(MockProxyScriptFetcher*
remover) {}
On 2009/02/17 20:38:04, darin wrote:
> nit: 80 chars

Done.

[darin (slow to review), 2009-02-18 20:40:47]

http://codereview.chromium.org/21328/diff/20/1020
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/21328/diff/20/1020#newcode489
Line 489: int rv = TryToCompleteSynchronously(req->url_, req->results_);
Thanks for taking the time to describe all of those scenarios.  They do sound
like useful cases to optimize as you have done.

> I am not quite following this.

I was just imagining a world in which PacRequests would hold a reference to a
configured proxy resolver, and then on configuration change, we would just
allocate a new proxy resolver.  I realize that right now the consumer allocates
the resolver, so such a change would be very involved.  Given the resolution on
the optimizations, however, it doesn't seem worth it :)

[darin (slow to review), 2009-02-18 20:43:21]

LGTM

http://codereview.chromium.org/21328/diff/1025/1027
File net/proxy/proxy_service.h (right):

http://codereview.chromium.org/21328/diff/1025/1027#newcode366
Line 366: ProxyResolver(bool does_fetch) : does_fetch_(does_fetch) {}
i guess there should also be some changes for the proxy resolver subclasses.

[wtc, 2009-02-19 00:20:22]

I already reviewed this CL last week *promptly*, but
neglected to publish my comments.  Sorry!

http://codereview.chromium.org/21328/diff/1011/1012
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/21328/diff/1011/1012#newcode207
Line 207: // TODO(eroman): Rename this to ProxyRequest since it isn't specific
to PAC.
How about ResolveProxyRequest or ProxyResolveRequest?

http://codereview.chromium.org/21328/diff/1011/1012#newcode230
Line 230: void Query() {
The Query() method probably should be renamed Start().

http://codereview.chromium.org/21328/diff/1011/1012#newcode234
Line 234: const GURL& query_url = SanitizeURLForProxyResolver(url_);
Are you sure query_url can be a const GURL&?  Does that
make the GURL object returned by SanitizeURLForProxyResolver
live until the end of this scope?

http://codereview.chromium.org/21328/diff/1011/1012#newcode248
Line 248: result_code));
Nit: should we align result_code with this?

http://codereview.chromium.org/21328/diff/1011/1012#newcode260
Line 260: bool WasCancelled() const { return callback_ == NULL; }
In the net module's convention, this method should be named
was_cancelled because it's essentially an accessor.

http://codereview.chromium.org/21328/diff/1011/1012#newcode363
Line 363: // using a direct proxy configuration, or when the config is bad.
"direct proxy configuration" => "direct connection"?

http://codereview.chromium.org/21328/diff/1011/1012#newcode379
Line 379: return ERR_IO_PENDING;
Nit: you can just return rv.

http://codereview.chromium.org/21328/diff/1011/1012#newcode533
Line 533: result, in_progress_fetch_bytes_));
Nit: result should be aligned with static_cast above.

http://codereview.chromium.org/21328/diff/1011/1013
File net/proxy/proxy_service.h (left):

http://codereview.chromium.org/21328/diff/1011/1013#oldcode153
Line 153: // Checks to see if the proxy configuration changed, and then updates
config_
If UpdateConfig() doesn't update config_, shouldn't we rename
this method?

http://codereview.chromium.org/21328/diff/1011/1013
File net/proxy/proxy_service.h (right):

http://codereview.chromium.org/21328/diff/1011/1013#newcode181
Line 181: // Otherwise it fill |result| with the proxy information for |url|.
"fill" => "fills".

http://codereview.chromium.org/21328/diff/1011/1013#newcode183
Line 183: // (ProxyResolver runs on PAC thread).
Nit: move the period inside the closing ')'.

http://codereview.chromium.org/21328/diff/1011/1013#newcode186
Line 186: // Start the PAC thread if it isn't already running.
Nit: In this comment and the next two comments, use "Starts"
and "Removes" for consistency with the other comments in this
header.

[eroman, 2009-02-19 02:21:05]

http://codereview.chromium.org/21328/diff/1011/1012
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/21328/diff/1011/1012#newcode207
Line 207: // TODO(eroman): Rename this to ProxyRequest since it isn't specific
to PAC.
On 2009/02/19 00:20:22, wtc wrote:
> How about ResolveProxyRequest or ProxyResolveRequest?

I have removed this TODO.

Here is the discussion from Darin's review:
http://codereview.chromium.org/21328/diff/20/1020#newcode207

http://codereview.chromium.org/21328/diff/1011/1012#newcode230
Line 230: void Query() {
On 2009/02/19 00:20:22, wtc wrote:
> The Query() method probably should be renamed Start().

Is it OK if I hold off on this, and save the refactoring for later?
In particular, I am wandering what rename it implies for "QueryComplete":

PacRequest::Query --> PacRequest::Start
PacRequest::DoQuery --> PacRequest::DoStart
PacRequest::QueryComplete  --> PacRequest::???

http://codereview.chromium.org/21328/diff/1011/1012#newcode234
Line 234: const GURL& query_url = SanitizeURLForProxyResolver(url_);
On 2009/02/19 00:20:22, wtc wrote:
> Are you sure query_url can be a const GURL&?  Does that
> make the GURL object returned by SanitizeURLForProxyResolver
> live until the end of this scope?

Done.

Changed to "GURL query_url"

For reference, I did confirm that these semantics are safe. It ends up making a
copy, and marking that copy as const; there are some other places in our code
where we do this, but I agree it is confusing.

http://codereview.chromium.org/21328/diff/1011/1012#newcode248
Line 248: result_code));
On 2009/02/19 00:20:22, wtc wrote:
> Nit: should we align result_code with this?

No longer applicable.

Result_code is gone following Darin's comments.

http://codereview.chromium.org/21328/diff/1011/1012#newcode260
Line 260: bool WasCancelled() const { return callback_ == NULL; }
On 2009/02/19 00:20:22, wtc wrote:
> In the net module's convention, this method should be named
> was_cancelled because it's essentially an accessor.

Done.

http://codereview.chromium.org/21328/diff/1011/1012#newcode363
Line 363: // using a direct proxy configuration, or when the config is bad.
On 2009/02/19 00:20:22, wtc wrote:
> "direct proxy configuration" => "direct connection"?

Done.

http://codereview.chromium.org/21328/diff/1011/1012#newcode379
Line 379: return ERR_IO_PENDING;
On 2009/02/19 00:20:22, wtc wrote:
> Nit: you can just return rv.

Done.

http://codereview.chromium.org/21328/diff/1011/1012#newcode533
Line 533: result, in_progress_fetch_bytes_));
On 2009/02/19 00:20:22, wtc wrote:
> Nit: result should be aligned with static_cast above.

No longer applicable.

the static_cast<> is gone, as is |result|.

http://codereview.chromium.org/21328/diff/1011/1013
File net/proxy/proxy_service.h (left):

http://codereview.chromium.org/21328/diff/1011/1013#oldcode153
Line 153: // Checks to see if the proxy configuration changed, and then updates
config_
On 2009/02/19 00:20:22, wtc wrote:
> If UpdateConfig() doesn't update config_, shouldn't we rename
> this method?

I don't know why I removed that comment -- adding back.

Must have gotten messed up from an earlier draft, where I experimented with an
"OnConfigChanged()" method.

http://codereview.chromium.org/21328/diff/1011/1013
File net/proxy/proxy_service.h (right):

http://codereview.chromium.org/21328/diff/1011/1013#newcode181
Line 181: // Otherwise it fill |result| with the proxy information for |url|.
On 2009/02/19 00:20:22, wtc wrote:
> "fill" => "fills".

Done.

http://codereview.chromium.org/21328/diff/1011/1013#newcode183
Line 183: // (ProxyResolver runs on PAC thread).
On 2009/02/19 00:20:22, wtc wrote:
> Nit: move the period inside the closing ')'.

Done.

http://codereview.chromium.org/21328/diff/1011/1013#newcode186
Line 186: // Start the PAC thread if it isn't already running.
On 2009/02/19 00:20:22, wtc wrote:
> Nit: In this comment and the next two comments, use "Starts"
> and "Removes" for consistency with the other comments in this
> header.

Done.
Sorry, I often forget these need to be passive tense.

[wtc, 2009-02-19 21:18:23]

http://codereview.chromium.org/21328/diff/1011/1012
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/21328/diff/1011/1012#newcode230
Line 230: void Query() {
On 2009/02/19 02:21:05, eroman wrote:
> On 2009/02/19 00:20:22, wtc wrote:
> > The Query() method probably should be renamed Start().
> 
> Is it OK if I hold off on this, and save the refactoring for later?

Yes, definitely.

> In particular, I am wandering what rename it implies for "QueryComplete":
> 
> PacRequest::Query --> PacRequest::Start
> PacReque]st::DoQuery --> PacRequest::DoStart
> PacRequest::QueryComplete  --> PacRequest::???

We have some classes with the OnStartCompleted and
OnReadCompleted methods, so perhaps you can use
OnStartCompleted.

[eroman, 2009-02-20 02:03:58]

Ok, got rid of the network error that was being passed to ProxyResolver, so it
is now:

ProxyResolver::SetPacScript(const std::string& bytes);

(and the ProxyService takes care of returning the fetch error).

[darin (slow to review), 2009-02-20 16:03:31]

LGTM