Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2834583002:
Change ScopedFeatureList to overrides FeatureList not reset (Closed)
DescriptionChange 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
Patch Set 1 #
Total comments: 36
Patch Set 2 : addressed some comments from Ilya #
Total comments: 21
Patch Set 3 : Ilya comments addressed #
Total comments: 9
Patch Set 4 : Alexei and Ilya comments addressed #
Total comments: 10
Patch Set 5 : Ilya comments addressed #Patch Set 6 : Alexei comment addressed #
Dependent Patchsets: Messages
Total messages: 44 (21 generated)
The CQ bit was checked by chaopeng@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: This issue passed the CQ dry run.
Description was changed from ========== Change ScopedFeatureLists to overrides FeatureList not reset BUG=713390 ========== to ========== Change ScopedFeatureLists to overrides FeatureList not reset BUG=713390 ==========
chaopeng@chromium.org changed reviewers: + isherman@chromium.org
Hi Ilya, it seems OK to change all to InitWithFeatureListAndOverrides. WDYT?
isherman@chromium.org changed reviewers: + asvitkine@chromium.org
+Alexei as an optional reviewer (FYI) Thanks, Jianpeng. https://codereview.chromium.org/2834583002/diff/1/base/test/scoped_feature_li... File base/test/scoped_feature_list.cc (right): https://codereview.chromium.org/2834583002/diff/1/base/test/scoped_feature_li... base/test/scoped_feature_list.cc:48: const std::initializer_list<base::Feature>& disabled_features) { Could this still be the main entry point? The API is pretty much identical to InitWithFeatureListAndOverrides(). https://codereview.chromium.org/2834583002/diff/1/base/test/scoped_feature_li... base/test/scoped_feature_list.cc:54: std::unique_ptr<FeatureList> feature_list) { Is this method really needed, or could it be replaced by calls to other Init() functions? It seems like an unnecessary way to circumvent any previously set FeatureList state. https://codereview.chromium.org/2834583002/diff/1/base/test/scoped_feature_li... base/test/scoped_feature_list.cc:62: const std::string& disable_features) { Could this either be removed or call into InitWithFeatures()? As above, it seems like an unnecessary escape from previously set FeatureList state. https://codereview.chromium.org/2834583002/diff/1/base/test/scoped_feature_li... base/test/scoped_feature_list.cc:70: InitWithFeatureListAndOverrides(feature_list, std::vector<std::string>()); nit: I think this could be simplified to InitWithFeatures({feature}, {}); A similar transformation could be made below. https://codereview.chromium.org/2834583002/diff/1/base/test/scoped_feature_li... base/test/scoped_feature_list.cc:78: void OverrideFeature(const std::string& features, Please add documentation for this function. https://codereview.chromium.org/2834583002/diff/1/base/test/scoped_feature_li... base/test/scoped_feature_list.cc:78: void OverrideFeature(const std::string& features, nit: s/Feature/Features https://codereview.chromium.org/2834583002/diff/1/base/test/scoped_feature_li... base/test/scoped_feature_list.cc:80: std::vector<std::string>* add_to_list) { I'd suggest writing this method like so: void AddFeatureOverrides(const std::string& features, const std::set<StringPiece>& already_overridden_feature_names, std::vector<std::string>* overrides) { std::vector<StringPiece> features_list = SplitStringPiece(features, ",", TRIM_WHITESPACE, SPLIT_WANT_NONEMPTY); for (const StringPiece& feature : features_list) { // Extract out a GetFeatureName() function. StringPiece feature_name = GetFeatureName(feature); if (already_overridden_feature_names.count(feature_name)) continue; overrides->push_back(feature); } } The already_overridden_feature_names set could contain both the enabled and the disabled features -- it doesn't matter in which state they were overridden. Writing the code this way obviates the need for some of the comments below, but please still pay attention to my questions about correctness, and add test coverage as needed to address them. https://codereview.chromium.org/2834583002/diff/1/base/test/scoped_feature_li... base/test/scoped_feature_list.cc:87: return; Should this be a continue instead? (If so, please add test coverage for this.) Actually, if field trial state isn't dropped, it seems more appropriate to keep these features. And it's not clear to me why we'd want to drop field trial state. https://codereview.chromium.org/2834583002/diff/1/base/test/scoped_feature_li... base/test/scoped_feature_list.cc:93: feature_name = feature.substr(0, index); Hmm, why is the field trial info being removed? Shouldn't it affect the feature state sometimes? https://codereview.chromium.org/2834583002/diff/1/base/test/scoped_feature_li... base/test/scoped_feature_list.cc:95: if (std::any_of( nit: std::find? https://codereview.chromium.org/2834583002/diff/1/base/test/scoped_feature_li... base/test/scoped_feature_list.cc:98: return; Ditto regarding continue vs. return. https://codereview.chromium.org/2834583002/diff/1/base/test/scoped_feature_li... base/test/scoped_feature_list.cc:103: return; Ditto regarding continue vs. return. https://codereview.chromium.org/2834583002/diff/1/base/test/scoped_feature_li... base/test/scoped_feature_list.cc:107: } nit: Please move this into the anonymous namespace, since it's a private implementation detail. https://codereview.chromium.org/2834583002/diff/1/base/test/scoped_feature_li... base/test/scoped_feature_list.cc:124: &merged_enabled_features); I think the latter two args are swapped? Please add test coverage for this if I'm not mistaken. https://codereview.chromium.org/2834583002/diff/1/base/test/scoped_feature_li... File base/test/scoped_feature_list_unittest.cc (right): https://codereview.chromium.org/2834583002/diff/1/base/test/scoped_feature_li... base/test/scoped_feature_list_unittest.cc:9: #include "testing/gmock/include/gmock/gmock.h" nit: Is gmock being used? https://codereview.chromium.org/2834583002/diff/1/base/test/scoped_feature_li... base/test/scoped_feature_list_unittest.cc:16: void SetUp() override { nit: Please do this work in the constructor instead. It's rare that SetUp() is preferable to doing work in the constructor. https://codereview.chromium.org/2834583002/diff/1/base/test/scoped_feature_li... base/test/scoped_feature_list_unittest.cc:32: } nit: This doesn't need to be part of the class AFAICT, so please move it into an anonymous namespace instead. https://codereview.chromium.org/2834583002/diff/1/base/test/scoped_feature_li... base/test/scoped_feature_list_unittest.cc:35: test::ScopedFeatureList feature_list; nit: Private members should end with a trailing underscore. https://codereview.chromium.org/2834583002/diff/1/base/test/scoped_feature_li... base/test/scoped_feature_list_unittest.cc:51: test::ScopedFeatureList feature_list1; I think it would be more realistic for the outer list to not be a ScopedFeatureList, but rather a regular feature list. This does mean that the test will duplicate some of the work that the ScopedFeatureList does, but I think it will be clearer as a result. Please try to structure these tests in the arrange/act/assert style, as described here: https://goto.google.com/unit-test-practices#structure
Patchset #2 (id:20001) has been deleted
Updated, PTAL. Thank you. https://codereview.chromium.org/2834583002/diff/1/base/test/scoped_feature_li... File base/test/scoped_feature_list.cc (right): https://codereview.chromium.org/2834583002/diff/1/base/test/scoped_feature_li... base/test/scoped_feature_list.cc:48: const std::initializer_list<base::Feature>& disabled_features) { On 2017/04/20 20:53:07, Ilya Sherman wrote: > Could this still be the main entry point? The API is pretty much identical to > InitWithFeatureListAndOverrides(). Yes. https://codereview.chromium.org/2834583002/diff/1/base/test/scoped_feature_li... base/test/scoped_feature_list.cc:54: std::unique_ptr<FeatureList> feature_list) { On 2017/04/20 20:53:07, Ilya Sherman wrote: > Is this method really needed, or could it be replaced by calls to other Init() > functions? It seems like an unnecessary way to circumvent any previously set > FeatureList state. This method has lot calls. I think it is better to have a separate CL to remove it. Do we still provider methods for reset features in case developers want only enable/disable the features they need? https://codereview.chromium.org/2834583002/diff/1/base/test/scoped_feature_li... base/test/scoped_feature_list.cc:62: const std::string& disable_features) { On 2017/04/20 20:53:07, Ilya Sherman wrote: > Could this either be removed or call into InitWithFeatures()? As above, it > seems like an unnecessary escape from previously set FeatureList state. same as below. https://codereview.chromium.org/2834583002/diff/1/base/test/scoped_feature_li... base/test/scoped_feature_list.cc:70: InitWithFeatureListAndOverrides(feature_list, std::vector<std::string>()); On 2017/04/20 20:53:07, Ilya Sherman wrote: > nit: I think this could be simplified to > > InitWithFeatures({feature}, {}); > > A similar transformation could be made below. Done. https://codereview.chromium.org/2834583002/diff/1/base/test/scoped_feature_li... base/test/scoped_feature_list.cc:78: void OverrideFeature(const std::string& features, On 2017/04/20 20:53:07, Ilya Sherman wrote: > nit: s/Feature/Features Done. https://codereview.chromium.org/2834583002/diff/1/base/test/scoped_feature_li... base/test/scoped_feature_list.cc:80: std::vector<std::string>* add_to_list) { On 2017/04/20 20:53:07, Ilya Sherman wrote: > I'd suggest writing this method like so: > > void AddFeatureOverrides(const std::string& features, > const std::set<StringPiece>& > already_overridden_feature_names, > std::vector<std::string>* overrides) { > std::vector<StringPiece> features_list = > SplitStringPiece(features, ",", TRIM_WHITESPACE, SPLIT_WANT_NONEMPTY); > for (const StringPiece& feature : features_list) { > // Extract out a GetFeatureName() function. > StringPiece feature_name = GetFeatureName(feature); > if (already_overridden_feature_names.count(feature_name)) continue; > overrides->push_back(feature); > } > } > > The already_overridden_feature_names set could contain both the enabled and the > disabled features -- it doesn't matter in which state they were overridden. > > Writing the code this way obviates the need for some of the comments below, but > please still pay attention to my questions about correctness, and add test > coverage as needed to address them. Done. https://codereview.chromium.org/2834583002/diff/1/base/test/scoped_feature_li... base/test/scoped_feature_list.cc:87: return; On 2017/04/20 20:53:07, Ilya Sherman wrote: > Should this be a continue instead? (If so, please add test coverage for this.) > > Actually, if field trial state isn't dropped, it seems more appropriate to keep > these features. And it's not clear to me why we'd want to drop field trial > state. Yes, we should keep it. https://codereview.chromium.org/2834583002/diff/1/base/test/scoped_feature_li... base/test/scoped_feature_list.cc:93: feature_name = feature.substr(0, index); On 2017/04/20 20:53:07, Ilya Sherman wrote: > Hmm, why is the field trial info being removed? Shouldn't it affect the feature > state sometimes? Not remove it now. https://codereview.chromium.org/2834583002/diff/1/base/test/scoped_feature_li... base/test/scoped_feature_list.cc:95: if (std::any_of( On 2017/04/20 20:53:07, Ilya Sherman wrote: > nit: std::find? change to count https://codereview.chromium.org/2834583002/diff/1/base/test/scoped_feature_li... base/test/scoped_feature_list.cc:98: return; On 2017/04/20 20:53:07, Ilya Sherman wrote: > Ditto regarding continue vs. return. Done. https://codereview.chromium.org/2834583002/diff/1/base/test/scoped_feature_li... base/test/scoped_feature_list.cc:103: return; On 2017/04/20 20:53:07, Ilya Sherman wrote: > Ditto regarding continue vs. return. Done. https://codereview.chromium.org/2834583002/diff/1/base/test/scoped_feature_li... base/test/scoped_feature_list.cc:107: } On 2017/04/20 20:53:07, Ilya Sherman wrote: > nit: Please move this into the anonymous namespace, since it's a private > implementation detail. Done. https://codereview.chromium.org/2834583002/diff/1/base/test/scoped_feature_li... base/test/scoped_feature_list.cc:124: &merged_enabled_features); On 2017/04/20 20:53:07, Ilya Sherman wrote: > I think the latter two args are swapped? Please add test coverage for this if > I'm not mistaken. Done. https://codereview.chromium.org/2834583002/diff/1/base/test/scoped_feature_li... File base/test/scoped_feature_list_unittest.cc (right): https://codereview.chromium.org/2834583002/diff/1/base/test/scoped_feature_li... base/test/scoped_feature_list_unittest.cc:9: #include "testing/gmock/include/gmock/gmock.h" On 2017/04/20 20:53:07, Ilya Sherman wrote: > nit: Is gmock being used? Done. https://codereview.chromium.org/2834583002/diff/1/base/test/scoped_feature_li... base/test/scoped_feature_list_unittest.cc:16: void SetUp() override { On 2017/04/20 20:53:08, Ilya Sherman wrote: > nit: Please do this work in the constructor instead. It's rare that SetUp() is > preferable to doing work in the constructor. Done. https://codereview.chromium.org/2834583002/diff/1/base/test/scoped_feature_li... base/test/scoped_feature_list_unittest.cc:32: } On 2017/04/20 20:53:07, Ilya Sherman wrote: > nit: This doesn't need to be part of the class AFAICT, so please move it into an > anonymous namespace instead. Done. https://codereview.chromium.org/2834583002/diff/1/base/test/scoped_feature_li... base/test/scoped_feature_list_unittest.cc:35: test::ScopedFeatureList feature_list; On 2017/04/20 20:53:08, Ilya Sherman wrote: > nit: Private members should end with a trailing underscore. Done.
Thanks! https://codereview.chromium.org/2834583002/diff/40001/base/test/scoped_featur... File base/test/scoped_feature_list.cc (right): https://codereview.chromium.org/2834583002/diff/40001/base/test/scoped_featur... base/test/scoped_feature_list.cc:19: static std::vector<std::string> GetFeatureString( nit: s/String/Vector https://codereview.chromium.org/2834583002/diff/40001/base/test/scoped_featur... base/test/scoped_feature_list.cc:19: static std::vector<std::string> GetFeatureString( nit: No need for "static" within an anonymous namespace. (Applies to the functions defined below as well.) https://codereview.chromium.org/2834583002/diff/40001/base/test/scoped_featur... base/test/scoped_feature_list.cc:22: for (const base::Feature& feature : features) nit: Please add curly braces. I'm not sure whether they're optional for loops as well as if-stmts; but within the metrics code, we prefer to always include them for loop constructs. https://codereview.chromium.org/2834583002/diff/40001/base/test/scoped_featur... base/test/scoped_feature_list.cc:28: static StringPiece GetFeatureName(const StringPiece& feature) { Please add documentation for this function. https://codereview.chromium.org/2834583002/diff/40001/base/test/scoped_featur... base/test/scoped_feature_list.cc:28: static StringPiece GetFeatureName(const StringPiece& feature) { Per the StringPiece documentation, it's preferable to pass StringPieces by value rather than const-ref: https://cs.chromium.org/chromium/src/base/strings/string_piece.h?l=14-20 https://codereview.chromium.org/2834583002/diff/40001/base/test/scoped_featur... base/test/scoped_feature_list.cc:31: // Remove started *. This just repeats what the code does. Please instead document what the idea is, i.e. what the asterisk means. https://codereview.chromium.org/2834583002/diff/40001/base/test/scoped_featur... base/test/scoped_feature_list.cc:33: feature_name.substr(1); Did you mean to assign this back to feature_name? If so, please add test coverage. https://codereview.chromium.org/2834583002/diff/40001/base/test/scoped_featur... base/test/scoped_feature_list.cc:44: // if it is not existing in already_overridden_feature_names or overrides. If you take my advice about passing a unioned set as the second param, you can simplify this implementation not to separately consider the overrides list. https://codereview.chromium.org/2834583002/diff/40001/base/test/scoped_featur... base/test/scoped_feature_list.cc:52: for (StringPiece& feature : features_list) { nit: Chromium strongly frowns upon non-const references. Since it's preferable to pass StringPiece by value, this should probably be a copy as well. But if you were to want a reference, it should be a const-ref. https://codereview.chromium.org/2834583002/diff/40001/base/test/scoped_featur... base/test/scoped_feature_list.cc:107: void ScopedFeatureList::InitWithFeatures( Please position this within the .cc file in the same order as it appears in the .h file. https://codereview.chromium.org/2834583002/diff/40001/base/test/scoped_featur... base/test/scoped_feature_list.cc:113: GetFeatureString(disabled_features); Please construct a set containing the union of these features, and pass it as the second param to OverrideFeatures. In addition to increasing code clarity (IMO), it also guards against the case that a feature is being overridden to be enabled in a separate state than it was in the existing FeatureList. (e.g. with different field trial params) https://codereview.chromium.org/2834583002/diff/40001/base/test/scoped_featur... base/test/scoped_feature_list.cc:121: OverrideFeatures(enabled_features, &merged_disabled_features, The second param is not an outparam, so please pass it by const-reference rather than as a pointer. https://codereview.chromium.org/2834583002/diff/40001/base/test/scoped_featur... File base/test/scoped_feature_list.h (right): https://codereview.chromium.org/2834583002/diff/40001/base/test/scoped_featur... base/test/scoped_feature_list.h:32: // enabled and disabled features (comma-separated names). For this function and the two above it, please add a warning that these should be used only in very rare cases. Something like: // WARNING: This method will reset any globally configured features to their // default values, which can hide feature interaction bugs. Please use // sparingly. https://crbug.com/NNNNNN https://codereview.chromium.org/2834583002/diff/40001/base/test/scoped_featur... base/test/scoped_feature_list.h:36: // Initializes and registers a FeatureList instance overrided with the given nit: s/overrided/overridden (applies throughout) https://codereview.chromium.org/2834583002/diff/40001/base/test/scoped_featur... base/test/scoped_feature_list.h:37: // enabled and disabled features. nit: To add a bit of clarity here, please add a bit more descriptive text, along the lines of // Any feature overrides already present in the global FeatureList will // continue to apply, unless they conflict with the overrides passed into this // method. This is important for testing potentially unexpected feature // interactions. (I think it's fine to only go into this extra detail for this method, and leave the simpler docs for the two methods below.) https://codereview.chromium.org/2834583002/diff/40001/base/test/scoped_featur... File base/test/scoped_feature_list_unittest.cc (right): https://codereview.chromium.org/2834583002/diff/40001/base/test/scoped_featur... base/test/scoped_feature_list_unittest.cc:12: static void ExpectFeatures(const std::string& enabled_features, nit: No need for 'static' in an anonymous namespace. https://codereview.chromium.org/2834583002/diff/40001/base/test/scoped_featur... base/test/scoped_feature_list_unittest.cc:34: feature_list->InitializeFromCommandLine("", ""); nit: Prefer std::string() to "" for empty strings. https://codereview.chromium.org/2834583002/diff/40001/base/test/scoped_featur... base/test/scoped_feature_list_unittest.cc:39: void TearDown() override { Please do this work in the destructor rather than in the TearDown() method. https://codereview.chromium.org/2834583002/diff/40001/base/test/scoped_featur... base/test/scoped_feature_list_unittest.cc:49: std::unique_ptr<FeatureList> original_feature_list_; Please document this member variable. https://codereview.chromium.org/2834583002/diff/40001/base/test/scoped_featur... base/test/scoped_feature_list_unittest.cc:53: const Feature kTestFeature2{"TestFeature2", FEATURE_DISABLED_BY_DEFAULT}; nit: Please move these into the anonymous namespace. https://codereview.chromium.org/2834583002/diff/40001/base/test/scoped_featur... base/test/scoped_feature_list_unittest.cc:88: } Please also test that if the feature is overridden in one way (e.g. with field trial info specified), and then subsequently overridden in a different way, the latter override wins.
Updated, PTAL. Thank you. https://codereview.chromium.org/2834583002/diff/60001/base/test/scoped_featur... File base/test/scoped_feature_list.cc (right): https://codereview.chromium.org/2834583002/diff/60001/base/test/scoped_featur... base/test/scoped_feature_list.cc:54: Features* merged_features) { Is this same as your comment?
Thanks for working on this. Please expand CL description to explain this change more and the reason for it. Thanks! https://codereview.chromium.org/2834583002/diff/60001/base/test/scoped_featur... File base/test/scoped_feature_list.cc (right): https://codereview.chromium.org/2834583002/diff/60001/base/test/scoped_featur... base/test/scoped_feature_list.cc:21: std::vector<std::string> output; Can this be a StringPiece vector? https://codereview.chromium.org/2834583002/diff/60001/base/test/scoped_featur... base/test/scoped_feature_list.cc:61: if (std::count(merged_features->enabled_feature_list.begin(), Nit: Use ContainsValue() from base/stl_util.h https://codereview.chromium.org/2834583002/diff/60001/base/test/scoped_featur... base/test/scoped_feature_list.cc:74: merged_features->disabled_feature_list.push_back(feature.as_string()); DCHECK that it's OVERRIDE_DISABLE_FEATURE? https://codereview.chromium.org/2834583002/diff/60001/base/test/scoped_featur... File base/test/scoped_feature_list_unittest.cc (right): https://codereview.chromium.org/2834583002/diff/60001/base/test/scoped_featur... base/test/scoped_feature_list_unittest.cc:33: namespace base { Use the same namespace as the .cc code - which is base::test Also, put this around the anon namespace block above. https://codereview.chromium.org/2834583002/diff/60001/base/test/scoped_featur... base/test/scoped_feature_list_unittest.cc:57: }; DISALLOW_COPY_AND_ASSIGN()
Thanks! Just a few nits left to address =) https://codereview.chromium.org/2834583002/diff/60001/base/test/scoped_featur... File base/test/scoped_feature_list.cc (right): https://codereview.chromium.org/2834583002/diff/60001/base/test/scoped_featur... base/test/scoped_feature_list.cc:29: // Get feature name for feature string from FeatureList::GetFeatureOverrides. nit: I'd rephrase this to something like // Extracts a feature name from a feature state string. For example, given // the input "*MyLovelyFeature<SomeFieldTrial", returns "MyLovelyFeature". https://codereview.chromium.org/2834583002/diff/60001/base/test/scoped_featur... base/test/scoped_feature_list.cc:51: // merged_features if it is not existing in merged_features. The semantics of the params are not entirely obvious (at least, not to me), so please document the expected semantics. https://codereview.chromium.org/2834583002/diff/60001/base/test/scoped_featur... base/test/scoped_feature_list.cc:54: Features* merged_features) { On 2017/04/22 03:18:15, chaopeng wrote: > Is this same as your comment? It's not really what I had in mind, but I guess this way is ok too.
Description was changed from ========== Change ScopedFeatureLists to overrides FeatureList not reset BUG=713390 ========== to ========== 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. We keep SFL::Init, SFL::InitWithFeatureList, SFL::InitFromCommandLine reset to empty list and warning to remind developer should use them with care. 2. We 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 ==========
Description was changed from ========== 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. We keep SFL::Init, SFL::InitWithFeatureList, SFL::InitFromCommandLine reset to empty list and warning to remind developer should use them with care. 2. We 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 ========== to ========== 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 and 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 ==========
Description was changed from ========== 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 and 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 ========== to ========== 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 ==========
Updated. PTAL. Thank you. https://codereview.chromium.org/2834583002/diff/80001/base/test/scoped_featur... File base/test/scoped_feature_list.cc (right): https://codereview.chromium.org/2834583002/diff/80001/base/test/scoped_featur... base/test/scoped_feature_list.cc:123: std::string disables; Re Alexei: I need to move this 2 line out of if-scope if I want to change GetFeatureVector returns vector<StringPiece>.
Thanks! LG % some nits, plus Alexei's request to use a StringPiece vector. https://codereview.chromium.org/2834583002/diff/80001/base/test/scoped_featur... File base/test/scoped_feature_list.cc (right): https://codereview.chromium.org/2834583002/diff/80001/base/test/scoped_featur... base/test/scoped_feature_list.cc:55: // merged_features. nit: Suggested rephrasing: // Merges previously-specified feature overrides with those passed into one of // the Init() methods. |features| should be a list of features previously // overridden to be in the |override_state|. |merged_features| should contain // the enabled and disabled features passed into the Init() method, plus any // overrides merged as a result of previous calls to this function. Of course, feel free to refine this further. https://codereview.chromium.org/2834583002/diff/80001/base/test/scoped_featur... base/test/scoped_feature_list.cc:72: DCHECK(override_state == nit: DCHECK_EQ https://codereview.chromium.org/2834583002/diff/80001/base/test/scoped_featur... base/test/scoped_feature_list.cc:123: std::string disables; nit: Please restore the previous variable names of "enabled_features" and "disabled_features". They were clearer. https://codereview.chromium.org/2834583002/diff/80001/base/test/scoped_featur... base/test/scoped_feature_list.cc:123: std::string disables; On 2017/04/25 21:21:07, chaopeng wrote: > Re Alexei: I need to move this 2 line out of if-scope if I want to change > GetFeatureVector returns vector<StringPiece>. Hmm, why do you need to move these in order to make that change? I'm not following the connection. https://codereview.chromium.org/2834583002/diff/80001/base/test/scoped_featur... File base/test/scoped_feature_list_unittest.cc (right): https://codereview.chromium.org/2834583002/diff/80001/base/test/scoped_featur... base/test/scoped_feature_list_unittest.cc:57: std::unique_ptr<FeatureList> original_feature_list_; nit: Please leave a blank line between this and DISALLOW_COPY_AND_ASSIGN
Patchset #5 (id:100001) has been deleted
Updated, PTAL. https://codereview.chromium.org/2834583002/diff/80001/base/test/scoped_featur... File base/test/scoped_feature_list.cc (right): https://codereview.chromium.org/2834583002/diff/80001/base/test/scoped_featur... base/test/scoped_feature_list.cc:123: std::string disables; On 2017/04/25 21:41:58, Ilya Sherman wrote: > nit: Please restore the previous variable names of "enabled_features" and > "disabled_features". They were clearer. these names conflict with the function arguments. I rename them to current_*. https://codereview.chromium.org/2834583002/diff/80001/base/test/scoped_featur... base/test/scoped_feature_list.cc:123: std::string disables; On 2017/04/25 21:41:58, Ilya Sherman wrote: > On 2017/04/25 21:21:07, chaopeng wrote: > > Re Alexei: I need to move this 2 line out of if-scope if I want to change > > GetFeatureVector returns vector<StringPiece>. > > Hmm, why do you need to move these in order to make that change? I'm not > following the connection. StringPiece is not alloc new memory but save a pointer to reuse the string's memory. The StringPiece we push_back at line 70 is from these two line and will out-of-scope before JoinString if I change |struct Features| and |GetFeatureVector| to using StringPiece.
Thanks! LGTM, but please wait for Alexei's review as well. https://codereview.chromium.org/2834583002/diff/80001/base/test/scoped_featur... File base/test/scoped_feature_list.cc (right): https://codereview.chromium.org/2834583002/diff/80001/base/test/scoped_featur... base/test/scoped_feature_list.cc:123: std::string disables; On 2017/04/26 01:04:39, chaopeng wrote: > On 2017/04/25 21:41:58, Ilya Sherman wrote: > > On 2017/04/25 21:21:07, chaopeng wrote: > > > Re Alexei: I need to move this 2 line out of if-scope if I want to change > > > GetFeatureVector returns vector<StringPiece>. > > > > Hmm, why do you need to move these in order to make that change? I'm not > > following the connection. > > StringPiece is not alloc new memory but save a pointer to reuse the string's > memory. > The StringPiece we push_back at line 70 is from these two line and will > out-of-scope before JoinString if I change |struct Features| and > |GetFeatureVector| to using StringPiece. Ah, yes, that makes sense. Thanks for the explanation. I think it would be fine to hoist the two variable up above the if-stmt, with a brief comment mentioning that it's necessary for StringPiece references to remain valid. I also don't feel strongly about using a StringPiece vector in the first place; will defer to Alexei on that.
https://codereview.chromium.org/2834583002/diff/80001/base/test/scoped_featur... File base/test/scoped_feature_list.cc (right): https://codereview.chromium.org/2834583002/diff/80001/base/test/scoped_featur... base/test/scoped_feature_list.cc:123: std::string disables; On 2017/04/25 21:21:07, chaopeng wrote: > Re Alexei: I need to move this 2 line out of if-scope if I want to change > GetFeatureVector returns vector<StringPiece>. Moving out of the if SGTM to me, with a corresponding comment.
On 2017/04/27 14:58:08, Alexei Svitkine (slow) wrote: > https://codereview.chromium.org/2834583002/diff/80001/base/test/scoped_featur... > File base/test/scoped_feature_list.cc (right): > > https://codereview.chromium.org/2834583002/diff/80001/base/test/scoped_featur... > base/test/scoped_feature_list.cc:123: std::string disables; > On 2017/04/25 21:21:07, chaopeng wrote: > > Re Alexei: I need to move this 2 line out of if-scope if I want to change > > GetFeatureVector returns vector<StringPiece>. > > Moving out of the if SGTM to me, with a corresponding comment. Updated. PTAL. Thank you.
lgtm
The CQ bit was checked by chaopeng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2834583002/#ps140001 (title: "Alexei comment addressed")
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...)
chaopeng@chromium.org changed reviewers: + dcheng@chromium.org
dcheng@chromium.org: Please review changes.
chaopeng@chromium.org changed reviewers: + thakis@chromium.org - dcheng@chromium.org
thakis: PTAL. Thank you.
rs-lgtm
The CQ bit was checked by chaopeng@chromium.org
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": 140001, "attempt_start_ts": 1493428543724780,
"parent_rev": "83c9f3f4faeb35a3a2607446def65f1a8da7aa49", "commit_rev":
"9c04ed553bd7abe820a6a93c5e8981e6738881a9"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/9c04ed553bd7abe820a6a93c5e89... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:140001) as https://chromium.googlesource.com/chromium/src/+/9c04ed553bd7abe820a6a93c5e89...
Message was sent while issue was closed.
Findit(https://goo.gl/kROfz5) identified this CL at revision 468210 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:140001) has been created in https://codereview.chromium.org/2850073002/ by thestig@chromium.org. The reason for reverting is: Mac ASAN bots reporting use-after-free errors..
Message was sent while issue was closed.
Description was changed from ========== 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/+/9c04ed553bd7abe820a6a93c5e89... ========== to ========== 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/+/9c04ed553bd7abe820a6a93c5e89... ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
