Add BASE_FEATURE() / BASE_DECLARE_FEATURE() macros for defining features

There are subtle requirements around declaring and defining
base::Features. In addition, these requirements also evolve over time:
updating the various uses requires updating an estimated 6000+ uses.

Introduce helper macros modelled after absl's ABSL_FLAG() and
ABSL_DECLARE_FLAG() macros instead; in the future, base::Feature may
even become an internal implementation detail.

This also simplifies the helper script for autogenerating Java features
from C++ features, as it no longer needs to try to accommodate all
potential reorderings of `CONSTINIT`, `const`, and export macros.

Bug: 1364289
Change-Id: Id88f7c4a60efa28cdab36accb4d8386ec84ea3e4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3902697
Reviewed-by: Egor Pasko <pasko@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1048852}

base/feature_list.h

@@ -39,15 +39,42 @@ enum FeatureState {
   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. It should never be used as a constexpr as it breaks
-// pointer-based identity lookup.
+// Recommended macros for declaring and defining features:
 //
-// Note: New code should use CONSTINIT on the base::Feature declaration, as in:
+// - `kFeature` is the C++ identifier that will be used for the `base::Feature`.
+// - `name` is the feature name, which must be globally unique. This name is
+//   used to enable/disable features via experiments and command-line flags.
+//   Names should use CamelCase-style naming, e.g. "MyGreatFeature".
+// - `default_state` is the default state to use for the feature, i.e.
+//   `base::FEATURE_DISABLED_BY_DEFAULT` or `base::FEATURE_ENABLED_BY_DEFAULT`.
+//   As noted above, the actual runtime state may differ from the default state,
+//   due to field trials or command-line switches.
+
+// Provides a forward declaration for `kFeature` in a header file, e.g.
 //
-//   CONSTINIT Feature kSomeFeature("FeatureName", FEATURE_DISABLED_BY_DEFAULT);
+//   BASE_DECLARE_FEATURE(kMyFeature);
+//
+// If the feature needs to be marked as exported, i.e. it is referenced by
+// multiple components, then write:
+//
+//   COMPONENT_EXPORT(MY_COMPONENT) BASE_DECLARE_FEATURE(kMyFeature);
+#define BASE_DECLARE_FEATURE(kFeature) \
+  extern CONSTINIT const base::Feature kFeature
+
+// Provides a definition for `kFeature` with `name` and `default_state`, e.g.
+//
+//   BASE_FEATURE(kMyFeature, "MyFeature", base::FEATURE_DISABLED_BY_DEFAULT);
+//
+// Features should *not* be defined in header files; do not use this macro in
+// header files.
+#define BASE_FEATURE(feature, name, default_state) \
+  CONSTINIT const base::Feature feature(name, default_state)
+
+// The Feature struct is used to define the default state for a feature. There
+// must only ever be one struct instance for a given feature name—generally
+// defined as a constant global variable or file static. Declare and define
+// features using the `BASE_DECLARE_FEATURE()` and `BASE_FEATURE()` macros
+// above, as there are some subtleties involved.
 //
 // Feature constants are internally mutable, as this allows them to contain a
 // mutable member to cache their override state, while still remaining declared

build/android/gyp/java_cpp_features.py

@@ -15,16 +15,24 @@ from util import java_cpp_utils
 
 
 class FeatureParserDelegate(java_cpp_utils.CppConstantParser.Delegate):
+  # Ex. 'BASE_FEATURE(kConstantName, "StringNameOfTheFeature", ...);'
+  # would parse as:
+  #   ExtractConstantName() -> 'ConstantName'
+  #   ExtractValue() -> '"StringNameOfTheFeature"'
+  FEATURE_RE = re.compile(r'BASE_FEATURE\(k([^,]+),')
   # Ex. 'const base::Feature kConstantName{"StringNameOfTheFeature", ...};'
   # would parse as:
   #   ExtractConstantName() -> 'ConstantName'
   #   ExtractValue() -> '"StringNameOfTheFeature"'
-  FEATURE_RE = re.compile(
+  LEGACY_FEATURE_RE = re.compile(
       r'\s*const(?:\s+\w+_EXPORT)? (?:base::)?Feature\s+k(\w+)\s*(?:=\s*)?')
   VALUE_RE = re.compile(r'\s*("(?:\"|[^"])*")\s*,')
 
   def ExtractConstantName(self, line):
     match = FeatureParserDelegate.FEATURE_RE.match(line)
+    if match:
+      return match.group(1)
+    match = FeatureParserDelegate.LEGACY_FEATURE_RE.match(line)
     return match.group(1) if match else None
 
   def ExtractValue(self, line):

docs/android_accessing_cpp_features_in_java.md

@@ -129,11 +129,9 @@ disabled-by-default for another). Example:
 
 ```c++
 #if defined(...)
-const base::Feature kMyFeature{
-    "MyFeature", base::FEATURE_ENABLED_BY_DEFAULT};
+BASE_FEATURE(kMyFeature, "MyFeature", base::FEATURE_ENABLED_BY_DEFAULT);
 #else
-const base::Feature kMyFeature{
-    "MyFeature", base::FEATURE_DISABLED_BY_DEFAULT};
+BASE_FEATURE(kMyFeature, "MyFeature", base::FEATURE_DISABLED_BY_DEFAULT);
 #endif
 ```
 
@@ -143,12 +141,12 @@ compilation error is complaining about). Fortunately, the workaround is fairly
 simple. Rewrite the definition to only use directives around the enabled state:
 
 ```c++
-const base::Feature kMyFeature{
-    "MyFeature",
+BASE_FEATURE(kMyFeature,
+             "MyFeature",
 #if defined(...)
-    base::FEATURE_ENABLED_BY_DEFAULT
+             base::FEATURE_ENABLED_BY_DEFAULT
 #else
-    base::FEATURE_DISABLED_BY_DEFAULT
+             base::FEATURE_DISABLED_BY_DEFAULT
 #endif
 };