Add base::FeatureParam<> struct

base/metrics/field_trial_params.h

Index: base/metrics/field_trial_params.h
diff --git a/base/metrics/field_trial_params.h b/base/metrics/field_trial_params.h
index 2490149f69457cca85f9798bd192ebd194ae7bfc..6b3b754d7fb6a7b75390c2f8948b4affd10444f9 100644
--- a/base/metrics/field_trial_params.h
+++ b/base/metrics/field_trial_params.h
@@ -91,6 +91,167 @@ BASE_EXPORT bool GetFieldTrialParamByFeatureAsBool(
     const std::string& param_name,
     bool default_value);
 
+// Shared declaration for various FeatureParam<T> types.
+//
+// TODO(sfiera): is this the way we want to parameterize for different types? I

[Alexei Svitkine (slow), 2017/04/24 18:35:47]

I think this is a good use of templates - since the type gets clearly defined at
the declaration - which is nice documentation for the param.

[sfiera, 2017/04/28 16:07:04]

Acknowledged.

+// tend to prefer template specialization when classes/functions have the same
+// functionality for different types, but separate functions are used above.
+// (Note that the T=enum case even has a slightly different API).
+template <typename T, bool IsEnum = std::is_enum<T>::value>
+struct FeatureParam {
+  // Prevent use of FeatureParam<> with unsupported types (e.g. void*). Uses T
+  // in its definition so that evaluation is deferred until the template is
+  // instantiated.
+  // TODO(sfiera): no-compile test:
+  //     https://www.chromium.org/developers/testing/no-compile-tests
+  static_assert(!std::is_same<T, T>::value, "unsupported FeatureParam<> type");
+};
+
+// Declares a string-valued parameter. Example:
+//
+//     constexpr FeatureParam<string> kAssistantName{
+//         &kAssistantFeature, "assistant_name", "HAL"};
+//
+// If the feature is not set, or set to the empty string, then Get() will return
+// the default value.
+//
+// TODO(sfiera): should Get() be a method on the object, or would it be better
+// to have a free function like GetFieldTrialParam(kAssistantName)?
+template <>
+struct FeatureParam<std::string> {
+  constexpr FeatureParam(const Feature* feature,
+                         const char* name,
+                         const char* default_value)
+      : feature(feature), name(name), default_value(default_value) {}
+
+  BASE_EXPORT std::string Get() const;
+
+  const Feature* const feature;
+  const char* const name;
+  const char* const default_value;
+};
+
+// Declares a double-valued parameter. Example:
+//
+//     constexpr FeatureParam<double> kAssistantTriggerThreshold{
+//         &kAssistantFeature, "trigger_threshold", 0.10};
+//
+// If the feature is not set, or set to an invalid double value, then Get() will
+// return the default value.
+template <>
+struct FeatureParam<double> {
+  constexpr FeatureParam(const Feature* feature,
+                         const char* name,
+                         double default_value)
+      : feature(feature), name(name), default_value(default_value) {}
+
+  BASE_EXPORT double Get() const;
+
+  const Feature* const feature;
+  const char* const name;
+  const double default_value;
+};
+
+// Declares an int-valued parameter. Example:
+//
+//     constexpr FeatureParam<int> kAssistantParallelism{
+//         &kAssistantFeature, "parallelism", 4};
+//
+// If the feature is not set, or set to an invalid double value, then Get() will
+// return the default value.
+template <>
+struct FeatureParam<int> {
+  constexpr FeatureParam(const Feature* feature,
+                         const char* name,
+                         int default_value)
+      : feature(feature), name(name), default_value(default_value) {}
+
+  BASE_EXPORT int Get() const;
+
+  const Feature* const feature;
+  const char* const name;
+  const int default_value;
+};
+
+template <>
+struct FeatureParam<bool> {
+  constexpr FeatureParam(const Feature* feature,
+                         const char* name,
+                         bool default_value)
+      : feature(feature), name(name), default_value(default_value) {}
+
+  BASE_EXPORT bool Get() const;
+
+  const Feature* const feature;
+  const char* const name;
+  const bool default_value;
+};
+
+BASE_EXPORT void LogInvalidEnumValue(const Feature& feature,
+                                     const std::string& param_name,
+                                     const std::string& value_as_string,
+                                     int default_value_as_int);
+
+// Feature param declaration for an enum, with associated options. Example:
+//
+//     constexpr FeatureParam<ShapeEnum>::Option[] kShapeParamOptions[] = {
+//         {SHAPE_CIRCLE, "circle"},
+//         {SHAPE_CYLINDER, "cylinder"},
+//         {SHAPE_PAPERCLIP, "paperclip"}};
+//     constexpr FeatureParam<ShapeEnum> kAssistantShapeParam{
+//         &kAssistantFeature, "shape", SHAPE_CIRCLE, &kShapeParamOptions};
+//
+// With this declaration, the parameter may be set to "circle", "cylinder", or
+// "paperclip", and that will be translated to one of the three enum values. By
+// default, or if the param is set to an unknown value, the parameter will be
+// assumed to be SHAPE_CIRCLE.
+//
+// TODO(sfiera): Should there be a "GetFieldTrialParamByFeatureAsEnum()"
+// function above to match the other types? Is this something we even want to
+// provide?

[Alexei Svitkine (slow), 2017/04/24 18:35:48]

I think it's not to have one given we'll be encouraging the use of these new
constructs instead.

[sfiera, 2017/04/28 16:07:04]

Acknowledged.

+template <typename Enum>
+struct FeatureParam<Enum, true> {
+  struct Option {
+    constexpr Option(Enum value, const char* name) : value(value), name(name) {}
+    Enum value;
+    const char* const name;
+  };
+
+  template <int option_count>
+  constexpr FeatureParam(const Feature* feature,
+                         const char* name,
+                         const Enum default_value,
+                         const Option (*options)[option_count])
+      : feature(feature),
+        name(name),
+        default_value(default_value),
+        options(*options),
+        option_count(option_count) {
+    // TODO(sfiera): no-compile test
+    static_assert(option_count >= 1, "FeatureParam<enum> has no options");
+  }
+
+  Enum Get() const {

[Alexei Svitkine (slow), 2017/04/24 18:35:48]

Can this be implemented out of line? If so, LogInvalidEnumValue() can be in the
anon namespace in the .cc file.

[sfiera, 2017/04/28 16:07:04]

No, unfortunately. It needs to be instantiated for each enum type.

+    std::string value = GetFieldTrialParamValueByFeature(*feature, name);
+    if (value.empty()) {

[Alexei Svitkine (slow), 2017/04/24 18:35:48]

Nit: No {}'s

[sfiera, 2017/04/28 16:07:04]

Done.

+      return default_value;
+    }
+    for (int i = 0; i < option_count; ++i) {
+      if (value == options[i].name) {
+        return options[i].value;
+      }
+    }
+    LogInvalidEnumValue(*feature, name, value, static_cast<int>(default_value));
+    return default_value;
+  }
+
+  const base::Feature* const feature;
+  const char* const name;
+  const Enum default_value;
+  const Option* const options;
+  const int option_count;
+};
+
 }  // namespace base
 
 #endif  // BASE_METRICS_FIELD_TRIAL_PARAMS_H_