Initial implementation of FeatureList in base/.

base/metrics/feature_list.h

Index: base/metrics/feature_list.h
diff --git a/base/metrics/feature_list.h b/base/metrics/feature_list.h
new file mode 100644
index 0000000000000000000000000000000000000000..554a8ffbf75fc7ce0fe09fd4fb534bfe79c7a32c
--- /dev/null
+++ b/base/metrics/feature_list.h
@@ -0,0 +1,137 @@
+// 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_METRICS_FEATURE_LIST_H_
+#define BASE_METRICS_FEATURE_LIST_H_
+
+#include <map>
+#include <string>
+
+#include "base/base_export.h"
+#include "base/basictypes.h"
+#include "base/containers/scoped_ptr_map.h"
+#include "base/gtest_prod_util.h"
+#include "base/synchronization/lock.h"
+
+namespace base {
+
+// 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 global variable.
+struct BASE_EXPORT Feature {
+  // The name of the feagture.

[rkaplow, 2015/08/30 19:36:22]

feature

[Alexei Svitkine (slow), 2015/08/31 16:21:01]

Done.

+  const char* const name;
+
+  // The default state (i.e. enabled or disabled) for this feature.
+  const bool default_state;

[Ilya Sherman, 2015/09/01 03:55:25]

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").

+};
+
+// 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", true
+//   };
+//
+// Then, client code that wishes to query the state of the feature would check:
+//
+//   if (base::FeatureList::IsEnabled(kMyGreatFeature)) {

[rkaplow, 2015/08/30 19:36:22]

super minor nit but for even for sample syntax I would have a close brace

[Alexei Svitkine (slow), 2015/08/31 16:21:01]

Done.

+//
+// 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

[rkaplow, 2015/08/30 19:36:22]

I would make it clear here the ordering of priority - which I assume is command
line, then experiment, then default.

[Alexei Svitkine (slow), 2015/08/31 16:21:01]

Done.

+// and finally its default state, to determine whether the feature is on.
+//
+// After initial 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 initialization phase
+  // (before FinalizeInitialization() has been called).
+  void InitializeFromCommandLine(const std::string& enable_features,
+                                 const std::string& disable_features);
+
+  // Finalizes the initialization state of the FeatureList instance, so that no
+  // further overrides can be registered. For the singleton feature list, this
+  // gets called when it is registered via SetInstance().
+  void FinalizeInitialization();
+
+  // Returns whether the given |feature| is enabled. This is member function is
+  // normally called through the static IsEnabled() function, which corresponds
+  // to GeInstance()->IsFeatureEnabled(). Must only be called on a FeatureList
+  // that has been fully initialized (FinalizeInitialization() called).
+  bool IsFeatureEnabled(const Feature& feature);

[rkaplow, 2015/08/30 19:36:22]

can this be private?

[Alexei Svitkine (slow), 2015/08/31 16:21:01]

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.

+
+  // 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 for in debug builds.
+  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);

[Ilya Sherman, 2015/09/01 03:55:25]

Please use a mechanism that exposes a more narrow set of internals, e.g. using
protected methods and a subclass for testing.

+
+  // 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, bool overriden_state);
+
+  // Checks whether |feature| has the same address as any previously seen
+  // Feature structs with the same name. Used only from DCHECKs and tests.
+  bool CheckFeatureIdentity(const Feature& feature);
+
+  struct OverrideEntry {
+    // The overridden enable (on/off) state of the feature.
+    bool overriden_state;
+
+    // TODO(asvitkine): Expand this as more support is added.
+
+    explicit OverrideEntry(bool overriden_state);
+  };
+  // Map from feature name to an OverrideEntry struct for the feature, if it
+  // exists. A ScopedPtrMap<> is used because OverrideEntry will later expand
+  // to hold additional non-copyable fields.
+  ScopedPtrMap<std::string, scoped_ptr<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_METRICS_FEATURE_LIST_H_