Enable SiteIsolationExtensions trial for developers.

Enable SiteIsolationExtensions trial for developers.

This CL corrects a typo so that Isolate Extensions is enabled for
developer trunk builds, waterfall bots, perf bots, etc. The goal is
to get broader test coverage and usage of the mode as we turn it on
for Stable users.

This does not affect users who do not receive a field trial config, who still
still have the mode disabled. The mode can also be disabled manually:
--force-fieldtrials=SiteIsolationExtensions/Control

BUG=545200
Review-Url: https://codereview.chromium.org/2692153002
Cr-Commit-Position: refs/heads/master@{#450821}
Committed: https://chromium.googlesource.com/chromium/src/+/d81f57c5700ed617cc183847ad3e0f35df1c8221

[nasko, 2017-02-13 22:09:14]

nasko@chromium.org changed reviewers:
+ creis@chromium.org, nick@chromium.org

[nasko, 2017-02-13 22:09:15]

Hey Charlie and Nick,
This CL flips the default for Isolate Extensions to be enabled by default and
keeps the field trial config code in place.
Can you please review it for me?
Thanks in advance!
Nasko

[nasko, 2017-02-13 22:09:37]

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

[commit-bot: I haz the power, 2017-02-13 22:10:19]

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

[nasko, 2017-02-13 22:18:22]

The CQ bit was unchecked by nasko@chromium.org

[Charlie Reis, 2017-02-13 22:25:42]

LGTM!  Nick, do you see any potential issues?

I like that we're continuing to check the Finch trial, which gives us a way to
test with it disabled (i.e.,
--force-fieldtrials=SiteIsolationExtensions/Control) and avoids changing when or
how often we call FieldTrialList::FindFullName.

One request: can you update the CL description to clarify "enabled by default"
vs "Default or Control group"?  It's a bit confusing that the Finch "Default"
group means disabled, while "default" is enabled.  We certainly don't want to
change the Finch "Default" behavior yet-- this is probably just a matter of
rephrasing the description.

[nasko, 2017-02-13 23:16:32]

Description was changed from

==========
Enable Isolate Extensions by default.

This CL changes the default behavior of Isolate Extensions and sets it
to be enabled by default. The field trial checks remain in place and it
can be disabled by placing the client in the Default or Control group.

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

to

==========
Enable Isolate Extensions by default.

This CL changes the behavior of Isolate Extensions and sets it
to be enabled by when it isn't explicitly configured by field trial.
The field trial checks remain in place and it can be disabled by 
placing the client in the "Default" or "Control" Finch groups, 
which both turn off the mode.

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

[nasko, 2017-02-13 23:42:13]

Updated the description and fixed the typo in the json config for field trials.

[nasko, 2017-02-14 00:05:13]

Description was changed from

==========
Enable Isolate Extensions by default.

This CL changes the behavior of Isolate Extensions and sets it
to be enabled by when it isn't explicitly configured by field trial.
The field trial checks remain in place and it can be disabled by 
placing the client in the "Default" or "Control" Finch groups, 
which both turn off the mode.

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

to

==========
Enable SiteIsolationExtensions trial.

This CL changes the behavior of Isolate Extensions to be enabled
unless explicitly disabled through field trial by placing the client
in the "Default" or "Control" groups, which both turn off the mode.

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

[Charlie Reis, 2017-02-14 00:23:38]

Thanks for bearing with Nick and me on chat about this.  :)  For the record, we
decided to not change IsIsolateExtensionsEnabled() to return true in the absence
of Finch, but rather to make Finch return "Enabled" in more places (i.e.,
developer builds, waterfall bots, perf bots, etc).

There were arguments for both sides-- making it return true in the absence of
Finch means that anyone who doesn't contact Finch will have the mode on.  That
made sense to me when I thought that developer builds and waterfall bots didn't
check in with Finch, since we want the mode to be enabled for them.  But it
turns out those cases are covered by fieldtrial_testing_config.json, and our
earlier CL for that file  (https://codereview.chromium.org/2658873002) had a
typo that didn't actually enable the mode for them.

On the flip side, returning true in the absence of Finch means we have these two
different confusing meanings of the word "default."  The mode would be on by
default if you can't reach Finch, but Finch's "Default" group turns it off. 
Since there's not many users who don't get a Finch config (at least that we care
about for testing this mode), it's arguably better to avoid the confusion.

Thus, I like PS3.  We're just making Finch return "Enabled" for developers,
which the end goal.

Can you update the CL description to the following?  Otherwise LGTM.

Enable SiteIsolationExtensions trial for developers.

This CL corrects a typo so that Isolate Extensions is enabled for
developer trunk builds, waterfall bots, perf bots, etc.  The goal is
to get broader test coverage and usage of the mode as we turn it on
for Stable users.

This does not affect users who do not receive a Finch config, who still
still have the mode disabled.  The mode can also be disabled manually:
--force-fieldtrials=SiteIsolationExtensions/Control

[nasko, 2017-02-14 00:32:54]

Description was changed from

==========
Enable SiteIsolationExtensions trial.

This CL changes the behavior of Isolate Extensions to be enabled
unless explicitly disabled through field trial by placing the client
in the "Default" or "Control" groups, which both turn off the mode.

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

to

==========
Enable SiteIsolationExtensions trial.

This CL corrects a typo so that Isolate Extensions is enabled for
developer trunk builds, waterfall bots, perf bots, etc. The goal is
to get broader test coverage and usage of the mode as we turn it on
for Stable users.

This does not affect users who do not receive a Finch config, who still
still have the mode disabled. The mode can also be disabled manually:
--force-fieldtrials=SiteIsolationExtensions/Control

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

[nasko, 2017-02-14 00:33:14]

Description was changed from

==========
Enable SiteIsolationExtensions trial.

This CL corrects a typo so that Isolate Extensions is enabled for
developer trunk builds, waterfall bots, perf bots, etc. The goal is
to get broader test coverage and usage of the mode as we turn it on
for Stable users.

This does not affect users who do not receive a Finch config, who still
still have the mode disabled. The mode can also be disabled manually:
--force-fieldtrials=SiteIsolationExtensions/Control

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

to

==========
Enable SiteIsolationExtensions trial for developers.

This CL corrects a typo so that Isolate Extensions is enabled for
developer trunk builds, waterfall bots, perf bots, etc. The goal is
to get broader test coverage and usage of the mode as we turn it on
for Stable users.

This does not affect users who do not receive a Finch config, who still
still have the mode disabled. The mode can also be disabled manually:
--force-fieldtrials=SiteIsolationExtensions/Control

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

[nasko, 2017-02-14 00:33:32]

The CQ bit was checked by nasko@chromium.org

[nasko, 2017-02-14 00:34:04]

Description was changed from

==========
Enable SiteIsolationExtensions trial for developers.

This CL corrects a typo so that Isolate Extensions is enabled for
developer trunk builds, waterfall bots, perf bots, etc. The goal is
to get broader test coverage and usage of the mode as we turn it on
for Stable users.

This does not affect users who do not receive a Finch config, who still
still have the mode disabled. The mode can also be disabled manually:
--force-fieldtrials=SiteIsolationExtensions/Control

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

to

==========
Enable SiteIsolationExtensions trial for developers.

This CL corrects a typo so that Isolate Extensions is enabled for
developer trunk builds, waterfall bots, perf bots, etc. The goal is
to get broader test coverage and usage of the mode as we turn it on
for Stable users.

This does not affect users who do not receive a field trial config, who still
still have the mode disabled. The mode can also be disabled manually:
--force-fieldtrials=SiteIsolationExtensions/Control

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

[commit-bot: I haz the power, 2017-02-14 00:34:29]

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, 2017-02-14 00:45:40]

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

[commit-bot: I haz the power, 2017-02-14 00:45:41]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Charlie Reis, 2017-02-14 00:49:20]

creis@chromium.org changed reviewers:
+ rkaplow@chromium.org

[Charlie Reis, 2017-02-14 00:49:21]

rkaplow@, can you review for owners approval?  (Basically just fixing a typo,
but there's some discussion of why we think this is sufficient in the CL review
comments.)  Thanks!

[rkaplow, 2017-02-14 01:34:25]

lgtm

[nasko, 2017-02-14 23:24:48]

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

[nasko, 2017-02-14 23:25:33]

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

[nasko, 2017-02-14 23:25:34]

Hey Devlin,
Can you review this CL for me? It implements the later initialization of test
config which we discussed offline.
Thanks in advance!
Nasko

[commit-bot: I haz the power, 2017-02-14 23:25:50]

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

[ncarter (slow), 2017-02-14 23:32:05]

lgtm

[Devlin, 2017-02-14 23:37:46]

lgtm

https://codereview.chromium.org/2692153002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/extension_apitest.cc (right):

https://codereview.chromium.org/2692153002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_apitest.cc:167: void
ExtensionApiTest::TearDownInProcessBrowserTestFixture() {
For symmetry's sake, let's make this TearDownOnMainThread() (and call
ExtensionBrowserTest::TearDownOnMainThread)

[commit-bot: I haz the power, 2017-02-15 00:38:34]

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

[commit-bot: I haz the power, 2017-02-15 00:38:35]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[nasko, 2017-02-15 17:10:54]

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

[commit-bot: I haz the power, 2017-02-15 17:11:37]

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, 2017-02-15 18:03:42]

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

[commit-bot: I haz the power, 2017-02-15 18:03:44]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[nasko, 2017-02-15 18:30:18]

Patchset #5 (id:80001) has been deleted

[nasko, 2017-02-15 18:30:23]

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

[commit-bot: I haz the power, 2017-02-15 18:31:41]

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

[nasko, 2017-02-15 19:20:25]

Devlin, can you take another look? I had to fix a few more places in extensions
world.

https://codereview.chromium.org/2692153002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/extension_apitest.cc (right):

https://codereview.chromium.org/2692153002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_apitest.cc:167: void
ExtensionApiTest::TearDownInProcessBrowserTestFixture() {
On 2017/02/14 23:37:46, Devlin wrote:
> For symmetry's sake, let's make this TearDownOnMainThread() (and call
> ExtensionBrowserTest::TearDownOnMainThread)

Done.

[Devlin, 2017-02-15 19:24:34]

https://codereview.chromium.org/2692153002/diff/100001/chrome/browser/extensi...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client_apitest.cc
(left):

https://codereview.chromium.org/2692153002/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/api/networking_private/networking_private_service_client_apitest.cc:107:
ExtensionApiTest::SetUpOnMainThread();
Heh not the first time I've seen this.  It'd be great if there was a way we
could have a warning for this ("Hey, it looks like you're calling the wrong
super version of the method"), but I don't have a good heuristic... ah well.

https://codereview.chromium.org/2692153002/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc
(right):

https://codereview.chromium.org/2692153002/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc:131:
void TearDownOnMainThread() override {
Shouldn't need this.

[Devlin, 2017-02-15 19:24:54]

oh, and s lgtm % the nit in webstore_private_apitest.cc

[nasko, 2017-02-15 19:34:57]

https://codereview.chromium.org/2692153002/diff/100001/chrome/browser/extensi...
File
chrome/browser/extensions/api/networking_private/networking_private_service_client_apitest.cc
(left):

https://codereview.chromium.org/2692153002/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/api/networking_private/networking_private_service_client_apitest.cc:107:
ExtensionApiTest::SetUpOnMainThread();
On 2017/02/15 19:24:34, Devlin wrote:
> Heh not the first time I've seen this.  It'd be great if there was a way we
> could have a warning for this ("Hey, it looks like you're calling the wrong
> super version of the method"), but I don't have a good heuristic... ah well.

Acknowledged.

https://codereview.chromium.org/2692153002/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc
(right):

https://codereview.chromium.org/2692153002/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc:131:
void TearDownOnMainThread() override {
On 2017/02/15 19:24:34, Devlin wrote:
> Shouldn't need this.

Done.

[nasko, 2017-02-15 19:35:04]

The CQ bit was checked by nasko@chromium.org

[nasko, 2017-02-15 19:35:05]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org,
rkaplow@chromium.org, nick@chromium.org, rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2692153002/#ps120001
(title: "Remove the TearDownOnMainThread method in
ExtensionWebstorePrivateApiTest")

[commit-bot: I haz the power, 2017-02-15 19:35:53]

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, 2017-02-15 22:46:55]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1487187304748600,
"parent_rev": "6c29ace559b19a2071f4402cd201d162abd1b7ac", "commit_rev":
"d81f57c5700ed617cc183847ad3e0f35df1c8221"}

[commit-bot: I haz the power, 2017-02-15 22:47:38]

Description was changed from

==========
Enable SiteIsolationExtensions trial for developers.

This CL corrects a typo so that Isolate Extensions is enabled for
developer trunk builds, waterfall bots, perf bots, etc. The goal is
to get broader test coverage and usage of the mode as we turn it on
for Stable users.

This does not affect users who do not receive a field trial config, who still
still have the mode disabled. The mode can also be disabled manually:
--force-fieldtrials=SiteIsolationExtensions/Control

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

to

==========
Enable SiteIsolationExtensions trial for developers.

This CL corrects a typo so that Isolate Extensions is enabled for
developer trunk builds, waterfall bots, perf bots, etc. The goal is
to get broader test coverage and usage of the mode as we turn it on
for Stable users.

This does not affect users who do not receive a field trial config, who still
still have the mode disabled. The mode can also be disabled manually:
--force-fieldtrials=SiteIsolationExtensions/Control

BUG=545200

Review-Url: https://codereview.chromium.org/2692153002
Cr-Commit-Position: refs/heads/master@{#450821}
Committed:
https://chromium.googlesource.com/chromium/src/+/d81f57c5700ed617cc183847ad3e...
==========

[commit-bot: I haz the power, 2017-02-15 22:47:39]

Committed patchset #6 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/d81f57c5700ed617cc183847ad3e...