Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(203)

Issue 2850073002: Revert of Change ScopedFeatureList to overrides FeatureList not reset (Closed)

Created:
3 years, 7 months ago by Lei Zhang
Modified:
3 years, 7 months ago
CC:
chromium-reviews, danakj+watch_chromium.org, vmpstr+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Revert of Change ScopedFeatureList to overrides FeatureList not reset (patchset #6 id:140001 of https://codereview.chromium.org/2834583002/ ) Reason for revert: Mac ASAN bots reporting use-after-free errors. Original issue's description: > Change ScopedFeatureList to overrides FeatureList not reset > > The current situation is that using ScopedFeatureList resets to an > empty feature list and then enables/disables an explicit list of > features. > > That's never what you want for browser tests (or other higher-level > tests) since it effectively overrides higher-level test configurations > (e.g. those in fieldtrial_testing_config.json, or a bot set up to > specifically test a feature). > > In this patch: > > 1. Keep SFL::Init, SFL::InitWithFeatureList, > SFL::InitFromCommandLine reset to empty list but add warning to > remind developer should use them with care. > 2. Change SFL::InitAndEnableFeature, SFL::InitAndDisableFeature and > SFL::InitWithFeatures to not reset but override current FeatureList > with given enables/disables. > > We also add unit tests for ScopedFeatureList. > > BUG=713390 > > Review-Url: https://codereview.chromium.org/2834583002 > Cr-Commit-Position: refs/heads/master@{#468210} > Committed: https://chromium.googlesource.com/chromium/src/+/9c04ed553bd7abe820a6a93c5e8981e6738881a9 TBR=isherman@chromium.org,asvitkine@chromium.org,thakis@chromium.org,chaopeng@chromium.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG=713390 Review-Url: https://codereview.chromium.org/2850073002 Cr-Commit-Position: refs/heads/master@{#468263} Committed: https://chromium.googlesource.com/chromium/src/+/6c14f269fb196627c72a810205488d694b63a7d5

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+25 lines, -306 lines) Patch
M base/BUILD.gn View 1 chunk +0 lines, -1 line 0 comments Download
M base/test/scoped_feature_list.h View 1 chunk +11 lines, -24 lines 0 comments Download
M base/test/scoped_feature_list.cc View 3 chunks +14 lines, -93 lines 0 comments Download
D base/test/scoped_feature_list_unittest.cc View 1 chunk +0 lines, -188 lines 0 comments Download

Messages

Total messages: 12 (5 generated)
Lei Zhang
Created Revert of Change ScopedFeatureList to overrides FeatureList not reset
3 years, 7 months ago (2017-04-30 20:58:14 UTC) #2
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2850073002/1
3 years, 7 months ago (2017-04-30 20:58:24 UTC) #3
Lei Zhang
Examples of failing bots: https://build.chromium.org/p/chromium.memory/builders/Mac%20ASan%2064%20Tests%20%281%29/builds/29621 https://build.chromium.org/p/chromium.mac/builders/Mac10.9%20Tests/builds/40520
3 years, 7 months ago (2017-04-30 20:58:47 UTC) #4
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/442426)
3 years, 7 months ago (2017-04-30 22:24:37 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2850073002/1
3 years, 7 months ago (2017-04-30 22:36:39 UTC) #8
commit-bot: I haz the power
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/6c14f269fb196627c72a810205488d694b63a7d5
3 years, 7 months ago (2017-05-01 00:00:29 UTC) #11
chaopeng
3 years, 5 months ago (2017-06-28 19:22:18 UTC) #12
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/2962963002/ by chaopeng@chromium.org.

The reason for reverting is: Ready for reland..

Powered by Google App Engine
This is Rietveld 408576698