|
|
DescriptionEnable 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
Patch Set 1 #Patch Set 2 : Fix typo in field trial name. #Patch Set 3 : Revert "Enable Site Isolation for Extensions by default." #Patch Set 4 : Fix extensions test by initializing the test config later in test startup sequence. #
Total comments: 2
Patch Set 5 : Fix failing test classes subclassing ExtensionApiTest. #
Total comments: 4
Patch Set 6 : Remove the TearDownOnMainThread method in ExtensionWebstorePrivateApiTest #
Messages
Total messages: 45 (28 generated)
nasko@chromium.org changed reviewers: + creis@chromium.org, nick@chromium.org
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
The CQ bit was checked by nasko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by nasko@chromium.org
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.
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 ==========
Updated the description and fixed the typo in the json config for field trials.
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 ==========
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
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 ==========
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 ==========
The CQ bit was checked by nasko@chromium.org
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 ==========
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
creis@chromium.org changed reviewers: + rkaplow@chromium.org
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!
lgtm
The CQ bit was checked by nasko@chromium.org to run a CQ dry run
nasko@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
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
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
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)
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by nasko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
Patchset #5 (id:80001) has been deleted
The CQ bit was checked by nasko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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.
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.
oh, and s lgtm % the nit in webstore_private_apitest.cc
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.
The CQ bit was checked by nasko@chromium.org
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")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 120001, "attempt_start_ts": 1487187304748600, "parent_rev": "6c29ace559b19a2071f4402cd201d162abd1b7ac", "commit_rev": "d81f57c5700ed617cc183847ad3e0f35df1c8221"}
Message was sent while issue was closed.
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... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001) as https://chromium.googlesource.com/chromium/src/+/d81f57c5700ed617cc183847ad3e... |