Initial 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
Review URL: https://codereview.chromium.org/1278403003
Cr-Commit-Position: refs/heads/master@{#347438}
diff --git a/base/BUILD.gn b/base/BUILD.gn
index 74b71e0..aa9261d 100644
--- a/base/BUILD.gn
+++ b/base/BUILD.gn
@@ -186,6 +186,8 @@
"deferred_sequenced_task_runner.h",
"environment.cc",
"environment.h",
+ "feature_list.cc",
+ "feature_list.h",
"file_descriptor_posix.h",
"file_version_info.h",
"file_version_info_mac.h",
diff --git a/base/OWNERS b/base/OWNERS
index bcaa81b..932fa89 100644
--- a/base/OWNERS
+++ b/base/OWNERS
@@ -26,3 +26,7 @@
per-file *android*=nyquist@chromium.org
per-file *android*=rmcilroy@chromium.org
per-file *android*=yfriedman@chromium.org
+
+# For FeatureList API:
+per-file feature_list*=asvitkine@chromium.org
+per-file feature_list*=isherman@chromium.org
diff --git a/base/base.gyp b/base/base.gyp
index 5264f2a..db52218 100644
--- a/base/base.gyp
+++ b/base/base.gyp
@@ -466,6 +466,7 @@
'debug/task_annotator_unittest.cc',
'deferred_sequenced_task_runner_unittest.cc',
'environment_unittest.cc',
+ 'feature_list_unittest.cc',
'file_version_info_unittest.cc',
'files/dir_reader_posix_unittest.cc',
'files/file_path_unittest.cc',
diff --git a/base/base.gypi b/base/base.gypi
index 39727098..ce980ed 100644
--- a/base/base.gypi
+++ b/base/base.gypi
@@ -177,6 +177,8 @@
'deferred_sequenced_task_runner.h',
'environment.cc',
'environment.h',
+ 'feature_list.cc',
+ 'feature_list.h',
'file_descriptor_posix.h',
'file_version_info.h',
'file_version_info_mac.h',
diff --git a/base/feature_list.cc b/base/feature_list.cc
new file mode 100644
index 0000000..16eba67
--- /dev/null
+++ b/base/feature_list.cc
@@ -0,0 +1,113 @@
+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/feature_list.h"
+
+#include <vector>
+
+#include "base/logging.h"
+#include "base/strings/string_split.h"
+
+namespace base {
+
+namespace {
+
+// Pointer to the FeatureList instance singleton that was set via
+// FeatureList::SetInstance(). Does not use base/memory/singleton.h in order to
+// have more control over initialization timing. Leaky.
+FeatureList* g_instance = nullptr;
+
+// Splits a comma-separated string containing feature names into a vector.
+std::vector<std::string> SplitFeatureListString(const std::string& input) {
+ return SplitString(input, ",", TRIM_WHITESPACE, SPLIT_WANT_NONEMPTY);
+}
+
+} // namespace
+
+FeatureList::FeatureList() : initialized_(false) {}
+
+FeatureList::~FeatureList() {}
+
+void FeatureList::InitializeFromCommandLine(
+ const std::string& enable_features,
+ const std::string& disable_features) {
+ DCHECK(!initialized_);
+
+ // Process disabled features first, so that disabled ones take precedence over
+ // enabled ones (since RegisterOverride() uses insert()).
+ for (const auto& feature_name : SplitFeatureListString(disable_features)) {
+ RegisterOverride(feature_name, OVERRIDE_DISABLE_FEATURE);
+ }
+ for (const auto& feature_name : SplitFeatureListString(enable_features)) {
+ RegisterOverride(feature_name, OVERRIDE_ENABLE_FEATURE);
+ }
+}
+
+// static
+bool FeatureList::IsEnabled(const Feature& feature) {
+ return GetInstance()->IsFeatureEnabled(feature);
+}
+
+// static
+FeatureList* FeatureList::GetInstance() {
+ return g_instance;
+}
+
+// static
+void FeatureList::SetInstance(scoped_ptr<FeatureList> instance) {
+ DCHECK(!g_instance);
+ instance->FinalizeInitialization();
+
+ // Note: Intentional leak of global singleton.
+ g_instance = instance.release();
+}
+
+// static
+void FeatureList::ClearInstanceForTesting() {
+ delete g_instance;
+ g_instance = nullptr;
+}
+
+void FeatureList::FinalizeInitialization() {
+ DCHECK(!initialized_);
+ initialized_ = true;
+}
+
+bool FeatureList::IsFeatureEnabled(const Feature& feature) {
+ DCHECK(initialized_);
+ DCHECK(CheckFeatureIdentity(feature)) << feature.name;
+
+ auto it = overrides_.find(feature.name);
+ if (it != overrides_.end()) {
+ const OverrideEntry& entry = it->second;
+ // TODO(asvitkine) Expand this section as more support is added.
+ return entry.overridden_state == OVERRIDE_ENABLE_FEATURE;
+ }
+ // Otherwise, return the default state.
+ return feature.default_state == FEATURE_ENABLED_BY_DEFAULT;
+}
+
+void FeatureList::RegisterOverride(const std::string& feature_name,
+ OverrideState overridden_state) {
+ DCHECK(!initialized_);
+ overrides_.insert(make_pair(feature_name, OverrideEntry(overridden_state)));
+}
+
+bool FeatureList::CheckFeatureIdentity(const Feature& feature) {
+ AutoLock auto_lock(feature_identity_tracker_lock_);
+
+ auto it = feature_identity_tracker_.find(feature.name);
+ if (it == feature_identity_tracker_.end()) {
+ // If it's not tracked yet, register it.
+ feature_identity_tracker_[feature.name] = &feature;
+ return true;
+ }
+ // Compare address of |feature| to the existing tracked entry.
+ return it->second == &feature;
+}
+
+FeatureList::OverrideEntry::OverrideEntry(OverrideState overridden_state)
+ : overridden_state(overridden_state) {}
+
+} // namespace base
diff --git a/base/feature_list.h b/base/feature_list.h
new file mode 100644
index 0000000..6e4ad58
--- /dev/null
+++ b/base/feature_list.h
@@ -0,0 +1,162 @@
+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef BASE_FEATURE_LIST_H_
+#define BASE_FEATURE_LIST_H_
+
+#include <map>
+#include <string>
+
+#include "base/base_export.h"
+#include "base/basictypes.h"
+#include "base/gtest_prod_util.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/synchronization/lock.h"
+
+namespace base {
+
+// Specifies whether a given feature is enabled or disabled by default.
+enum FeatureState {
+ FEATURE_DISABLED_BY_DEFAULT,
+ FEATURE_ENABLED_BY_DEFAULT,
+};
+
+// The Feature struct is used to define the default state for a feature. See
+// comment below for more details. There must only ever be one struct instance
+// for a given feature name - generally defined as a constant global variable or
+// file static.
+struct BASE_EXPORT Feature {
+ // The name of the feature. This should be unique to each feature and is used
+ // for enabling/disabling features via command line flags and experiments.
+ const char* const name;
+
+ // The default state (i.e. enabled or disabled) for this feature.
+ const FeatureState default_state;
+};
+
+// The FeatureList class is used to determine whether a given feature is on or
+// off. It provides an authoritative answer, taking into account command-line
+// overrides and experimental control.
+//
+// The basic use case is for any feature that can be toggled (e.g. through
+// command-line or an experiment) to have a defined Feature struct, e.g.:
+//
+// struct base::Feature kMyGreatFeature {
+// "MyGreatFeature", base::FEATURE_ENABLED_BY_DEFAULT
+// };
+//
+// Then, client code that wishes to query the state of the feature would check:
+//
+// if (base::FeatureList::IsEnabled(kMyGreatFeature)) {
+// // Feature code goes here.
+// }
+//
+// Behind the scenes, the above call would take into account any command-line
+// flags to enable or disable the feature, any experiments that may control it
+// and finally its default state (in that order of priority), to determine
+// whether the feature is on.
+//
+// Features can be explicitly forced on or off by specifying a list of comma-
+// separated feature names via the following command-line flags:
+//
+// --enable-features=Feature5,Feature7
+// --disable-features=Feature1,Feature2,Feature3
+//
+// After initialization (which should be done single-threaded), the FeatureList
+// API is thread safe.
+//
+// Note: This class is a singleton, but does not use base/memory/singleton.h in
+// order to have control over its initialization sequence. Specifically, the
+// intended use is to create an instance of this class and fully initialize it,
+// before setting it as the singleton for a process, via SetInstance().
+class BASE_EXPORT FeatureList {
+ public:
+ FeatureList();
+ ~FeatureList();
+
+ // Initializes feature overrides via command-line flags |enable_features| and
+ // |disable_features|, each of which is a comma-separated list of features to
+ // enable or disable, respectively. If a feature appears on both lists, then
+ // it will be disabled. Must only be invoked during the initialization phase
+ // (before FinalizeInitialization() has been called).
+ void InitializeFromCommandLine(const std::string& enable_features,
+ const std::string& disable_features);
+
+ // Returns whether the given |feature| is enabled. Must only be called after
+ // the singleton instance has been registered via SetInstance(). Additionally,
+ // a feature with a given name must only have a single corresponding Feature
+ // struct, which is checked in builds with DCHECKs enabled.
+ static bool IsEnabled(const Feature& feature);
+
+ // Returns the singleton instance of FeatureList. Will return null until an
+ // instance is registered via SetInstance().
+ static FeatureList* GetInstance();
+
+ // Registers the given |instance| to be the singleton feature list for this
+ // process. This should only be called once and |instance| must not be null.
+ static void SetInstance(scoped_ptr<FeatureList> instance);
+
+ // Clears the previously-registered singleton instance for tests.
+ static void ClearInstanceForTesting();
+
+ private:
+ FRIEND_TEST_ALL_PREFIXES(FeatureListTest, CheckFeatureIdentity);
+
+ // Specifies whether a feature override enables or disables the feature.
+ enum OverrideState {
+ OVERRIDE_DISABLE_FEATURE,
+ OVERRIDE_ENABLE_FEATURE,
+ };
+
+ // Finalizes the initialization state of the FeatureList, so that no further
+ // overrides can be registered. This is called by SetInstance() on the
+ // singleton feature list that is being registered.
+ void FinalizeInitialization();
+
+ // Returns whether the given |feature| is enabled. This is invoked by the
+ // public FeatureList::IsEnabled() static function on the global singleton.
+ // Requires the FeatureList to have already been fully initialized.
+ bool IsFeatureEnabled(const Feature& feature);
+
+ // Registers an override for feature |feature_name|. The override specifies
+ // whether the feature should be on or off (via |overridden_state|), which
+ // will take precedence over the feature's default state.
+ void RegisterOverride(const std::string& feature_name,
+ OverrideState overridden_state);
+
+ // Verifies that there's only a single definition of a Feature struct for a
+ // given feature name. Keeps track of the first seen Feature struct for each
+ // feature. Returns false when called on a Feature struct with a different
+ // address than the first one it saw for that feature name. Used only from
+ // DCHECKs and tests.
+ bool CheckFeatureIdentity(const Feature& feature);
+
+ struct OverrideEntry {
+ // The overridden enable (on/off) state of the feature.
+ const OverrideState overridden_state;
+
+ // TODO(asvitkine): Expand this as more support is added.
+
+ explicit OverrideEntry(OverrideState overridden_state);
+ };
+ // Map from feature name to an OverrideEntry struct for the feature, if it
+ // exists.
+ std::map<std::string, OverrideEntry> overrides_;
+
+ // Locked map that keeps track of seen features, to ensure a single feature is
+ // only defined once. This verification is only done in builds with DCHECKs
+ // enabled.
+ Lock feature_identity_tracker_lock_;
+ std::map<std::string, const Feature*> feature_identity_tracker_;
+
+ // Whether this object has been fully initialized. This gets set to true as a
+ // result of FinalizeInitialization().
+ bool initialized_;
+
+ DISALLOW_COPY_AND_ASSIGN(FeatureList);
+};
+
+} // namespace base
+
+#endif // BASE_FEATURE_LIST_H_
diff --git a/base/feature_list_unittest.cc b/base/feature_list_unittest.cc
new file mode 100644
index 0000000..c9423ce7
--- /dev/null
+++ b/base/feature_list_unittest.cc
@@ -0,0 +1,108 @@
+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/feature_list.h"
+
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace base {
+
+namespace {
+
+const char kFeatureOnByDefaultName[] = "OnByDefault";
+struct Feature kFeatureOnByDefault {
+ kFeatureOnByDefaultName, FEATURE_ENABLED_BY_DEFAULT
+};
+
+const char kFeatureOffByDefaultName[] = "OffByDefault";
+struct Feature kFeatureOffByDefault {
+ kFeatureOffByDefaultName, FEATURE_DISABLED_BY_DEFAULT
+};
+
+} // namespace
+
+class FeatureListTest : public testing::Test {
+ public:
+ FeatureListTest() : feature_list_(nullptr) {
+ RegisterFeatureListInstance(make_scoped_ptr(new FeatureList));
+ }
+ ~FeatureListTest() override { ClearFeatureListInstance(); }
+
+ void RegisterFeatureListInstance(scoped_ptr<FeatureList> feature_list) {
+ feature_list_ = feature_list.get();
+ FeatureList::SetInstance(feature_list.Pass());
+ }
+ void ClearFeatureListInstance() {
+ FeatureList::ClearInstanceForTesting();
+ feature_list_ = nullptr;
+ }
+
+ FeatureList* feature_list() { return feature_list_; }
+
+ private:
+ // Weak. Owned by the FeatureList::SetInstance().
+ FeatureList* feature_list_;
+
+ DISALLOW_COPY_AND_ASSIGN(FeatureListTest);
+};
+
+TEST_F(FeatureListTest, DefaultStates) {
+ EXPECT_TRUE(FeatureList::IsEnabled(kFeatureOnByDefault));
+ EXPECT_FALSE(FeatureList::IsEnabled(kFeatureOffByDefault));
+}
+
+TEST_F(FeatureListTest, InitializeFromCommandLine) {
+ struct {
+ const char* enable_features;
+ const char* disable_features;
+ bool expected_feature_on_state;
+ bool expected_feature_off_state;
+ } test_cases[] = {
+ {"", "", true, false},
+ {"OffByDefault", "", true, true},
+ {"OffByDefault", "OnByDefault", false, true},
+ {"OnByDefault,OffByDefault", "", true, true},
+ {"", "OnByDefault,OffByDefault", false, false},
+ // In the case an entry is both, disable takes precedence.
+ {"OnByDefault", "OnByDefault,OffByDefault", false, false},
+ };
+
+ for (size_t i = 0; i < arraysize(test_cases); ++i) {
+ const auto& test_case = test_cases[i];
+
+ ClearFeatureListInstance();
+ scoped_ptr<FeatureList> feature_list(new FeatureList);
+ feature_list->InitializeFromCommandLine(test_case.enable_features,
+ test_case.disable_features);
+ RegisterFeatureListInstance(feature_list.Pass());
+
+ EXPECT_EQ(test_case.expected_feature_on_state,
+ FeatureList::IsEnabled(kFeatureOnByDefault))
+ << i;
+ EXPECT_EQ(test_case.expected_feature_off_state,
+ FeatureList::IsEnabled(kFeatureOffByDefault))
+ << i;
+ }
+}
+
+TEST_F(FeatureListTest, CheckFeatureIdentity) {
+ // Tests that CheckFeatureIdentity() correctly detects when two different
+ // structs with the same feature name are passed to it.
+
+ // Call it twice for each feature at the top of the file, since the first call
+ // makes it remember the entry and the second call will verify it.
+ EXPECT_TRUE(feature_list()->CheckFeatureIdentity(kFeatureOnByDefault));
+ EXPECT_TRUE(feature_list()->CheckFeatureIdentity(kFeatureOnByDefault));
+ EXPECT_TRUE(feature_list()->CheckFeatureIdentity(kFeatureOffByDefault));
+ EXPECT_TRUE(feature_list()->CheckFeatureIdentity(kFeatureOffByDefault));
+
+ // Now, call it with a distinct struct for |kFeatureOnByDefaultName|, which
+ // should return false.
+ struct Feature kFeatureOnByDefault2 {
+ kFeatureOnByDefaultName, FEATURE_ENABLED_BY_DEFAULT
+ };
+ EXPECT_FALSE(feature_list()->CheckFeatureIdentity(kFeatureOnByDefault2));
+}
+
+} // namespace base
diff --git a/chrome/browser/chrome_browser_main.cc b/chrome/browser/chrome_browser_main.cc
index 4e3f70b..87ec5c0 100644
--- a/chrome/browser/chrome_browser_main.cc
+++ b/chrome/browser/chrome_browser_main.cc
@@ -13,6 +13,7 @@
#include "base/command_line.h"
#include "base/debug/crash_logging.h"
#include "base/debug/debugger.h"
+#include "base/feature_list.h"
#include "base/files/file_path.h"
#include "base/files/file_util.h"
#include "base/metrics/field_trial.h"
@@ -684,11 +685,20 @@
<< " list specified.";
metrics->AddSyntheticTrialObserver(provider);
}
+
+ scoped_ptr<base::FeatureList> feature_list(new base::FeatureList);
+ feature_list->InitializeFromCommandLine(
+ command_line->GetSwitchValueASCII(switches::kEnableFeatures),
+ command_line->GetSwitchValueASCII(switches::kDisableFeatures));
+
variations::VariationsService* variations_service =
browser_process_->variations_service();
if (variations_service)
variations_service->CreateTrialsFromSeed();
+ // TODO(asvitkine): Pass |feature_list| to CreateTrialsFromSeed() above.
+ base::FeatureList::SetInstance(feature_list.Pass());
+
// This must be called after |local_state_| is initialized.
browser_field_trials_.SetupFieldTrials();
diff --git a/chrome/common/chrome_switches.cc b/chrome/common/chrome_switches.cc
index 3a31e26..dde04fc 100644
--- a/chrome/common/chrome_switches.cc
+++ b/chrome/common/chrome_switches.cc
@@ -266,6 +266,9 @@
const char kDisableExtensionsHttpThrottling[] =
"disable-extensions-http-throttling";
+// Comma-separated list of feature names to disable. See also kEnableFeatures.
+const char kDisableFeatures[] = "disable-features";
+
// Disable field trial tests configured in fieldtrial_testing_config.json.
const char kDisableFieldTrialTestingConfig[] = "disable-field-trial-config";
@@ -437,7 +440,10 @@
// Enable the fast unload controller, which speeds up tab/window close by
// running a tab's onunload js handler independently of the GUI -
// crbug.com/142458 .
-const char kEnableFastUnload[] = "enable-fast-unload";
+const char kEnableFastUnload[] = "enable-fast-unload";
+
+// Comma-separated list of feature names to enable. See also kDisableFeatures.
+const char kEnableFeatures[] = "enable-features";
// Enables support for the QUIC protocol for insecure schemes (http://).
// This is a temporary testing flag.
diff --git a/chrome/common/chrome_switches.h b/chrome/common/chrome_switches.h
index 6eef301f..82f5f5cc 100644
--- a/chrome/common/chrome_switches.h
+++ b/chrome/common/chrome_switches.h
@@ -79,6 +79,7 @@
extern const char kDisableExtensionsFileAccessCheck[];
extern const char kDisableExtensionsHttpThrottling[];
extern const char kDisableExtensions[];
+extern const char kDisableFeatures[];
extern const char kDisableFieldTrialTestingConfig[];
extern const char kDisableJavaScriptHarmonyShipping[];
extern const char kDisableMaterialDesignDownloads[];
@@ -128,6 +129,7 @@
extern const char kEnableExtensionActivityLogging[];
extern const char kEnableExtensionActivityLogTesting[];
extern const char kEnableFastUnload[];
+extern const char kEnableFeatures[];
extern const char kEnableInsecureQuic[];
extern const char kEnableMaterialDesignDownloads[];
extern const char kEnableMaterialDesignSettings[];