Added field trial for enabling AppContainer and a chrome://flags entry.

Added field trial for enabling AppContainer and a chrome://flags entry.
This CL adds support for field trials for enabling the AppContainer
lockdown policy as well as exposing the setting via the chrome://flags page.

BUG=455496
Committed: https://crrev.com/8e087947c7bf6f4d235d672b0d1ad35f5aeafe8d
Cr-Commit-Position: refs/heads/master@{#357620}

[forshaw, 2015-11-02 14:12:21]

forshaw@chromium.org changed reviewers:
+ asvitkine@chromium.org, jam@chromium.org, wfh@chromium.org

[forshaw, 2015-11-02 14:12:21]

Please can you take a look at this patch to add support for trialing
AppContainer support on Windows as well as exposing it through chrome://flags. 

jam@chromium.org: Please review changes in chrome/*
wfh@chromium.org: Please review changes in sandbox_win.cc
asvitkine@chromium.org: Please review changes in histograms.xml

Thanks.

[Alexei Svitkine (slow), 2015-11-02 17:27:52]

https://codereview.chromium.org/1407883005/diff/1/content/common/sandbox_win.cc
File content/common/sandbox_win.cc (right):

https://codereview.chromium.org/1407883005/diff/1/content/common/sandbox_win....
content/common/sandbox_win.cc:576: appcontainer_trial_name == "Enabled") {
Does this code run in the child process, browser process or both?

I'm asking because I'd like to suggest you to use the new base/feature_list.h
API (which automatically can integrate with FieldTrials without using the
FieldTrial API explicitly), but currently there's a limitation with it that has
an issue with content/ code that runs in the browser process. (I have a CL
fixing this, but it hasn't landed yet).

If this doesn't run in the browser process, then I think it should be fine to
switch to the new API.

[jam, 2015-11-02 18:30:44]

lgtm

[forshaw, 2015-11-02 18:53:53]

On 2015/11/02 17:27:52, Alexei Svitkine (slow) wrote:
>
https://codereview.chromium.org/1407883005/diff/1/content/common/sandbox_win.cc
> File content/common/sandbox_win.cc (right):
> 
>
https://codereview.chromium.org/1407883005/diff/1/content/common/sandbox_win....
> content/common/sandbox_win.cc:576: appcontainer_trial_name == "Enabled") {
> Does this code run in the child process, browser process or both?
> 
> I'm asking because I'd like to suggest you to use the new base/feature_list.h
> API (which automatically can integrate with FieldTrials without using the
> FieldTrial API explicitly), but currently there's a limitation with it that
has
> an issue with content/ code that runs in the browser process. (I have a CL
> fixing this, but it hasn't landed yet).
> 
> If this doesn't run in the browser process, then I think it should be fine to
> switch to the new API.

asvitkine@, This runs entirely in the browser process and it's not likely to
change as it isn't possible to change the sandbox security setting this controls
in the sandboxed processes. As most of this code is pre-existing and just adding
the FieldTrial call I'm not sure I want to change it too much, especially if it
isn't working correctly atm for the browser process.

[Alexei Svitkine (slow), 2015-11-02 18:58:55]

On 2015/11/02 18:53:53, forshaw wrote:
> asvitkine@, This runs entirely in the browser process and it's not likely to
> change as it isn't possible to change the sandbox security setting this
controls
> in the sandboxed processes. As most of this code is pre-existing and just
adding
> the FieldTrial call I'm not sure I want to change it too much, especially if
it
> isn't working correctly atm for the browser process.

That's fair.

https://codereview.chromium.org/1407883005/diff/1/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/1407883005/diff/1/chrome/browser/about_flags....
chrome/browser/about_flags.cc:2128:
SINGLE_VALUE_TYPE(switches::kEnableAppContainer)},
Suggest making this ENABLE_DISABLE_VALUE_TYPE() with both an enable and disable
switch.

Now that this will be controlled by a field trial, a user's default state may
actually be enabled and if you're providing control in about:flags, there should
be a way to turn it off as well.

[forshaw, 2015-11-03 10:52:13]

PTAL, I've added an explicit disable flag and changed the about flags property
to suit.

[forshaw, 2015-11-03 11:37:52]

Patchset #2 (id:20001) has been deleted

[Alexei Svitkine (slow), 2015-11-03 16:32:08]

lgtm

[Will Harris, 2015-11-03 18:40:54]

lgtm

[forshaw, 2015-11-03 18:43:31]

The CQ bit was checked by forshaw@chromium.org

[forshaw, 2015-11-03 18:43:31]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1407883005/#ps40001
(title: "Added disable switch and changed flags")

[commit-bot: I haz the power, 2015-11-03 18:44:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1407883005/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1407883005/40001

[forshaw, 2015-11-03 18:44:13]

On 2015/11/03 18:40:54, Will Harris wrote:
> lgtm

Thanks jam@, asvitkine@ and wfh@ for the reviews and advice on best practice.
Much appreciated.

[commit-bot: I haz the power, 2015-11-03 21:00:07]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2015-11-03 21:01:53]

Patchset 2 (id:??) landed as
https://crrev.com/8e087947c7bf6f4d235d672b0d1ad35f5aeafe8d
Cr-Commit-Position: refs/heads/master@{#357620}