Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1278403003:
Initial implementation of FeatureList in base/. (Closed)
|
Created:
5 years, 4 months ago by Alexei Svitkine (slow) Modified:
5 years, 3 months ago CC:
chromium-reviews, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionInitial implementation of FeatureList in base/.
This first CL adds the initial version of FeatureList singleton class,
providing support for the following:
- Initial class structure and singleton registration
- Support for --enable-features= and --disable-features= flags
- API to test if a feature is enabled or not
- Its registration in chrome_browser_main.cc
- Debug checks to ensure each Feature struct is defined only once
- Basic unit tests
Parts that will be implemented in follow-up CLs:
- Integration with FieldTrials (split out into a follow-up CL)
- VariationsService integration and kill switch support
- Various bits above base/ (e.g. about flags, sub-process stuff ,..)
BUG=526169
Committed: https://crrev.com/bccbb867acc61bb9b8c88e3a7b0cbee6b08df0de
Cr-Commit-Position: refs/heads/master@{#347438}
Patch Set 1 : #
Total comments: 10
Patch Set 2 : Address comments. #
Total comments: 10
Patch Set 3 : More comments addressed. #
Total comments: 55
Patch Set 4 : Address isherman@'s comments. #
Total comments: 11
Patch Set 5 : Rebase. #Patch Set 6 : Move to base/ from base/metrics/ #Patch Set 7 : Update comments. #Patch Set 8 : Update enums and owners file. #Patch Set 9 : Rebase. #
Messages
Total messages: 59 (33 generated)
Patchset #4 (id:60001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #5 (id:120001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #5 (id:160001) has been deleted
Patchset #3 (id:100001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #4 (id:200001) has been deleted
Patchset #2 (id:140001) has been deleted
Patchset #4 (id:240001) has been deleted
Patchset #2 (id:180001) has been deleted
Patchset #4 (id:280001) has been deleted
Patchset #3 (id:260001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:220001) has been deleted
Patchset #1 (id:300001) has been deleted
Patchset #1 (id:320001) has been deleted
Patchset #1 (id:340001) has been deleted
Patchset #1 (id:340001) has been deleted
Patchset #2 (id:380001) has been deleted
Patchset #1 (id:360001) has been deleted
asvitkine@chromium.org changed reviewers: + brettw@chromium.org, rkaplow@chromium.org
Patchset #1 (id:400001) has been deleted
asvitkine@chromium.org changed reviewers: + isherman@chromium.org
+isherman
https://codereview.chromium.org/1278403003/diff/420001/base/metrics/feature_l... File base/metrics/feature_list.h (right): https://codereview.chromium.org/1278403003/diff/420001/base/metrics/feature_l... base/metrics/feature_list.h:23: // The name of the feagture. feature https://codereview.chromium.org/1278403003/diff/420001/base/metrics/feature_l... base/metrics/feature_list.h:43: // if (base::FeatureList::IsEnabled(kMyGreatFeature)) { super minor nit but for even for sample syntax I would have a close brace https://codereview.chromium.org/1278403003/diff/420001/base/metrics/feature_l... base/metrics/feature_list.h:46: // flags to enable or disable the feature, any experiments that may control it I would make it clear here the ordering of priority - which I assume is command line, then experiment, then default. https://codereview.chromium.org/1278403003/diff/420001/base/metrics/feature_l... base/metrics/feature_list.h:78: bool IsFeatureEnabled(const Feature& feature); can this be private?
https://codereview.chromium.org/1278403003/diff/420001/base/metrics/feature_l... File base/metrics/feature_list.h (right): https://codereview.chromium.org/1278403003/diff/420001/base/metrics/feature_l... base/metrics/feature_list.h:23: // The name of the feagture. On 2015/08/30 19:36:22, rkaplow wrote: > feature Done. https://codereview.chromium.org/1278403003/diff/420001/base/metrics/feature_l... base/metrics/feature_list.h:43: // if (base::FeatureList::IsEnabled(kMyGreatFeature)) { On 2015/08/30 19:36:22, rkaplow wrote: > super minor nit but for even for sample syntax I would have a close brace Done. https://codereview.chromium.org/1278403003/diff/420001/base/metrics/feature_l... base/metrics/feature_list.h:46: // flags to enable or disable the feature, any experiments that may control it On 2015/08/30 19:36:22, rkaplow wrote: > I would make it clear here the ordering of priority - which I assume is command > line, then experiment, then default. Done. https://codereview.chromium.org/1278403003/diff/420001/base/metrics/feature_l... base/metrics/feature_list.h:78: bool IsFeatureEnabled(const Feature& feature); On 2015/08/30 19:36:22, rkaplow wrote: > can this be private? I don't think there's a problem with it being public - it doesn't expose anything dangerous. Making it private would having a friend declaration for InitializeFromCommandLine test and several other tests I'll be adding in a follow-up CL, so I think it's better to keep it public.
https://codereview.chromium.org/1278403003/diff/440001/base/metrics/feature_l... File base/metrics/feature_list.h (right): https://codereview.chromium.org/1278403003/diff/440001/base/metrics/feature_l... base/metrics/feature_list.h:5: #ifndef BASE_METRICS_FEATURE_LIST_H_ I think this would actually be better in base/feature_list.h. I think of this as an extension or a peer to the command-line feature switching that happens to integrate with the metrics system, not a component of the metrics system. It also seems like it's reasonable to have "Features" with no Finch flags. https://codereview.chromium.org/1278403003/diff/440001/base/metrics/feature_l... base/metrics/feature_list.h:21: // for a given feature name - generally defined as a global variable. maybe append "or file static" after global? I don't want people to feel like they have to make it globally visible. https://codereview.chromium.org/1278403003/diff/440001/base/metrics/feature_l... base/metrics/feature_list.h:23: // The name of the feature. It would be nice to mention here what this is used for and what it needs to correspond to (finch flags, etc.). https://codereview.chromium.org/1278403003/diff/440001/base/metrics/feature_l... base/metrics/feature_list.h:53: // FeatureList API is thread safe. It would be nice to give an example here of how to control a feature (and also more than one) from the command line. Also, how to control in Finch (this part might be a link to somewhere else, dunnow). https://codereview.chromium.org/1278403003/diff/440001/base/metrics/feature_l... base/metrics/feature_list.h:116: // TODO(asvitkine): Expand this as more support is added. I'm curious, what extra stuff are you thinking of putting here?
https://codereview.chromium.org/1278403003/diff/440001/base/metrics/feature_l... File base/metrics/feature_list.h (right): https://codereview.chromium.org/1278403003/diff/440001/base/metrics/feature_l... base/metrics/feature_list.h:5: #ifndef BASE_METRICS_FEATURE_LIST_H_ On 2015/08/31 21:26:00, brettw wrote: > I think this would actually be better in base/feature_list.h. I think of this as > an extension or a peer to the command-line feature switching that happens to > integrate with the metrics system, not a component of the metrics system. It > also seems like it's reasonable to have "Features" with no Finch flags. Fair enough. I guess one reason I wanted it here is so I could be in OWNERS of this code (since I'm in base/metrics OWNERS), but I suppose that can be done with per-file owners declarations. Will do this in the next patch set tomorrow. https://codereview.chromium.org/1278403003/diff/440001/base/metrics/feature_l... base/metrics/feature_list.h:21: // for a given feature name - generally defined as a global variable. On 2015/08/31 21:26:00, brettw wrote: > maybe append "or file static" after global? I don't want people to feel like > they have to make it globally visible. Done. https://codereview.chromium.org/1278403003/diff/440001/base/metrics/feature_l... base/metrics/feature_list.h:23: // The name of the feature. On 2015/08/31 21:26:00, brettw wrote: > It would be nice to mention here what this is used for and what it needs to > correspond to (finch flags, etc.). Done. https://codereview.chromium.org/1278403003/diff/440001/base/metrics/feature_l... base/metrics/feature_list.h:53: // FeatureList API is thread safe. On 2015/08/31 21:26:00, brettw wrote: > It would be nice to give an example here of how to control a feature (and also > more than one) from the command line. Also, how to control in Finch (this part > might be a link to somewhere else, dunnow). Added command-line examples. I think the experimentation stuff can probably documented elsewhere. https://codereview.chromium.org/1278403003/diff/440001/base/metrics/feature_l... base/metrics/feature_list.h:116: // TODO(asvitkine): Expand this as more support is added. On 2015/08/31 21:26:00, brettw wrote: > I'm curious, what extra stuff are you thinking of putting here? It will come in future CLs, but mainly extra bookkeeping for association with field trials: - FieldTrial* pointer to an associated field trial that gets activated when state of a feature is queried. - Some state for features that will want to opt-in into runtime killable behavior - mainly callback and a lock (only used for killable features). The specifics can be reviewed in follow-up CLs that add them. The normal field trial integration will come next and kill switch stuff will come last (probably after some other parts are in outside of base/ for FieldTrial stuff - i.e. in VariationsService).
https://codereview.chromium.org/1278403003/diff/420001/base/metrics/feature_l... File base/metrics/feature_list.h (right): https://codereview.chromium.org/1278403003/diff/420001/base/metrics/feature_l... base/metrics/feature_list.h:27: const bool default_state; Let's use an enum rather than a bool. "default_state := false" is not as clear as "default_state := DISABLED_BY_DEFAULT" (or perhaps "DEFAULT_DISABLED"). https://codereview.chromium.org/1278403003/diff/420001/base/metrics/feature_l... base/metrics/feature_list.h:98: FRIEND_TEST_ALL_PREFIXES(FeatureListTest, CheckFeatureIdentity); Please use a mechanism that exposes a more narrow set of internals, e.g. using protected methods and a subclass for testing. https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... File base/metrics/feature_list.cc (right): https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list.cc:34: const std::string& disable_features) { Currently, there is no guard preventing this method from being called twice during initialization. Should there be? https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list.cc:58: auto entry = &*it->second; nit: Boy, this line makes me do a double-take. I think something like "const OverrideEntry& entry = *it->second;" would be a little clearer. https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list.cc:68: DCHECK(GetInstance()); nit: This line seems a little silly, since the following line would crash if the instance were null. https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... File base/metrics/feature_list.h (right): https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list.h:21: // for a given feature name - generally defined as a global variable or a file nit: Might be worth emphasizing that the global variable would be a constant. https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list.h:29: const bool default_state; Let's use an enum rather than a bool. "default_state := false" is not as clear as "default_state := DISABLED_BY_DEFAULT" (or perhaps "DEFAULT_DISABLED"). https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list.h:54: // Features can be explicitly forced on or off by specifying a list of comma nit: s/comma/comma- https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list.h:60: // After initial initialization (which should be done single-threaded), the nit: "initial initialization" reads a little weirdly. I'd drop the word "initial". https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list.h:75: // it will be disabled. Must only be invoked during initialization phase nit: "during initialization phase" -> "during the initialization phase" https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list.h:83: void FinalizeInitialization(); nit: Can this method be private? https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list.h:85: // Returns whether the given |feature| is enabled. This is member function is nit: "This is" -> "This" https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list.h:89: bool IsFeatureEnabled(const Feature& feature); I agree with Rob that it's probably sensible to make this private, and to have tests use the same public API that production code would use. https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list.h:94: // struct, which is checked for in debug builds. nit: Below, you write this as "builds with DCHECKs enabled". Probably worth being consistent. https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list.h:109: FRIEND_TEST_ALL_PREFIXES(FeatureListTest, CheckFeatureIdentity); Please use a mechanism that exposes a more narrow set of internals, e.g. using protected methods and a subclass for testing. https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list.h:116: // Checks whether |feature| has the same address as any previously seen This comment is a little bit confusing/misleading. Suppose that I call CheckFeatureIdentity(A), CheckFeatureIdentity(B), CheckFeatureIdentity(B). As written, the comment suggests that the second call and the third call would return different results. https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list.h:117: // Feature structs with the same name. Used only from DCHECKs and tests. The semantics of the return value are probably worth clarifying. From reading this description, I would have guessed that the method returns |true| if the |feature| has been seen before, and |false| otherwise. But it also returns |true| if the |feature|'s name has never been seen before. I think it might be more helpful to phrase this method's description as an explanation of what sort of bug it's trying to catch, rather than an idea of what the implementation is. https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list.h:122: bool overriden_state; nit: "riden" -> "ridden" (applies throughout) https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list.h:122: bool overriden_state; nit: I think it would be much nicer, for readability, to have an enum consisting of ENABLED and DISABLED rather than just a raw bool. https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list.h:130: // to hold additional non-copyable fields. nit: I would personally prefer to use a regular map for this CL, and upgrade it to a ScopedPtrMap when needed. https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... File base/metrics/feature_list_unittest.cc (right): https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list_unittest.cc:13: const char kFeatureOnName[] = "Example1"; I think the tests would be easier to follow if this line were something like const char kFeatureOnByDefaultName[] = "Example_OnByDefault"; and the other lines were changed similarly. https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list_unittest.cc:23: class FeatureListTest : public testing::Test { nit: The test harness class should be declared outside the anonymous namespace, so that the ODR is less likely to be violated by tests. https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list_unittest.cc:32: } This doesn't make much sense as a public method if the caller can't control the value of |feature_list_|. Perhaps this method should accept a pointer to the desired feature list? https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list_unittest.cc:35: FeatureList* feature_list() { return feature_list_; } This can return a pointer to freed memory if ClearFeatureListInstance() has been called. Probably that method should set |feature_list_| to nullptr. https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list_unittest.cc:67: for (size_t i = 0; i < arraysize(test_cases); i++) { nit: ++i https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list_unittest.cc:73: feature_list.FinalizeInitialization(); It seems more appropriate to set the singleton, and later to call FeatureList::ClearInstanceForTesting(). That more closely mirrors what actual client code would do (except for the resetting part, of course). https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list_unittest.cc:84: TEST_F(FeatureListTest, CheckFeatureIdentity) { This purpose of this test is not immediately obvious. It might be worth writing a brief comment explaining what it's doing. (Better yet would be to test the public API rather than the private one, if possible.) https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list_unittest.cc:88: EXPECT_TRUE(feature_list()->CheckFeatureIdentity(kExampleFeatureOff)); Why are these lines written twice each?
Thanks! Ilya's comments addressed, PTAL. Have not yet moved the code from base/metrics to base/, will do that next (though have a lot of meetings today, so might not get to it until tomorrow.) https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... File base/metrics/feature_list.cc (right): https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list.cc:34: const std::string& disable_features) { On 2015/09/01 03:55:25, Ilya Sherman wrote: > Currently, there is no guard preventing this method from being called twice > during initialization. Should there be? I don't think it's really worth guarding against - since that will only be called during initialization anyway in early start up (and it's already checked that it's done during initialization). So I don't think there's any opportunity for abuse, etc. But if you prefer we check that, it can be done by switching the |initialized_| bool to an enum with different states. Let me know what you think - I've left it as-is for now. https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list.cc:58: auto entry = &*it->second; On 2015/09/01 03:55:25, Ilya Sherman wrote: > nit: Boy, this line makes me do a double-take. I think something like "const > OverrideEntry& entry = *it->second;" would be a little clearer. Done. In the future it won't be able to be a const (i.e. when there's a lock for killable features), but we can keep it like you suggest for now. https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list.cc:68: DCHECK(GetInstance()); On 2015/09/01 03:55:25, Ilya Sherman wrote: > nit: This line seems a little silly, since the following line would crash if the > instance were null. Done. https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... File base/metrics/feature_list.h (right): https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list.h:21: // for a given feature name - generally defined as a global variable or a file On 2015/09/01 03:55:26, Ilya Sherman wrote: > nit: Might be worth emphasizing that the global variable would be a constant. Done. https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list.h:29: const bool default_state; On 2015/09/01 03:55:26, Ilya Sherman wrote: > Let's use an enum rather than a bool. "default_state := false" is not as clear > as "default_state := DISABLED_BY_DEFAULT" (or perhaps "DEFAULT_DISABLED"). Done. https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list.h:54: // Features can be explicitly forced on or off by specifying a list of comma On 2015/09/01 03:55:25, Ilya Sherman wrote: > nit: s/comma/comma- Done. https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list.h:60: // After initial initialization (which should be done single-threaded), the On 2015/09/01 03:55:25, Ilya Sherman wrote: > nit: "initial initialization" reads a little weirdly. I'd drop the word > "initial". Done. https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list.h:75: // it will be disabled. Must only be invoked during initialization phase On 2015/09/01 03:55:25, Ilya Sherman wrote: > nit: "during initialization phase" -> "during the initialization phase" Done. https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list.h:83: void FinalizeInitialization(); On 2015/09/01 03:55:25, Ilya Sherman wrote: > nit: Can this method be private? Done. https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list.h:85: // Returns whether the given |feature| is enabled. This is member function is On 2015/09/01 03:55:26, Ilya Sherman wrote: > nit: "This is" -> "This" Done. https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list.h:89: bool IsFeatureEnabled(const Feature& feature); On 2015/09/01 03:55:25, Ilya Sherman wrote: > I agree with Rob that it's probably sensible to make this private, and to have > tests use the same public API that production code would use. Done. See my other comment for my original reasoning for this. https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list.h:94: // struct, which is checked for in debug builds. On 2015/09/01 03:55:26, Ilya Sherman wrote: > nit: Below, you write this as "builds with DCHECKs enabled". Probably worth > being consistent. Done. https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list.h:109: FRIEND_TEST_ALL_PREFIXES(FeatureListTest, CheckFeatureIdentity); On 2015/09/01 03:55:25, Ilya Sherman wrote: > Please use a mechanism that exposes a more narrow set of internals, e.g. using > protected methods and a subclass for testing. It seems like that would result in more boilerplate. What's the benefit, exactly? Are you trying to guard against someone changing the test to reach into more parts of the class? Seems unlikely to me, since the test is pretty short and specific. https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list.h:116: // Checks whether |feature| has the same address as any previously seen On 2015/09/01 03:55:26, Ilya Sherman wrote: > This comment is a little bit confusing/misleading. Suppose that I call > CheckFeatureIdentity(A), CheckFeatureIdentity(B), CheckFeatureIdentity(B). As > written, the comment suggests that the second call and the third call would > return different results. Done. https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list.h:117: // Feature structs with the same name. Used only from DCHECKs and tests. On 2015/09/01 03:55:25, Ilya Sherman wrote: > The semantics of the return value are probably worth clarifying. From reading > this description, I would have guessed that the method returns |true| if the > |feature| has been seen before, and |false| otherwise. But it also returns > |true| if the |feature|'s name has never been seen before. I think it might be > more helpful to phrase this method's description as an explanation of what sort > of bug it's trying to catch, rather than an idea of what the implementation is. Done. https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list.h:122: bool overriden_state; On 2015/09/01 03:55:26, Ilya Sherman wrote: > nit: "riden" -> "ridden" (applies throughout) Done. https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list.h:122: bool overriden_state; On 2015/09/01 03:55:26, Ilya Sherman wrote: > nit: I think it would be much nicer, for readability, to have an enum consisting > of ENABLED and DISABLED rather than just a raw bool. Done. https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list.h:130: // to hold additional non-copyable fields. On 2015/09/01 03:55:25, Ilya Sherman wrote: > nit: I would personally prefer to use a regular map for this CL, and upgrade it > to a ScopedPtrMap when needed. Done. https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... File base/metrics/feature_list_unittest.cc (right): https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list_unittest.cc:13: const char kFeatureOnName[] = "Example1"; On 2015/09/01 03:55:26, Ilya Sherman wrote: > I think the tests would be easier to follow if this line were something like > > const char kFeatureOnByDefaultName[] = "Example_OnByDefault"; > > and the other lines were changed similarly. Done. https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list_unittest.cc:23: class FeatureListTest : public testing::Test { On 2015/09/01 03:55:26, Ilya Sherman wrote: > nit: The test harness class should be declared outside the anonymous namespace, > so that the ODR is less likely to be violated by tests. Done. https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list_unittest.cc:32: } On 2015/09/01 03:55:26, Ilya Sherman wrote: > This doesn't make much sense as a public method if the caller can't control the > value of |feature_list_|. Perhaps this method should accept a pointer to the > desired feature list? Done. https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list_unittest.cc:35: FeatureList* feature_list() { return feature_list_; } On 2015/09/01 03:55:26, Ilya Sherman wrote: > This can return a pointer to freed memory if ClearFeatureListInstance() has been > called. Probably that method should set |feature_list_| to nullptr. Done. https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list_unittest.cc:67: for (size_t i = 0; i < arraysize(test_cases); i++) { On 2015/09/01 03:55:26, Ilya Sherman wrote: > nit: ++i Done. https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list_unittest.cc:73: feature_list.FinalizeInitialization(); On 2015/09/01 03:55:26, Ilya Sherman wrote: > It seems more appropriate to set the singleton, and later to call > FeatureList::ClearInstanceForTesting(). That more closely mirrors what actual > client code would do (except for the resetting part, of course). Done. I was originally thinking to keep the class flexible - such that if someone wants to use this as a free-standing instance outside of the singleton, the class lets you do this. But I actually don't have a reason in mind why someone would want to do this, so restricted things as you suggested. https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list_unittest.cc:84: TEST_F(FeatureListTest, CheckFeatureIdentity) { On 2015/09/01 03:55:26, Ilya Sherman wrote: > This purpose of this test is not immediately obvious. It might be worth writing > a brief comment explaining what it's doing. (Better yet would be to test the > public API rather than the private one, if possible.) I don't think a test using the public API is possible. Such a test would need to somehow test that a DCHECK was hit - and I don't think there's a mechanism for this. (Let me know if one exists and I'll gladly change this.) Added a comment per your other suggestion. https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list_unittest.cc:88: EXPECT_TRUE(feature_list()->CheckFeatureIdentity(kExampleFeatureOff)); On 2015/09/01 03:55:26, Ilya Sherman wrote: > Why are these lines written twice each? The first time would register it and the second time would check against the registered entry. Clarified in the comment.
Now moved to base/ from base/metrics/
Thanks, Alexei! Pretty much LGTM % some final nits. https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... File base/metrics/feature_list.h (right): https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list.h:109: FRIEND_TEST_ALL_PREFIXES(FeatureListTest, CheckFeatureIdentity); On 2015/09/01 15:53:44, Alexei Svitkine wrote: > On 2015/09/01 03:55:25, Ilya Sherman wrote: > > Please use a mechanism that exposes a more narrow set of internals, e.g. using > > protected methods and a subclass for testing. > > It seems like that would result in more boilerplate. What's the benefit, > exactly? Are you trying to guard against someone changing the test to reach into > more parts of the class? Seems unlikely to me, since the test is pretty short > and specific. I think it's generally nice to clearly distinguish the internals that tests can muck vs. ones they can't. I agree that for the current test in its current state, there's not much to worry about. My concern is mostly that once a file starts to use FRIEND_TEST_ALL_PREFIXES, subsequent tests tend to also start using it, which leads to a bad state. As long as it's just this one test -- which is itself testing an internal method -- that we use the macro for, then I'm ok with it. I'm just worried about setting a misleading precedent. https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... File base/metrics/feature_list_unittest.cc (right): https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list_unittest.cc:84: TEST_F(FeatureListTest, CheckFeatureIdentity) { On 2015/09/01 15:53:44, Alexei Svitkine wrote: > On 2015/09/01 03:55:26, Ilya Sherman wrote: > > This purpose of this test is not immediately obvious. It might be worth > writing > > a brief comment explaining what it's doing. (Better yet would be to test the > > public API rather than the private one, if possible.) > > I don't think a test using the public API is possible. Such a test would need to > somehow test that a DCHECK was hit - and I don't think there's a mechanism for > this. (Let me know if one exists and I'll gladly change this.) > > Added a comment per your other suggestion. I was thinking we could use a gtest Death Test: [ https://code.google.com/p/googletest/wiki/AdvancedGuide#Death_Tests ]. (Caveat: I haven't tried this recently, and remember it being somewhat finicky.) https://codereview.chromium.org/1278403003/diff/480001/base/metrics/feature_l... File base/metrics/feature_list.h (right): https://codereview.chromium.org/1278403003/diff/480001/base/metrics/feature_l... base/metrics/feature_list.h:20: enum FeatureState { Optional nit: I tend to prefer enum class, because I think FeatureState::DISABLED is a little bit more readable than FEATURE_DISABLED. https://codereview.chromium.org/1278403003/diff/480001/base/metrics/feature_l... base/metrics/feature_list.h:35: const FeatureState default_state; Optional nit: I think it would be clearer to have a separate enum for the default state, which uses the word "DEFAULT" somewhere in it. That is, I think struct base::Feature kMyGreatFeature { "MyGreatFeature", base::FEATURE_ENABLED_BY_DEFAULT }; makes more sense to pass into a function named base::FeatureList::IsEnabled() than struct base::Feature kMyGreatFeature { "MyGreatFeature", base::FEATURE_ENABLED }; https://codereview.chromium.org/1278403003/diff/480001/base/metrics/feature_l... base/metrics/feature_list.h:108: // gets called when it is registered via SetInstance(). nit: Please update this comment. https://codereview.chromium.org/1278403003/diff/480001/base/metrics/feature_l... base/metrics/feature_list.h:114: // FeatureList that has been initialized (FinalizeInitialization() called). nit: Please update this comment.
https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... File base/metrics/feature_list_unittest.cc (right): https://codereview.chromium.org/1278403003/diff/460001/base/metrics/feature_l... base/metrics/feature_list_unittest.cc:84: TEST_F(FeatureListTest, CheckFeatureIdentity) { On 2015/09/01 20:17:17, Ilya Sherman wrote: > On 2015/09/01 15:53:44, Alexei Svitkine wrote: > > On 2015/09/01 03:55:26, Ilya Sherman wrote: > > > This purpose of this test is not immediately obvious. It might be worth > > writing > > > a brief comment explaining what it's doing. (Better yet would be to test > the > > > public API rather than the private one, if possible.) > > > > I don't think a test using the public API is possible. Such a test would need > to > > somehow test that a DCHECK was hit - and I don't think there's a mechanism for > > this. (Let me know if one exists and I'll gladly change this.) > > > > Added a comment per your other suggestion. > > I was thinking we could use a gtest Death Test: [ > https://code.google.com/p/googletest/wiki/AdvancedGuide#Death_Tests ]. (Caveat: > I haven't tried this recently, and remember it being somewhat finicky.) Right, then the test would have to only be enabled if DCHECKs are on. I suppose it's possible, but I feel like it would be a bit convoluted so, would rather leave as-is. https://codereview.chromium.org/1278403003/diff/480001/base/metrics/feature_l... File base/metrics/feature_list.h (right): https://codereview.chromium.org/1278403003/diff/480001/base/metrics/feature_l... base/metrics/feature_list.h:20: enum FeatureState { On 2015/09/01 20:17:17, Ilya Sherman wrote: > Optional nit: I tend to prefer enum class, because I think > FeatureState::DISABLED is a little bit more readable than FEATURE_DISABLED. Started the discussion on this on your other comment. https://codereview.chromium.org/1278403003/diff/480001/base/metrics/feature_l... base/metrics/feature_list.h:35: const FeatureState default_state; On 2015/09/01 20:17:17, Ilya Sherman wrote: > Optional nit: I think it would be clearer to have a separate enum for the > default state, which uses the word "DEFAULT" somewhere in it. That is, I think > > struct base::Feature kMyGreatFeature { > "MyGreatFeature", base::FEATURE_ENABLED_BY_DEFAULT > }; > > makes more sense to pass into a function named base::FeatureList::IsEnabled() > than > > struct base::Feature kMyGreatFeature { > "MyGreatFeature", base::FEATURE_ENABLED > }; Hmm, so if combining with your other comment above, it would look like: struct base::Feature kMyGreatFeature { "MyGreatFeature", base::DefaultFeatureState::ENABLED_BY_DEFAULT }; I guess it feels a bit verbose to me. brettw@ / rkaplow@, any opinions on this? https://codereview.chromium.org/1278403003/diff/480001/base/metrics/feature_l... base/metrics/feature_list.h:108: // gets called when it is registered via SetInstance(). On 2015/09/01 20:17:17, Ilya Sherman wrote: > nit: Please update this comment. Done. https://codereview.chromium.org/1278403003/diff/480001/base/metrics/feature_l... base/metrics/feature_list.h:114: // FeatureList that has been initialized (FinalizeInitialization() called). On 2015/09/01 20:17:17, Ilya Sherman wrote: > nit: Please update this comment. Done.
https://codereview.chromium.org/1278403003/diff/480001/base/metrics/feature_l... File base/metrics/feature_list.h (right): https://codereview.chromium.org/1278403003/diff/480001/base/metrics/feature_l... base/metrics/feature_list.h:35: const FeatureState default_state; On 2015/09/01 21:18:39, Alexei Svitkine wrote: > On 2015/09/01 20:17:17, Ilya Sherman wrote: > > Optional nit: I think it would be clearer to have a separate enum for the > > default state, which uses the word "DEFAULT" somewhere in it. That is, I > think > > > > struct base::Feature kMyGreatFeature { > > "MyGreatFeature", base::FEATURE_ENABLED_BY_DEFAULT > > }; > > > > makes more sense to pass into a function named base::FeatureList::IsEnabled() > > than > > > > struct base::Feature kMyGreatFeature { > > "MyGreatFeature", base::FEATURE_ENABLED > > }; > > Hmm, so if combining with your other comment above, it would look like: > > struct base::Feature kMyGreatFeature { > "MyGreatFeature", base::DefaultFeatureState::ENABLED_BY_DEFAULT > }; > > I guess it feels a bit verbose to me. > > brettw@ / rkaplow@, any opinions on this? Well, if the enum is named DefaultFeatureState, then the constant could be just "ENABLED".
https://codereview.chromium.org/1278403003/diff/480001/base/metrics/feature_l... File base/metrics/feature_list.h (right): https://codereview.chromium.org/1278403003/diff/480001/base/metrics/feature_l... base/metrics/feature_list.h:35: const FeatureState default_state; On 2015/09/01 21:20:37, Ilya Sherman wrote: > On 2015/09/01 21:18:39, Alexei Svitkine wrote: > > On 2015/09/01 20:17:17, Ilya Sherman wrote: > > > Optional nit: I think it would be clearer to have a separate enum for the > > > default state, which uses the word "DEFAULT" somewhere in it. That is, I > > think > > > > > > struct base::Feature kMyGreatFeature { > > > "MyGreatFeature", base::FEATURE_ENABLED_BY_DEFAULT > > > }; > > > > > > makes more sense to pass into a function named > base::FeatureList::IsEnabled() > > > than > > > > > > struct base::Feature kMyGreatFeature { > > > "MyGreatFeature", base::FEATURE_ENABLED > > > }; > > > > Hmm, so if combining with your other comment above, it would look like: > > > > struct base::Feature kMyGreatFeature { > > "MyGreatFeature", base::DefaultFeatureState::ENABLED_BY_DEFAULT > > }; > > > > I guess it feels a bit verbose to me. > > > > brettw@ / rkaplow@, any opinions on this? > > Well, if the enum is named DefaultFeatureState, then the constant could be just > "ENABLED". True. It won't be as obvious as ENABLED_BY_DEFAULT - since people might skim the prefix, but might be an ok compromise. Would still like to hear what others think on this front, though - in terms of how verbose or not we should make this.
On 2015/09/01 21:36:00, Alexei Svitkine wrote: > https://codereview.chromium.org/1278403003/diff/480001/base/metrics/feature_l... > File base/metrics/feature_list.h (right): > > https://codereview.chromium.org/1278403003/diff/480001/base/metrics/feature_l... > base/metrics/feature_list.h:35: const FeatureState default_state; > On 2015/09/01 21:20:37, Ilya Sherman wrote: > > On 2015/09/01 21:18:39, Alexei Svitkine wrote: > > > On 2015/09/01 20:17:17, Ilya Sherman wrote: > > > > Optional nit: I think it would be clearer to have a separate enum for the > > > > default state, which uses the word "DEFAULT" somewhere in it. That is, I > > > think > > > > > > > > struct base::Feature kMyGreatFeature { > > > > "MyGreatFeature", base::FEATURE_ENABLED_BY_DEFAULT > > > > }; > > > > > > > > makes more sense to pass into a function named > > base::FeatureList::IsEnabled() > > > > than > > > > > > > > struct base::Feature kMyGreatFeature { > > > > "MyGreatFeature", base::FEATURE_ENABLED > > > > }; > > > > > > Hmm, so if combining with your other comment above, it would look like: > > > > > > struct base::Feature kMyGreatFeature { > > > "MyGreatFeature", base::DefaultFeatureState::ENABLED_BY_DEFAULT > > > }; > > > > > > I guess it feels a bit verbose to me. > > > > > > brettw@ / rkaplow@, any opinions on this? > > > > Well, if the enum is named DefaultFeatureState, then the constant could be > just > > "ENABLED". > > True. It won't be as obvious as ENABLED_BY_DEFAULT - since people might skim the > prefix, but might be an ok compromise. Would still like to hear what others > think on this front, though - in terms of how verbose or not we should make > this. No strong opinion. I'd probably go for the ENABLED_BY_DEFAULT, but I always am on the verbose side of this type of discussion. Either seems fine to me.
lgtm
Patchset #8 (id:560001) has been deleted
Thanks, PTAL! https://codereview.chromium.org/1278403003/diff/480001/base/metrics/feature_l... File base/metrics/feature_list.h (right): https://codereview.chromium.org/1278403003/diff/480001/base/metrics/feature_l... base/metrics/feature_list.h:35: const FeatureState default_state; On 2015/09/01 21:36:00, Alexei Svitkine wrote: > On 2015/09/01 21:20:37, Ilya Sherman wrote: > > On 2015/09/01 21:18:39, Alexei Svitkine wrote: > > > On 2015/09/01 20:17:17, Ilya Sherman wrote: > > > > Optional nit: I think it would be clearer to have a separate enum for the > > > > default state, which uses the word "DEFAULT" somewhere in it. That is, I > > > think > > > > > > > > struct base::Feature kMyGreatFeature { > > > > "MyGreatFeature", base::FEATURE_ENABLED_BY_DEFAULT > > > > }; > > > > > > > > makes more sense to pass into a function named > > base::FeatureList::IsEnabled() > > > > than > > > > > > > > struct base::Feature kMyGreatFeature { > > > > "MyGreatFeature", base::FEATURE_ENABLED > > > > }; > > > > > > Hmm, so if combining with your other comment above, it would look like: > > > > > > struct base::Feature kMyGreatFeature { > > > "MyGreatFeature", base::DefaultFeatureState::ENABLED_BY_DEFAULT > > > }; > > > > > > I guess it feels a bit verbose to me. > > > > > > brettw@ / rkaplow@, any opinions on this? > > > > Well, if the enum is named DefaultFeatureState, then the constant could be > just > > "ENABLED". > > True. It won't be as obvious as ENABLED_BY_DEFAULT - since people might skim the > prefix, but might be an ok compromise. Would still like to hear what others > think on this front, though - in terms of how verbose or not we should make > this. Thinking about this more, I liked the verbosity / obviousness of "ENABLED_BY_DEFAULT", but didn't like the extra verbosity of the base::DefaultFeatureState prefix if we used an enum class (since I think most people would just skim it, so it's extra noise). So I went with base::FEATURE_ENABLED_BY_DEFAULT and base::FEATURE_DISABLED_BY_DEFAULT and also updated the enum on the OverrideEntry to be a separate enum (since that doesn't specify default behavior).
Still LGTM -- thanks =)
BrettW: friendly ping On Wed, Sep 2, 2015 at 7:37 PM, <isherman@chromium.org> wrote: > Still LGTM -- thanks =) > > https://codereview.chromium.org/1278403003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Whoops, sorry. LGTM
The CQ bit was checked by asvitkine@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rkaplow@chromium.org Link to the patchset: https://codereview.chromium.org/1278403003/#ps580001 (title: "Update enums and owners file.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1278403003/580001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1278403003/580001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
The CQ bit was checked by asvitkine@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1278403003/580001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1278403003/580001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
The CQ bit was checked by asvitkine@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rkaplow@chromium.org, brettw@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1278403003/#ps600001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1278403003/600001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1278403003/600001
Message was sent while issue was closed.
Committed patchset #9 (id:600001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/bccbb867acc61bb9b8c88e3a7b0cbee6b08df0de Cr-Commit-Position: refs/heads/master@{#347438}
Message was sent while issue was closed.
Patchset #10 (id:620001) has been deleted |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
