[go: nahoru, domu]

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[];