Expand FeatureList to support FieldTrial association.

base/feature_list.cc

Index: base/feature_list.cc
diff --git a/base/feature_list.cc b/base/feature_list.cc
index 16eba67e99fd1ecd467cc1b0f20fde6c7f9b9149..cad6c81b87863764c49290976d7392b33905fd59 100644
--- a/base/feature_list.cc
+++ b/base/feature_list.cc
@@ -7,6 +7,7 @@
 #include <vector>
 
 #include "base/logging.h"
+#include "base/metrics/field_trial.h"
 #include "base/strings/string_split.h"
 
 namespace base {
@@ -37,13 +38,55 @@ void FeatureList::InitializeFromCommandLine(
   // Process disabled features first, so that disabled ones take precedence over
   // enabled ones (since RegisterOverride() uses insert()).
   for (const auto& feature_name : SplitFeatureListString(disable_features)) {
-    RegisterOverride(feature_name, OVERRIDE_DISABLE_FEATURE);
+    RegisterOverride(feature_name, OVERRIDE_DISABLE_FEATURE, nullptr);
   }
   for (const auto& feature_name : SplitFeatureListString(enable_features)) {
-    RegisterOverride(feature_name, OVERRIDE_ENABLE_FEATURE);
+    RegisterOverride(feature_name, OVERRIDE_ENABLE_FEATURE, nullptr);
   }
 }
 
+bool FeatureList::IsFeatureOverriddenFromCommandLine(
+    const std::string& feature_name,
+    OverrideState state) const {
+  auto it = overrides_.find(feature_name);
+  return it != overrides_.end() && it->second.overridden_state == state &&
+         !it->second.overriden_by_field_trial;

[Ilya Sherman, 2015/09/25 03:47:22]

nit: s/riden/ridden

[Alexei Svitkine (slow), 2015/09/25 15:27:17]

Done.

+}
+
+void FeatureList::RegisterFieldTrialOverride(const std::string& feature_name,
+                                             OverrideState override_state,
+                                             FieldTrial* field_trial) {
+  DCHECK(field_trial);
+  DCHECK(!ContainsKey(overrides_, feature_name) ||
+         !overrides_.at(feature_name).field_trial)

[Ilya Sherman, 2015/09/25 03:47:22]

nit: Please use operator[] (or .find()) rather than .at(), as .at() can throw an
exception.  Note that the ContainsKey call ensures that operator[] is
side-effect free.

[Alexei Svitkine (slow), 2015/09/25 03:56:52]

I used at() because [] requires a default ctor. I suppose there's no "harm" in
adding one, since our access with [] ensures we don't accidentally add extra
entries into the map. Didn't realise that at() throws though..

[Ilya Sherman, 2015/09/25 04:33:36]

Hrm.  I guess we could also just use .find() instead of either .at() or
operator[], since .find() neither throws nor requires a default constructor.

[Alexei Svitkine (slow), 2015/09/25 15:27:18]

Done.

+      << "Feature " << feature_name
+      << " has conflicting field trial overrides: "
+      << overrides_.at(feature_name).field_trial->trial_name() << " / "

[Ilya Sherman, 2015/09/25 03:47:22]

(here too)

[Alexei Svitkine (slow), 2015/09/25 15:27:18]

Done.

+      << field_trial->trial_name();
+
+  RegisterOverride(feature_name, override_state, field_trial);
+}
+
+void FeatureList::AssociateReportingFieldTrial(
+    const std::string& feature_name,
+    OverrideState for_overridden_state,
+    FieldTrial* field_trial) {
+  DCHECK(
+      IsFeatureOverriddenFromCommandLine(feature_name, for_overridden_state));
+
+  // Only one associated field trial is supported per feature. This is generally
+  // enforced server-side.
+  OverrideEntry* entry = &overrides_.at(feature_name);

[Ilya Sherman, 2015/09/25 03:47:22]

nit: Here too.

[Alexei Svitkine (slow), 2015/09/25 15:27:18]

Done.

+  if (entry->field_trial) {
+    NOTREACHED() << "Feature " << feature_name
+                 << " already has trial: " << entry->field_trial->trial_name()
+                 << ", associating trial: " << field_trial->trial_name();
+    return;
+  }
+
+  entry->field_trial = field_trial;
+}
+
 // static
 bool FeatureList::IsEnabled(const Feature& feature) {
   return GetInstance()->IsFeatureEnabled(feature);
@@ -81,6 +124,11 @@ bool FeatureList::IsFeatureEnabled(const Feature& feature) {
   auto it = overrides_.find(feature.name);
   if (it != overrides_.end()) {
     const OverrideEntry& entry = it->second;
+
+    // Activate the corresponding field trial, if necessary.
+    if (entry.field_trial)
+      entry.field_trial->group();
+
     // TODO(asvitkine) Expand this section as more support is added.
     return entry.overridden_state == OVERRIDE_ENABLE_FEATURE;
   }
@@ -89,9 +137,14 @@ bool FeatureList::IsFeatureEnabled(const Feature& feature) {
 }
 
 void FeatureList::RegisterOverride(const std::string& feature_name,
-                                   OverrideState overridden_state) {
+                                   OverrideState overridden_state,
+                                   FieldTrial* field_trial) {
   DCHECK(!initialized_);
-  overrides_.insert(make_pair(feature_name, OverrideEntry(overridden_state)));
+  // Note: The semantics of insert() is that it does not overwrite the entry if
+  // one already exists for the key. Thus, only the first override for a given
+  // feature name takes effect.
+  overrides_.insert(
+      make_pair(feature_name, OverrideEntry(overridden_state, field_trial)));

[Ilya Sherman, 2015/09/25 03:47:22]

nit: Missing "std::" in front of make_pair?

[Alexei Svitkine (slow), 2015/09/25 15:27:18]

Done. Not sure why it worked before.

 }
 
 bool FeatureList::CheckFeatureIdentity(const Feature& feature) {
@@ -107,7 +160,10 @@ bool FeatureList::CheckFeatureIdentity(const Feature& feature) {
   return it->second == &feature;
 }
 
-FeatureList::OverrideEntry::OverrideEntry(OverrideState overridden_state)
-    : overridden_state(overridden_state) {}
+FeatureList::OverrideEntry::OverrideEntry(OverrideState overridden_state,
+                                          FieldTrial* field_trial)
+    : overridden_state(overridden_state),
+      field_trial(field_trial),
+      overriden_by_field_trial(field_trial != nullptr) {}
 
 }  // namespace base