Refactor: Extract "InitProxyResolver" to "ProxyScriptDecider".

Refactor: Extract "InitProxyResolver" to "ProxyScriptDecider".

This is primarily a rename, with the exception that the initialization of ProxyResolver (i.e. the javascript runtime environment for the PAC script) is no longer done inside of ProxyScriptDecider.

The motivation for doing this is to make it possible to poll our automatic proxy settings (PAC URLs or WPAD) in the background, _without_ needing to feed the scripts into the javascript parser for validation.

There likely won't be any user-visible consequence of this change, however the new mechanism is weaker than the original -- it is possible for the PAC selection to now choose a PAC script which fails to parse as javascript, even though there was a later fallback choice which does parse. This should be rare though (the bad response would need to contain the substring "FindProxyForURL").

BUG=TODO (bugtracker down right now)
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=114281

[willchan no longer on Chromium, 2011-12-12 23:53:47]

lgtm

http://codereview.chromium.org/8896019/diff/8001/net/proxy/proxy_script_decid...
File net/proxy/proxy_script_decider.cc (right):

http://codereview.chromium.org/8896019/diff/8001/net/proxy/proxy_script_decid...
net/proxy/proxy_script_decider.cc:232: next_state_ = STATE_VERIFY_PAC_SCRIPT;
Can you just move the DoVerifyPacScript() code in here? I personally like it
when all the DoFoo() calls have DoFooComplete().

http://codereview.chromium.org/8896019/diff/8001/net/proxy/proxy_script_decid...
File net/proxy/proxy_script_decider.h (right):

http://codereview.chromium.org/8896019/diff/8001/net/proxy/proxy_script_decid...
net/proxy/proxy_script_decider.h:79: // TODO(eroman): Return a const-pointer.
Is there a reason you didn't do this now? Is this nontrivial?

http://codereview.chromium.org/8896019/diff/8001/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/8896019/diff/8001/net/proxy/proxy_service.cc#n...
net/proxy/proxy_service.cc:325: // things similar. It will probably end up
changing while solving bug XXX.
bug XXX?

[eroman, 2011-12-13 00:10:26]

I'll ping when new patchset is up. Here are initial responses:

http://codereview.chromium.org/8896019/diff/8001/net/proxy/proxy_script_decid...
File net/proxy/proxy_script_decider.cc (right):

http://codereview.chromium.org/8896019/diff/8001/net/proxy/proxy_script_decid...
net/proxy/proxy_script_decider.cc:232: next_state_ = STATE_VERIFY_PAC_SCRIPT;
On 2011/12/12 23:53:47, willchan wrote:
> Can you just move the DoVerifyPacScript() code in here? I personally like it
> when all the DoFoo() calls have DoFooComplete().

There are situations where we start at STATE_VERIFY_PAC_SCRIPT (see
GetStartState()), so inlining isn't ideal.

What I'll do instead is add a DoVerifyPacScriptComplete() function, even though
the current behavior is synchronous. At some point I may re-introduce proper
javascript parsing again, at which stage it would need to be async anyway.

http://codereview.chromium.org/8896019/diff/8001/net/proxy/proxy_script_decid...
File net/proxy/proxy_script_decider.h (right):

http://codereview.chromium.org/8896019/diff/8001/net/proxy/proxy_script_decid...
net/proxy/proxy_script_decider.h:79: // TODO(eroman): Return a const-pointer.
On 2011/12/12 23:53:47, willchan wrote:
> Is there a reason you didn't do this now? Is this nontrivial?

Unfortunately it is non-trivial since one of the interfaces which consumes this
data, ProxyResolver::SetPacScript() is expecting a non-const pointer (for no
good reason). Before I can fix this, I need to massage the interface and its
consumers.

[willchan no longer on Chromium, 2011-12-13 01:17:09]

http://codereview.chromium.org/8896019/diff/8001/net/proxy/proxy_script_decid...
File net/proxy/proxy_script_decider.cc (right):

http://codereview.chromium.org/8896019/diff/8001/net/proxy/proxy_script_decid...
net/proxy/proxy_script_decider.cc:232: next_state_ = STATE_VERIFY_PAC_SCRIPT;
On 2011/12/13 00:10:26, eroman wrote:
> On 2011/12/12 23:53:47, willchan wrote:
> > Can you just move the DoVerifyPacScript() code in here? I personally like it
> > when all the DoFoo() calls have DoFooComplete().
> 
> There are situations where we start at STATE_VERIFY_PAC_SCRIPT (see
> GetStartState()), so inlining isn't ideal.
> 
> What I'll do instead is add a DoVerifyPacScriptComplete() function, even
though
> the current behavior is synchronous. At some point I may re-introduce proper
> javascript parsing again, at which stage it would need to be async anyway.

OIC, ok this makes sense.

[eroman, 2011-12-13 02:25:30]

Uploaded new patchset.

@battre: Please review my change to parse_error.js (during PAC detection the
script isn't executed as javascript unless it contains the substring
"FindProxyForURL").

[eroman, 2011-12-13 04:14:11]

Actually add Dominic this time.

[battre, 2011-12-13 10:03:28]

On 2011/12/13 04:14:11, eroman wrote:
> Actually add Dominic this time.

Can you explain the need for the change to parse_errors.js to me? The old
version seemed broken enough to be to trigger an error.

I am somewhat concerned about the flag that indicates that the PAC script is
mandatory and that we must not fall back to the system proxy configuration. The
old parse_errors.js should have been broken enough to trigger this mechanism.

[eroman, 2011-12-13 19:21:21]

@battre:

Prior to this change, the decision on which PAC script to use (in the presence
of 1 or more options) was made by:

  (1) Try to download the PAC script -- if it fails, the script was 'bad' and
won't be used

  (2) Feed the downloaded PAC script to the ProxyResolver (i.e. javascript
environment). If it fails to parse, the script was 'bad' and won't be used.


Since the proxy error extension API works by listening to any errors thrown by
the ProxyResolver, it was observing the errors from this decision test as a
side-effect.

In my change I modified step (2), so the regular ProxyResolver is no longer used
as a tester of PAC scripts when deciding. (I needed to do this since I want the
ability to test PAC scripts in background, while the main ProxyResolver may
already be configured and in-use). To do the testing I could have provisioned a
separate Javascript environment and did the same parse test in this isolated
context. (whose errors will not be witnessed by extension API). In the future I
may in fact spawn a new javascript environment for the sake of validation, but
what I am doing right now is a simpler validation test:  If the script does not
contain the string "FindProxyForURL" it is deemed invalid and won't be chosen.

Once the PAC script passes this validation test and is chosen, it is fed into
the ProxyResolver as before, and will now generate the parse error since it
isn't valid javascript.

To make that test continue to pass, I added the string "FindProxyForURL" so that
it makes it past the validation step, and then fails in parsing inside
ProxyResolver as before.

[battre, 2011-12-13 20:43:44]

Thanks for the explanation.

LGTM for parse_error.js.

[commit-bot: I haz the power, 2011-12-13 20:52:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eroman@chromium.org/8896019/20001

[commit-bot: I haz the power, 2011-12-13 20:53:12]

Can't apply patch for file net/proxy/proxy_script_decider.cc.
While running patch -p0 --forward --force;
patching file net/proxy/proxy_script_decider.cc
Hunk #1 FAILED at 2.
Hunk #2 FAILED at 32.
Hunk #3 FAILED at 84.
Hunk #4 FAILED at 105.
Hunk #5 FAILED at 135.
Hunk #6 FAILED at 151.
Hunk #7 FAILED at 166.
Hunk #8 FAILED at 193.
Hunk #9 FAILED at 206.
Hunk #10 FAILED at 214.
Hunk #11 FAILED at 302.
Hunk #12 FAILED at 338.
Hunk #13 FAILED at 364.
Hunk #14 FAILED at 376.
14 out of 14 hunks FAILED -- saving rejects to file
net/proxy/proxy_script_decider.cc.rej

[commit-bot: I haz the power, 2011-12-13 21:23:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eroman@chromium.org/8896019/20006

[commit-bot: I haz the power, 2011-12-13 21:24:05]

Can't apply patch for file net/proxy/proxy_script_decider.cc.
While running patch -p0 --forward --force;
patching file net/proxy/proxy_script_decider.cc
Hunk #1 FAILED at 2.
Hunk #2 FAILED at 32.
Hunk #3 FAILED at 84.
Hunk #4 FAILED at 105.
Hunk #5 FAILED at 135.
Hunk #6 FAILED at 151.
Hunk #7 FAILED at 166.
Hunk #8 FAILED at 193.
Hunk #9 FAILED at 206.
Hunk #10 FAILED at 214.
Hunk #11 FAILED at 302.
Hunk #12 FAILED at 338.
Hunk #13 FAILED at 364.
Hunk #14 FAILED at 376.
14 out of 14 hunks FAILED -- saving rejects to file
net/proxy/proxy_script_decider.cc.rej