Better match IE's proxy settings....

Better match IE's proxy settings.

 * When BOTH autodetect and custom PAC script are given, try both.
 * Use successful PAC parsing as the heuristic for determining when a script   is valid (rather than first-request). 
 * Only apply the proxy bypass list when using non-PAC.

The high level explanation on how this works:

http://sites.google.com/a/chromium.org/dev/developers/design-documents/proxy-settings-fallback

BUG= http://crbug.com/18271, http://crbug.com/9985
TEST=unit tests.

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

[eroman, 2009-08-02 10:58:38]

wtc: You may want to review my design document.
willchan: Please review the code.

[willchan no longer on Chromium, 2009-08-03 23:45:47]

I'm still looking at the change to the SingleThreadedProxyResolver.  Here are
some initial comments, just nits really.

http://codereview.chromium.org/160510/diff/54/73
File net/proxy/init_proxy_resolver.cc (right):

http://codereview.chromium.org/160510/diff/54/73#newcode65
Line 65: pac_urls_.push_back(pac_url);
indentation is off here

http://codereview.chromium.org/160510/diff/54/73#newcode78
Line 78: int InitProxyResolver::DoLoop(int last_io_result) {
Is this the common naming practice?  I feel like I usually just see int result
everywhere.  Do you feel like it makes it clearer as to the difference between
rv and result?  I don't feel strongly either way, I just wanted to note it.

http://codereview.chromium.org/160510/diff/54/73#newcode130
Line 130: << " with error " << ErrorToString(result)
Perhaps you should change it from error to return value, since OK isn't an
error.  They'll know it's an error since the string will start with ERR_.

http://codereview.chromium.org/160510/diff/54/73#newcode136
Line 136: next_state_ = STATE_SET_PAC_SCRIPT;
It seems weird to me that you set next_state_ here when TryToFallbackPacUrl() is
already setting next_state_.  It looks like that sets the wrong state though.  I
think it makes more sense for TryToFallbackPacUrl() not to set next_state_, and
rather have the call sites set it to whatever is right.

http://codereview.chromium.org/160510/diff/54/73#newcode141
Line 141: GURL pac_url = current_pac_url();
Should this be const GURL&?

http://codereview.chromium.org/160510/diff/54/74
File net/proxy/init_proxy_resolver.h (right):

http://codereview.chromium.org/160510/diff/54/74#newcode5
Line 5: #ifndef NET_PROXY_INIT_PROXY_RESOLVER_
This is missing the H_, right?

http://codereview.chromium.org/160510/diff/54/74#newcode59
Line 59: void BuildPacUrlsFallbackList(const ProxyConfig& config);
I'm not sure how you feel about this, but when only one thing gets modified, I
prefer to use a return value and make the member function const.  This
suggestion is a little kludgier in this case since NRVO isn't possible if you
try to store directly into |pac_urls_|, so I'd suggest doing something like
BuildPacUrlsFallbackList(config).swap(&pac_urls_)

http://codereview.chromium.org/160510/diff/54/74#newcode98
Line 98: };
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/160510/diff/54/65
File net/proxy/init_proxy_resolver_unittest.cc (right):

http://codereview.chromium.org/160510/diff/54/65#newcode29
Line 29: : url(url),
Your indentation is off here, you need another whitespace

http://codereview.chromium.org/160510/diff/54/55
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/160510/diff/54/55#newcode274
Line 274: bool resolver_is_ready = !IsInitingProxyResolver();
Initing might not be obvious to people, perhaps don't bother abbreviating and
call it IsInitializingProxyResolver()?

http://codereview.chromium.org/160510/diff/54/66
File net/proxy/proxy_service_unittest.cc (right):

http://codereview.chromium.org/160510/diff/54/66#newcode962
Line 962: // Succeed the initialization of the PAC script.
This sounds weird.  Perhaps "Successfully initialize the PAC script."?

http://codereview.chromium.org/160510/diff/54/66#newcode1352
Line 1352: // been sent to ti.
s/ti/it/

http://codereview.chromium.org/160510/diff/54/66#newcode1429
Line 1429: }
Are there tests for the suspension / resumption of requests?

[wtc, 2009-08-03 23:53:16]

eroman is the best person for reviewing this CL.

Eric, could you please review this CL?

[wtc, 2009-08-03 23:54:22]

I'm so sorry.  I thought evan was the author of this CL.
I better get my eyes checked.

[willchan no longer on Chromium, 2009-08-04 00:33:07]

Ok, finished looking at the rest.

http://codereview.chromium.org/160510/diff/1036/1039
File net/proxy/single_threaded_proxy_resolver.cc (right):

http://codereview.chromium.org/160510/diff/1036/1039#newcode34
Line 34: AddRef();  // balanced in RequestComplete
Why is this AddRef() necessary?  Won't RunnableMethod manage the reference
counts correctly?

[eroman, 2009-08-04 01:00:53]

http://codereview.chromium.org/160510/diff/54/73
File net/proxy/init_proxy_resolver.cc (right):

http://codereview.chromium.org/160510/diff/54/73#newcode65
Line 65: pac_urls_.push_back(pac_url);
On 2009/08/03 23:45:48, willchan wrote:
> indentation is off here

Done.

Weird, lint didn't pick that up.

http://codereview.chromium.org/160510/diff/54/73#newcode78
Line 78: int InitProxyResolver::DoLoop(int last_io_result) {
On 2009/08/03 23:45:48, willchan wrote:
> Is this the common naming practice?  I feel like I usually just see int result
> everywhere.  Do you feel like it makes it clearer as to the difference between
> rv and result?  I don't feel strongly either way, I just wanted to note it.

Done.

I copy-pasted the skeleton from for state-machine from another file, and hadn't
given it much thought.

http://codereview.chromium.org/160510/diff/54/73#newcode130
Line 130: << " with error " << ErrorToString(result)
On 2009/08/03 23:45:48, willchan wrote:
> Perhaps you should change it from error to return value, since OK isn't an
> error.  They'll know it's an error since the string will start with ERR_.

Done.

Reworded to "result".

http://codereview.chromium.org/160510/diff/54/73#newcode136
Line 136: next_state_ = STATE_SET_PAC_SCRIPT;
On 2009/08/03 23:45:48, willchan wrote:
> It seems weird to me that you set next_state_ here when TryToFallbackPacUrl()
is
> already setting next_state_.  It looks like that sets the wrong state though. 
I
> think it makes more sense for TryToFallbackPacUrl() not to set next_state_,
and
> rather have the call sites set it to whatever is right.

I don't understand what you mean by "wrong state"...

http://codereview.chromium.org/160510/diff/54/73#newcode141
Line 141: GURL pac_url = current_pac_url();
On 2009/08/03 23:45:48, willchan wrote:
> Should this be const GURL&?

Done.

http://codereview.chromium.org/160510/diff/54/74
File net/proxy/init_proxy_resolver.h (right):

http://codereview.chromium.org/160510/diff/54/74#newcode5
Line 5: #ifndef NET_PROXY_INIT_PROXY_RESOLVER_
On 2009/08/03 23:45:48, willchan wrote:
> This is missing the H_, right?

Done. Oops!

http://codereview.chromium.org/160510/diff/54/74#newcode59
Line 59: void BuildPacUrlsFallbackList(const ProxyConfig& config);
On 2009/08/03 23:45:48, willchan wrote:
> I'm not sure how you feel about this, but when only one thing gets modified, I
> prefer to use a return value and make the member function const.  This
> suggestion is a little kludgier in this case since NRVO isn't possible if you
> try to store directly into |pac_urls_|, so I'd suggest doing something like
> BuildPacUrlsFallbackList(config).swap(&pac_urls_)

Done. Changed to:

// Returns ordered list of PAC urls to try for |config|.
UrlList BuildPacUrlsFallbackList(const ProxyConfig& config) const;

I am not worried about the NRVO optimization thing, since this codepath is run
maybe once during the whole application's lifetime :)

http://codereview.chromium.org/160510/diff/54/74#newcode98
Line 98: };
On 2009/08/03 23:45:48, willchan wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

http://codereview.chromium.org/160510/diff/54/65
File net/proxy/init_proxy_resolver_unittest.cc (right):

http://codereview.chromium.org/160510/diff/54/65#newcode29
Line 29: : url(url),
On 2009/08/03 23:45:48, willchan wrote:
> Your indentation is off here, you need another whitespace

Really? I though I was supposed to indent it by 4 spaces...

http://codereview.chromium.org/160510/diff/54/55
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/160510/diff/54/55#newcode274
Line 274: bool resolver_is_ready = !IsInitingProxyResolver();
On 2009/08/03 23:45:48, willchan wrote:
> Initing might not be obvious to people, perhaps don't bother abbreviating and
> call it IsInitializingProxyResolver()?

Done.
Dunno what I was thinking.

http://codereview.chromium.org/160510/diff/54/66
File net/proxy/proxy_service_unittest.cc (right):

http://codereview.chromium.org/160510/diff/54/66#newcode962
Line 962: // Succeed the initialization of the PAC script.
On 2009/08/03 23:45:48, willchan wrote:
> This sounds weird.  Perhaps "Successfully initialize the PAC script."?

Done.

http://codereview.chromium.org/160510/diff/54/66#newcode1352
Line 1352: // been sent to ti.
On 2009/08/03 23:45:48, willchan wrote:
> s/ti/it/

Done.

http://codereview.chromium.org/160510/diff/54/66#newcode1429
Line 1429: }
On 2009/08/03 23:45:48, willchan wrote:
> Are there tests for the suspension / resumption of requests?

ProxyServiceTest.InitialPACScriptDownload tests the resumption path (the
requests started while PAC is downloading are resumed on completion of the
download).

However, there is not a test that exercises SuspendAllPendingRequests()... at
least not yet. 

I am leaving a TODO for that, and will tackle it in a follow-up.

[eroman, 2009-08-04 01:13:56]

http://codereview.chromium.org/160510/diff/1036/1039
File net/proxy/single_threaded_proxy_resolver.cc (right):

http://codereview.chromium.org/160510/diff/1036/1039#newcode34
Line 34: AddRef();  // balanced in RequestComplete
On 2009/08/04 00:33:07, willchan wrote:
> Why is this AddRef() necessary?  Won't RunnableMethod manage the reference
> counts correctly?

That is a good question.

I copied the skeleton of the request job class, and didn't think about this too
much. It doesn't seem like like it should be necessary, but I would like to
think about this some more before removing it.

How about I tackle this in a follow-up, so I can clean up the other guy as well?

[willchan no longer on Chromium, 2009-08-04 01:18:14]

Some more nits.  It also looks like the linux trybot is failing for some reason,
so you might want to look into that.

http://codereview.chromium.org/160510/diff/54/73
File net/proxy/init_proxy_resolver.cc (right):

http://codereview.chromium.org/160510/diff/54/73#newcode136
Line 136: next_state_ = STATE_SET_PAC_SCRIPT;
On 2009/08/04 01:00:54, eroman wrote:
> On 2009/08/03 23:45:48, willchan wrote:
> > It seems weird to me that you set next_state_ here when
TryToFallbackPacUrl()
> is
> > already setting next_state_.  It looks like that sets the wrong state
though. 
> I
> > think it makes more sense for TryToFallbackPacUrl() not to set next_state_,
> and
> > rather have the call sites set it to whatever is right.
> 
> I don't understand what you mean by "wrong state"...

Oh, I'm brain damaged, ignore me, I misread the code.

http://codereview.chromium.org/160510/diff/54/65
File net/proxy/init_proxy_resolver_unittest.cc (right):

http://codereview.chromium.org/160510/diff/54/65#newcode29
Line 29: : url(url),
On 2009/08/04 01:00:54, eroman wrote:
> On 2009/08/03 23:45:48, willchan wrote:
> > Your indentation is off here, you need another whitespace
> 
> Really? I though I was supposed to indent it by 4 spaces...

Man, I'm blind, that indeed is 4 spaces :)

http://codereview.chromium.org/160510/diff/1036/1039
File net/proxy/single_threaded_proxy_resolver.cc (right):

http://codereview.chromium.org/160510/diff/1036/1039#newcode34
Line 34: AddRef();  // balanced in RequestComplete
On 2009/08/04 01:13:56, eroman wrote:
> On 2009/08/04 00:33:07, willchan wrote:
> > Why is this AddRef() necessary?  Won't RunnableMethod manage the reference
> > counts correctly?
> 
> That is a good question.
> 
> I copied the skeleton of the request job class, and didn't think about this
too
> much. It doesn't seem like like it should be necessary, but I would like to
> think about this some more before removing it.
> 
> How about I tackle this in a follow-up, so I can clean up the other guy as
well?

Fine with me, perhaps add a TODO to remind yourself?

http://codereview.chromium.org/160510/diff/129/1093
File net/proxy/proxy_resolver_mac.h (right):

http://codereview.chromium.org/160510/diff/129/1093#newcode41
Line 41: private:
This private: looks unnecessary.

http://codereview.chromium.org/160510/diff/129/1082
File net/proxy/proxy_resolver_v8.cc (right):

http://codereview.chromium.org/160510/diff/129/1082#newcode56
Line 56: explicit Context(ProxyResolverJSBindings* js_bindings) :
js_bindings_(js_bindings) {
80 columns

http://codereview.chromium.org/160510/diff/129/1091
File net/proxy/proxy_script_fetcher.h (right):

http://codereview.chromium.org/160510/diff/129/1091#newcode40
Line 40: virtual int Fetch(const GURL& url, std::string* bytes,
Need #include <string>

[eroman, 2009-08-04 01:30:56]

> Some more nits.  It also looks like the linux trybot is failing for some
> reason, so you might want to look into that.

Yeah, it was reaching a NOTREACHED I added to
resolve_proxy_msg_helper_unittest.cc.

I have fixed this (or will shortly).. I forgot I hadn't de-janked the
ResolveProxyMessageHelper unittest (so it still has dependency on
SingleThreadedProxyResolver).

http://codereview.chromium.org/160510/diff/1036/1039
File net/proxy/single_threaded_proxy_resolver.cc (right):

http://codereview.chromium.org/160510/diff/1036/1039#newcode34
Line 34: AddRef();  // balanced in RequestComplete
On 2009/08/04 01:18:14, willchan wrote:
> On 2009/08/04 01:13:56, eroman wrote:
> > On 2009/08/04 00:33:07, willchan wrote:
> > > Why is this AddRef() necessary?  Won't RunnableMethod manage the reference
> > > counts correctly?
> > 
> > That is a good question.
> > 
> > I copied the skeleton of the request job class, and didn't think about this
> too
> > much. It doesn't seem like like it should be necessary, but I would like to
> > think about this some more before removing it.
> > 
> > How about I tackle this in a follow-up, so I can clean up the other guy as
> well?
> 
> Fine with me, perhaps add a TODO to remind yourself?

Done.

http://codereview.chromium.org/160510/diff/129/1093
File net/proxy/proxy_resolver_mac.h (right):

http://codereview.chromium.org/160510/diff/129/1093#newcode41
Line 41: private:
On 2009/08/04 01:18:14, willchan wrote:
> This private: looks unnecessary.

Done.
Darn, was a left-over.

http://codereview.chromium.org/160510/diff/129/1082
File net/proxy/proxy_resolver_v8.cc (right):

http://codereview.chromium.org/160510/diff/129/1082#newcode56
Line 56: explicit Context(ProxyResolverJSBindings* js_bindings) :
js_bindings_(js_bindings) {
On 2009/08/04 01:18:14, willchan wrote:
> 80 columns

Done.

(Thanks for pointing that out, I picked it up when merging a conflict in a
recent sync).

http://codereview.chromium.org/160510/diff/129/1091
File net/proxy/proxy_script_fetcher.h (right):

http://codereview.chromium.org/160510/diff/129/1091#newcode40
Line 40: virtual int Fetch(const GURL& url, std::string* bytes,
On 2009/08/04 01:18:14, willchan wrote:
> Need #include <string>

Done.

[willchan no longer on Chromium, 2009-08-04 16:00:26]

LGTM

On 2009/08/04 01:30:56, eroman wrote:
> > Some more nits.  It also looks like the linux trybot is failing for some
> > reason, so you might want to look into that.
> 
> Yeah, it was reaching a NOTREACHED I added to
> resolve_proxy_msg_helper_unittest.cc.
> 
> I have fixed this (or will shortly).. I forgot I hadn't de-janked the
> ResolveProxyMessageHelper unittest (so it still has dependency on
> SingleThreadedProxyResolver).
> 
> http://codereview.chromium.org/160510/diff/1036/1039
> File net/proxy/single_threaded_proxy_resolver.cc (right):
> 
> http://codereview.chromium.org/160510/diff/1036/1039#newcode34
> Line 34: AddRef();  // balanced in RequestComplete
> On 2009/08/04 01:18:14, willchan wrote:
> > On 2009/08/04 01:13:56, eroman wrote:
> > > On 2009/08/04 00:33:07, willchan wrote:
> > > > Why is this AddRef() necessary?  Won't RunnableMethod manage the
reference
> > > > counts correctly?
> > > 
> > > That is a good question.
> > > 
> > > I copied the skeleton of the request job class, and didn't think about
this
> > too
> > > much. It doesn't seem like like it should be necessary, but I would like
to
> > > think about this some more before removing it.
> > > 
> > > How about I tackle this in a follow-up, so I can clean up the other guy as
> > well?
> > 
> > Fine with me, perhaps add a TODO to remind yourself?
> 
> Done.
> 
> http://codereview.chromium.org/160510/diff/129/1093
> File net/proxy/proxy_resolver_mac.h (right):
> 
> http://codereview.chromium.org/160510/diff/129/1093#newcode41
> Line 41: private:
> On 2009/08/04 01:18:14, willchan wrote:
> > This private: looks unnecessary.
> 
> Done.
> Darn, was a left-over.
> 
> http://codereview.chromium.org/160510/diff/129/1082
> File net/proxy/proxy_resolver_v8.cc (right):
> 
> http://codereview.chromium.org/160510/diff/129/1082#newcode56
> Line 56: explicit Context(ProxyResolverJSBindings* js_bindings) :
> js_bindings_(js_bindings) {
> On 2009/08/04 01:18:14, willchan wrote:
> > 80 columns
> 
> Done.
> 
> (Thanks for pointing that out, I picked it up when merging a conflict in a
> recent sync).
> 
> http://codereview.chromium.org/160510/diff/129/1091
> File net/proxy/proxy_script_fetcher.h (right):
> 
> http://codereview.chromium.org/160510/diff/129/1091#newcode40
> Line 40: virtual int Fetch(const GURL& url, std::string* bytes,
> On 2009/08/04 01:18:14, willchan wrote:
> > Need #include <string>
> 
> Done.