Add support for field trial config for Isolate Extensions.

Add support for field trial config for Isolate Extensions.

This CL adds support for field trials of Isolate Extensions with a
default behavior of being enabled.

BUG=545200
Committed: https://crrev.com/33dd5ea6a929ba57f0d1d419c5e9064efce5b23f
Cr-Commit-Position: refs/heads/master@{#425198}

[nasko, 2016-10-13 17:38:04]

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

[commit-bot: I haz the power, 2016-10-13 17:38:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-13 17:55:05]

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

[commit-bot: I haz the power, 2016-10-13 17:55:06]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[nasko, 2016-10-13 18:26:59]

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

[commit-bot: I haz the power, 2016-10-13 18:27:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nasko, 2016-10-13 18:38:54]

nasko@chromium.org changed reviewers:
+ nick@chromium.org, rdevlin.cronin@chromium.org

[nasko, 2016-10-13 18:38:54]

Hey Nick and Devlin,
Can you review this CL for me? It adds the capability of disabling Isolate
Extensions through field trial, but remains on by default.
Thanks in advance!
Nasko

[nasko, 2016-10-13 18:41:47]

Description was changed from

==========
Add support for field trial config for Isolate Extensions.

This CL adds support for field trials of Isolate Extensions with a
default behavior of being enabled.

BUG=545200
==========

to

==========
Add support for field trial config for Isolate Extensions.

This CL adds support for field trials of Isolate Extensions with a
default behavior of being enabled.

BUG=545200
==========

[Devlin, 2016-10-13 19:44:27]

lgtm, but can you give a bit more reason why we need this?  Is it just for the
ability to turn it off?  If so, should we rename the group to reflect that the
default is on?  ("Control" makes it sound like the default)

https://codereview.chromium.org/2417873002/diff/20001/chrome/common/extension...
File chrome/common/extensions/extension_process_policy.cc (right):

https://codereview.chromium.org/2417873002/diff/20001/chrome/common/extension...
chrome/common/extensions/extension_process_policy.cc:73: bool control_group =
base::StartsWith(group_name, "Control",
nitty nit: maybe "is_control_group"?

https://codereview.chromium.org/2417873002/diff/20001/chrome/common/extension...
chrome/common/extensions/extension_process_policy.cc:75: if (control_group)
any reason to not just return !is_control_group here?

[nasko, 2016-10-13 20:33:55]

> Is it just for the ability to turn it off?  

Yes!

> If so, should we rename the group to reflect that the
> default is on?  ("Control" makes it sound like the default)

The field trial config has existed for almost a year now, so changing it is not
desirable. The goal of this is to exist for as short period of time as possible.

https://codereview.chromium.org/2417873002/diff/20001/chrome/common/extension...
File chrome/common/extensions/extension_process_policy.cc (right):

https://codereview.chromium.org/2417873002/diff/20001/chrome/common/extension...
chrome/common/extensions/extension_process_policy.cc:73: bool control_group =
base::StartsWith(group_name, "Control",
On 2016/10/13 19:44:27, Devlin wrote:
> nitty nit: maybe "is_control_group"?

Done.

https://codereview.chromium.org/2417873002/diff/20001/chrome/common/extension...
chrome/common/extensions/extension_process_policy.cc:75: if (control_group)
On 2016/10/13 19:44:27, Devlin wrote:
> any reason to not just return !is_control_group here?

Done.

[nasko, 2016-10-13 20:34:08]

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

[commit-bot: I haz the power, 2016-10-13 20:34:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ncarter (slow), 2016-10-13 20:39:45]

https://codereview.chromium.org/2417873002/diff/30001/chrome/common/extension...
File chrome/common/extensions/extension_process_policy.cc (right):

https://codereview.chromium.org/2417873002/diff/30001/chrome/common/extension...
chrome/common/extensions/extension_process_policy.cc:74: group_name, "Control",
base::CompareCase::INSENSITIVE_ASCII);
Is Control already at zero percent for M55+?

What behavior do we get if a browser starts up with no internet connection, and
hence no available finch config?

[commit-bot: I haz the power, 2016-10-13 21:17:27]

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

[commit-bot: I haz the power, 2016-10-13 21:17:28]

Dry run: This issue passed the CQ dry run.

[ncarter (slow), 2016-10-13 21:21:07]

lgtm

[nasko, 2016-10-13 22:22:40]

nasko@chromium.org changed reviewers:
+ thestig@chromium.org

[nasko, 2016-10-13 22:22:40]

Hey Lei,
Can you review this CL for OWNERS?
Thanks in advance!
Nasko

https://codereview.chromium.org/2417873002/diff/30001/chrome/common/extension...
File chrome/common/extensions/extension_process_policy.cc (right):

https://codereview.chromium.org/2417873002/diff/30001/chrome/common/extension...
chrome/common/extensions/extension_process_policy.cc:74: group_name, "Control",
base::CompareCase::INSENSITIVE_ASCII);
On 2016/10/13 20:39:45, ncarter (ooo 10-14 back 10-17) wrote:
> Is Control already at zero percent for M55+?

Yes.

> What behavior do we get if a browser starts up with no internet connection,
and
> hence no available finch config?

I think it falls back to previously cached response from the server.

[Lei Zhang, 2016-10-13 22:27:53]

lgtm

[nasko, 2016-10-13 22:45:24]

The CQ bit was checked by nasko@chromium.org

[nasko, 2016-10-13 22:45:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2417873002/#ps30001
(title: "Address review comments.")

[commit-bot: I haz the power, 2016-10-13 22:45:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-13 22:51:28]

Description was changed from

==========
Add support for field trial config for Isolate Extensions.

This CL adds support for field trials of Isolate Extensions with a
default behavior of being enabled.

BUG=545200
==========

to

==========
Add support for field trial config for Isolate Extensions.

This CL adds support for field trials of Isolate Extensions with a
default behavior of being enabled.

BUG=545200
==========

[commit-bot: I haz the power, 2016-10-13 22:51:29]

Committed patchset #3 (id:30001)

[commit-bot: I haz the power, 2016-10-13 22:53:06]

Description was changed from

==========
Add support for field trial config for Isolate Extensions.

This CL adds support for field trials of Isolate Extensions with a
default behavior of being enabled.

BUG=545200
==========

to

==========
Add support for field trial config for Isolate Extensions.

This CL adds support for field trials of Isolate Extensions with a
default behavior of being enabled.

BUG=545200

Committed: https://crrev.com/33dd5ea6a929ba57f0d1d419c5e9064efce5b23f
Cr-Commit-Position: refs/heads/master@{#425198}
==========

[commit-bot: I haz the power, 2016-10-13 22:53:08]

Patchset 3 (id:??) landed as
https://crrev.com/33dd5ea6a929ba57f0d1d419c5e9064efce5b23f
Cr-Commit-Position: refs/heads/master@{#425198}