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..ed5461fe14e16c9073706ef8649d9f552c7c7a8e
--- /dev/null
+++ b/base/metrics/feature_list.h
@@ -0,0 +1,148 @@
+// 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 or a file

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

nit: Might be worth emphasizing that the global variable would be a constant.

[Alexei Svitkine (slow), 2015/09/01 15:53:44]

Done.

+// 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 bool default_state;

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

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

[Alexei Svitkine (slow), 2015/09/01 15:53:44]

Done.

+};
+
+// 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.
+//
+// Features can be explicitly forced on or off by specifying a list of comma

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

nit: s/comma/comma-

[Alexei Svitkine (slow), 2015/09/01 15:53:43]

Done.

+// separated feature names via the following command-line flags:
+//
+//   --enable-features=Feature5,Feature7
+//   --disable-features=Feature1,Feature2,Feature3
+//
+// After initial initialization (which should be done single-threaded), the

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

nit: "initial initialization" reads a little weirdly.  I'd drop the word
"initial".

[Alexei Svitkine (slow), 2015/09/01 15:53:44]

Done.

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

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

nit: "during initialization phase" -> "during the initialization phase"

[Alexei Svitkine (slow), 2015/09/01 15:53:44]

Done.

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

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

nit: Can this method be private?

[Alexei Svitkine (slow), 2015/09/01 15:53:43]

Done.

+
+  // Returns whether the given |feature| is enabled. This is member function is

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

nit: "This is" -> "This"

[Alexei Svitkine (slow), 2015/09/01 15:53:44]

Done.

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

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

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.

[Alexei Svitkine (slow), 2015/09/01 15:53:44]

Done. See my other comment for my original reasoning for this.

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

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

nit: Below, you write this as "builds with DCHECKs enabled".  Probably worth
being consistent.

[Alexei Svitkine (slow), 2015/09/01 15:53:44]

Done.

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

[Alexei Svitkine (slow), 2015/09/01 15:53:44]

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.

[Ilya Sherman, 2015/09/01 20:17:16]

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.

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

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

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.

[Alexei Svitkine (slow), 2015/09/01 15:53:43]

Done.

+  // Feature structs with the same name. Used only from DCHECKs and tests.

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

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.

[Alexei Svitkine (slow), 2015/09/01 15:53:44]

Done.

+  bool CheckFeatureIdentity(const Feature& feature);
+
+  struct OverrideEntry {
+    // The overridden enable (on/off) state of the feature.
+    bool overriden_state;

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

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.

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

nit: "riden" -> "ridden" (applies throughout)

[Alexei Svitkine (slow), 2015/09/01 15:53:43]

Done.

[Alexei Svitkine (slow), 2015/09/01 15:53:44]

Done.

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

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

nit: I would personally prefer to use a regular map for this CL, and upgrade it
to a ScopedPtrMap when needed.

[Alexei Svitkine (slow), 2015/09/01 15:53:43]

Done.

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