Add a field trial to enable out-of-process V8 pac.

Add a field trial to enable out-of-process V8 pac.

At the same time, add a "disable" flag as an escape hatch for when
things go bad.

BUG=467832
Committed: https://crrev.com/5cd812f68b855416889bace1169d0c5a52d8db5d
Cr-Commit-Position: refs/heads/master@{#330175}

[Anand Mistry (off Chromium), 2015-05-12 02:27:34]

amistry@chromium.org changed reviewers:
+ eroman@chromium.org, rkaplow@chromium.org

[eroman, 2015-05-12 05:21:41]

https://codereview.chromium.org/1124083004/diff/20001/chrome/browser/net/prox...
File chrome/browser/net/proxy_service_factory.cc (right):

https://codereview.chromium.org/1124083004/diff/20001/chrome/browser/net/prox...
chrome/browser/net/proxy_service_factory.cc:124: bool use_v8 =
!command_line.HasSwitch(switches::kWinHttpProxyResolver);
I think the presence of this flag should also override out-of-process PAC. Since
if PAC goes out of process, there will not be support for using the system proxy
resolver (which is what kWinHttpProxyResolver actually means)

https://codereview.chromium.org/1124083004/diff/20001/chrome/browser/net/prox...
chrome/browser/net/proxy_service_factory.cc:174: if (!proxy_service &&
EnableOutOfProcessV8Pac(command_line)) {
rather than !proxy_service, can you do an "else if" ?

[Anand Mistry (off Chromium), 2015-05-12 07:16:04]

https://codereview.chromium.org/1124083004/diff/20001/chrome/browser/net/prox...
File chrome/browser/net/proxy_service_factory.cc (right):

https://codereview.chromium.org/1124083004/diff/20001/chrome/browser/net/prox...
chrome/browser/net/proxy_service_factory.cc:124: bool use_v8 =
!command_line.HasSwitch(switches::kWinHttpProxyResolver);
On 2015/05/12 05:21:40, eroman wrote:
> I think the presence of this flag should also override out-of-process PAC.
Since
> if PAC goes out of process, there will not be support for using the system
proxy
> resolver (which is what kWinHttpProxyResolver actually means)

Isn't that already the case? If this flag is set, use_v8 == false, and the code
path that creates the out-of-process resolver isn't hit and the system resolver
is created instead.

https://codereview.chromium.org/1124083004/diff/20001/chrome/browser/net/prox...
chrome/browser/net/proxy_service_factory.cc:174: if (!proxy_service &&
EnableOutOfProcessV8Pac(command_line)) {
On 2015/05/12 05:21:41, eroman wrote:
> rather than !proxy_service, can you do an "else if" ?

Done.

[Anand Mistry (off Chromium), 2015-05-13 22:25:33]

Ping! I'd like to get this in by branch point, if possible.

[rkaplow, 2015-05-14 14:15:48]

lgtm

lgtm but you still want someone with OWNERS for histograms, asvitkine for ex.

[Anand Mistry (off Chromium), 2015-05-14 21:32:34]

amistry@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Anand Mistry (off Chromium), 2015-05-14 21:32:34]

asvitkine@chromium.org: For histograms.xml

[eroman, 2015-05-14 22:28:28]

sorry was out sick, catching up on reviews ...

[eroman, 2015-05-14 22:31:29]

lgtm

https://codereview.chromium.org/1124083004/diff/20001/chrome/browser/net/prox...
File chrome/browser/net/proxy_service_factory.cc (right):

https://codereview.chromium.org/1124083004/diff/20001/chrome/browser/net/prox...
chrome/browser/net/proxy_service_factory.cc:124: bool use_v8 =
!command_line.HasSwitch(switches::kWinHttpProxyResolver);
On 2015/05/12 07:16:04, Anand Mistry wrote:
> On 2015/05/12 05:21:40, eroman wrote:
> > I think the presence of this flag should also override out-of-process PAC.
> Since
> > if PAC goes out of process, there will not be support for using the system
> proxy
> > resolver (which is what kWinHttpProxyResolver actually means)
> 
> Isn't that already the case? If this flag is set, use_v8 == false, and the
code
> path that creates the out-of-process resolver isn't hit and the system
resolver
> is created instead.

right you are!

https://codereview.chromium.org/1124083004/diff/40001/chrome/browser/net/prox...
File chrome/browser/net/proxy_service_factory.cc (right):

https://codereview.chromium.org/1124083004/diff/40001/chrome/browser/net/prox...
chrome/browser/net/proxy_service_factory.cc:165: // In-process Mojo PAC can only
be set on the command line, so it's presence
it's --> its

[Anand Mistry (off Chromium), 2015-05-14 23:23:38]

https://codereview.chromium.org/1124083004/diff/40001/chrome/browser/net/prox...
File chrome/browser/net/proxy_service_factory.cc (right):

https://codereview.chromium.org/1124083004/diff/40001/chrome/browser/net/prox...
chrome/browser/net/proxy_service_factory.cc:165: // In-process Mojo PAC can only
be set on the command line, so it's presence
On 2015/05/14 22:31:29, eroman wrote:
> it's --> its

Done.

[Anand Mistry (off Chromium), 2015-05-14 23:24:23]

The CQ bit was checked by amistry@chromium.org to run a CQ dry run

[Anand Mistry (off Chromium), 2015-05-14 23:24:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
rkaplow@chromium.org, eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/1124083004/#ps60001
(title: "Fix comment and rebase.")

[commit-bot: I haz the power, 2015-05-14 23:26:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1124083004/60001

[commit-bot: I haz the power, 2015-05-15 01:13:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-15 01:13:18]

Dry run: This issue passed the CQ dry run.

[Alexei Svitkine (slow), 2015-05-15 14:33:42]

lgtm % comment

https://codereview.chromium.org/1124083004/diff/60001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/1124083004/diff/60001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:2346: MULTI_VALUE_TYPE(kV8PacChoices),
Nit: Use ENABLE_DISABLE_VALUE_TYPE() instead.

[Anand Mistry (off Chromium), 2015-05-15 18:47:58]

https://codereview.chromium.org/1124083004/diff/60001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/1124083004/diff/60001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:2346: MULTI_VALUE_TYPE(kV8PacChoices),
On 2015/05/15 14:33:42, Alexei Svitkine wrote:
> Nit: Use ENABLE_DISABLE_VALUE_TYPE() instead.

Done.

[Anand Mistry (off Chromium), 2015-05-15 18:49:41]

The CQ bit was checked by amistry@chromium.org

[Anand Mistry (off Chromium), 2015-05-15 18:49:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
rkaplow@chromium.org, eroman@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1124083004/#ps80001
(title: "Address review comment.")

[commit-bot: I haz the power, 2015-05-15 18:50:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1124083004/80001

[commit-bot: I haz the power, 2015-05-15 19:53:40]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-05-15 19:54:30]

Patchset 5 (id:??) landed as
https://crrev.com/5cd812f68b855416889bace1169d0c5a52d8db5d
Cr-Commit-Position: refs/heads/master@{#330175}