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..63176bde6319f626cec2e2826cb1dc76bc593e56
--- /dev/null
+++ b/base/metrics/feature_list.h
@@ -0,0 +1,140 @@
+// 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_

[brettw, 2015/08/31 21:26:00]

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.

[Alexei Svitkine (slow), 2015/08/31 21:59:09]

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.

+#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.

[brettw, 2015/08/31 21:26:00]

maybe append "or file static" after global? I don't want people to feel like
they have to make it globally visible.

[Alexei Svitkine (slow), 2015/08/31 21:59:09]

Done.

+struct BASE_EXPORT Feature {
+  // The name of the feature.

[brettw, 2015/08/31 21:26:00]

It would be nice to mention here what this is used for and what it needs to
correspond to (finch flags, etc.).

[Alexei Svitkine (slow), 2015/08/31 21:59:09]

Done.

+  const char* const name;
+
+  // The default state (i.e. enabled or disabled) for this feature.
+  const bool 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", true
+//   };
+//
+// 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.
+//
+// After initial initialization (which should be done single-threaded), the
+// FeatureList API is thread safe.

[brettw, 2015/08/31 21:26:00]

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

[Alexei Svitkine (slow), 2015/08/31 21:59:09]

Added command-line examples.

I think the experimentation stuff can probably documented elsewhere.

+//
+// 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);
+
+  // 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);
+
+  // 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.

[brettw, 2015/08/31 21:26:00]

I'm curious, what extra stuff are you thinking of putting here?

[Alexei Svitkine (slow), 2015/08/31 21:59:09]

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

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