Get rid of synchronous ResolveProxy API.

Get rid of synchronous ResolveProxy API.
Also remove the ProxyInfo::Apply method.

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

[eroman, 2009-01-30 10:56:01]

This change depends on (http://codereview.chromium.org/14142) which gets rid of
the (non-unit-test) consumer of the sync API.

The motivation in getting rid of the synchronous API is to make room for hooking
up ProxyScriptFetcher into ProxyService (so you can download PAC scripts from
the IO thread).

[wtc, 2009-01-30 18:21:34]

LGTM.  Please review my suggested changes and questions
below.

Why do you get rid of the synchronous mode of ResolveProxy,
and then add a SyncProxyServiceHelper class (in
http://codereview.chromium.org/19502)?

http://codereview.chromium.org/19502/diff/11/13
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/19502/diff/11/13#newcode188
Line 188: origin_loop_( MessageLoop::current()) {
Should we DCHECK(callback_)?

http://codereview.chromium.org/19502/diff/11/13#newcode218
Line 218: // Invokes the callback on the origin thread to indicate that the
completion
I think the new comment says the same thing twice: "Invokes
the callback .... to indicate that the completion callback
should be run".

How about "This runs on the origin thread to indicate that the completion
callback should be run." or
"Runs the completion callback on the origin thread."?

http://codereview.chromium.org/19502/diff/11/13#newcode224
Line 224: if (!was_cancelled) {
Comment: it took me a while to understand this change.  The
original code tests service_, results_, and callback_.  The
new code only tests callback_ (was_cancelled).  These are
equivalent because Cancel() sets service_, callback_, and
results_ to NULL at the same time.

http://codereview.chromium.org/19502/diff/11/13#newcode293
Line 293: DCHECK(callback);
Why is this DCHECK removed?

http://codereview.chromium.org/19502/diff/11/13#newcode354
Line 354: // Create PAC thread for asynchronous mode.
I guess we can remove this comment now because asynchronous
mode is the only mode supported.

http://codereview.chromium.org/19502/diff/11/14
File net/proxy/proxy_service.h (left):

http://codereview.chromium.org/19502/diff/11/14#oldcode250
Line 250: void Apply(HINTERNET request_handle);
You may want to mention that you also get rid of the
ProxyInfo::Apply method.

Thanks for cleaning this up.  I missed this when I removed
the WinHTTP code.

http://codereview.chromium.org/19502/diff/11/14
File net/proxy/proxy_service.h (right):

http://codereview.chromium.org/19502/diff/11/14#newcode92
Line 92: // Returns OK if proxy information could be provided synchronously. 
Else,
Note: I verified that this block comment does not mention
you can use ResolveProxy in synchronous mode by passing a
NULL callback.  So we don't need to update the comment.

http://codereview.chromium.org/19502/diff/11/12
File net/proxy/proxy_service_unittest.cc (right):

http://codereview.chromium.org/19502/diff/11/12#newcode67
Line 67: LazyInitThreadAndHelper();
Why don't you initialize the thread and helper in the
constructor?  Why do you initialize them lazily?

http://codereview.chromium.org/19502/diff/11/12#newcode90
Line 90: scoped_refptr<net::SyncProxyServiceHelper> sync_proxy_service_;
SyncProxyService is a very thin wrapper around SyncProxServiceHelper.  Why do
you need two classes?

http://codereview.chromium.org/19502/diff/11/12#newcode180
Line 180: int rv = service.ResolveProxy(url, &info);
Why does this ResolveProxy call return ERR_FAILED instead of
OK now?

http://codereview.chromium.org/19502/diff/11/12#newcode345
Line 345: rv = service.ResolveProxy(url, &info2);
Same question: why does this ResolveProxy call return ERR_FAILED instead of OK
now?

[eroman, 2009-01-30 22:31:47]

> Why do you get rid of the synchronous mode of ResolveProxy,
> and then add a SyncProxyServiceHelper class (in
> http://codereview.chromium.org/19502)?

SyncProxyServiceHelper bridges between two threads to create the synchronous
semantics, whereas the ProxyService sync mode did all the work on the requesting
thread.

The way SyncProxyServiceHelper is used in test_shell, is to forward proxy
resolve requests arriving on the plugin thread (same as the renderer tread), to
the IO thread where the application's global proxy service lives. The request is
now carried out in async mode, using the application's proxy settings.

By contrast, to use the synchronous mode API, you need to create a separate
instance of ProxyService (since ProxyService is not thread safe). 

This was the case before -- it would create a winhttp-based proxy service, and
use it synchronously on the plugin thread.

Behaviorally this was a problem, since it could be using different settings than
the main application.

Moreover, once ProxyService is extended to fetch PAC scripts
(ProxyScriptFetcher), it becomes more effort support the synchronous mode
operation, for no real gain.

http://codereview.chromium.org/19502/diff/11/13
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/19502/diff/11/13#newcode188
Line 188: origin_loop_( MessageLoop::current()) {
On 2009/01/30 18:21:35, wtc wrote:
> Should we DCHECK(callback_)?

Done.

http://codereview.chromium.org/19502/diff/11/13#newcode218
Line 218: // Invokes the callback on the origin thread to indicate that the
completion
On 2009/01/30 18:21:35, wtc wrote:
> I think the new comment says the same thing twice: "Invokes
> the callback .... to indicate that the completion callback
> should be run".
> 
> How about "This runs on the origin thread to indicate that the completion
> callback should be run." or
> "Runs the completion callback on the origin thread."?

Done.

http://codereview.chromium.org/19502/diff/11/13#newcode224
Line 224: if (!was_cancelled) {
On 2009/01/30 18:21:35, wtc wrote:
> Comment: it took me a while to understand this change.  The
> original code tests service_, results_, and callback_.  The
> new code only tests callback_ (was_cancelled).  These are
> equivalent because Cancel() sets service_, callback_, and
> results_ to NULL at the same time.

Added a comment to explain that.

http://codereview.chromium.org/19502/diff/11/13#newcode293
Line 293: DCHECK(callback);
On 2009/01/30 18:21:35, wtc wrote:
> Why is this DCHECK removed?

This is an _added_ DCHECK.

http://codereview.chromium.org/19502/diff/11/13#newcode354
Line 354: // Create PAC thread for asynchronous mode.
On 2009/01/30 18:21:35, wtc wrote:
> I guess we can remove this comment now because asynchronous
> mode is the only mode supported.

Done.

http://codereview.chromium.org/19502/diff/11/14
File net/proxy/proxy_service.h (left):

http://codereview.chromium.org/19502/diff/11/14#oldcode250
Line 250: void Apply(HINTERNET request_handle);
On 2009/01/30 18:21:35, wtc wrote:
> You may want to mention that you also get rid of the
> ProxyInfo::Apply method.
> 
> Thanks for cleaning this up.  I missed this when I removed
> the WinHTTP code.

Done.

http://codereview.chromium.org/19502/diff/11/14
File net/proxy/proxy_service.h (right):

http://codereview.chromium.org/19502/diff/11/14#newcode92
Line 92: // Returns OK if proxy information could be provided synchronously. 
Else,
On 2009/01/30 18:21:35, wtc wrote:
> Note: I verified that this block comment does not mention
> you can use ResolveProxy in synchronous mode by passing a
> NULL callback.  So we don't need to update the comment.

Updated this comment block to explain error code in terms of !ERR_IO_PENDING.

http://codereview.chromium.org/19502/diff/11/12
File net/proxy/proxy_service_unittest.cc (right):

http://codereview.chromium.org/19502/diff/11/12#newcode67
Line 67: LazyInitThreadAndHelper();
On 2009/01/30 18:21:35, wtc wrote:
> Why don't you initialize the thread and helper in the
> constructor?  Why do you initialize them lazily?

Done.

D'oh! Forgot to remove that.

http://codereview.chromium.org/19502/diff/11/12#newcode90
Line 90: scoped_refptr<net::SyncProxyServiceHelper> sync_proxy_service_;
On 2009/01/30 18:21:35, wtc wrote:
> SyncProxyService is a very thin wrapper around SyncProxServiceHelper.  Why do
> you need two classes?

Unit tests is the only place that needs the helper thread. See message comment.

http://codereview.chromium.org/19502/diff/11/12#newcode180
Line 180: int rv = service.ResolveProxy(url, &info);
On 2009/01/30 18:21:35, wtc wrote:
> Why does this ResolveProxy call return ERR_FAILED instead of
> OK now?

There was a mismatch between error codes used in synchronous mode operation and
async operation.

In sync operation, OK would always be returned (regardless of if there was a
failure).
Whereas in async operation, ERR_IO_PENDING is returned, followed by the (real)
error code in the completion callback.

The async behavior makes more sense to me (and is in fact what we have been
using all along), so I simply updated the tests to reflect that behavior, now
that they use the async mode codepath.


You can see this in the old code here:

if (callback) {
 if (pac_request)
   *pac_request = req;
  return ERR_IO_PENDING;  // Wait for callback.
}	
return OK;  <--- Always return OK for sync mode.

I wouldn't necessarily say this is a bug, since the documentation calls it out:

"Returns OK if proxy information could be provided synchronously.  Else
ERR_IO_PENDING..."

But the difference strikes me as undesirable.

http://codereview.chromium.org/19502/diff/11/12#newcode345
Line 345: rv = service.ResolveProxy(url, &info2);
On 2009/01/30 18:21:35, wtc wrote:
> Same question: why does this ResolveProxy call return ERR_FAILED instead of OK
> now?

Same explanation as above.

Failure is expected because of "resolver->fail_get_proxy_for_url = true" in the
line above -- sync operation would mask the failure with OK.

[wtc, 2009-01-30 23:06:06]

LGTM!  Thanks for explaining the background to me
as I didn't look at the CL this CL depends on.

I suggested a minor comment change for ResolveProxy below.

http://codereview.chromium.org/19502/diff/11/13
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/19502/diff/11/13#newcode224
Line 224: if (!was_cancelled) {
On 2009/01/30 22:31:47, eroman wrote:
>
> Added a comment to explain that.

Actually I didn't mean to ask you to add a comment.
But it doesn't hurt.

http://codereview.chromium.org/19502/diff/11/13#newcode293
Line 293: DCHECK(callback);
On 2009/01/30 22:31:47, eroman wrote:
> On 2009/01/30 18:21:35, wtc wrote:
> > Why is this DCHECK removed?
> 
> This is an _added_ DCHECK.

D'oh!  I was stupid.

http://codereview.chromium.org/19502/diff/203/206
File net/proxy/proxy_service.h (right):

http://codereview.chromium.org/19502/diff/203/206#newcode93
Line 93: // Returns ERR_IO_PENDING if the proxy information could not be
provided
Ah, so this block comment still needs to be updated.
I missed it this morning.

Since ResolveProxy doesn't have a sync mode any more,
if seems that we should say this instead:
  Returns ERR_IO_PENDING if we have initiated
  asynchronous proxy resolution successfully.