Expand FeatureList to support FieldTrial association.

components/variations/variations_seed_processor.cc

Index: components/variations/variations_seed_processor.cc
diff --git a/components/variations/variations_seed_processor.cc b/components/variations/variations_seed_processor.cc
index 169ac63104b2e983b778fe00b9de0a3fa26630fb..4228c6445217bba5d4d89b9141d314157f4a58e7 100644
--- a/components/variations/variations_seed_processor.cc
+++ b/components/variations/variations_seed_processor.cc
@@ -8,6 +8,7 @@
 #include <vector>
 
 #include "base/command_line.h"
+#include "base/feature_list.h"
 #include "base/metrics/field_trial.h"
 #include "base/strings/utf_string_conversions.h"
 #include "components/variations/processed_study.h"
@@ -79,6 +80,51 @@ void ApplyUIStringOverrides(
   }
 }
 
+// Forces the specified |experiment| to be enabled in |study|.
+void ForceExperimentState(
+    const Study& study,
+    const Study_Experiment& experiment,
+    const VariationsSeedProcessor::UIStringOverrideCallback& override_callback,
+    base::FieldTrial* trial) {
+  RegisterExperimentParams(study, experiment);
+  RegisterVariationIds(experiment, study.name());
+  if (study.activation_type() == Study_ActivationType_ACTIVATION_AUTO) {
+    trial->group();
+    // UI Strings can only be overridden from ACTIVATION_AUTO experiments.
+    ApplyUIStringOverrides(experiment, override_callback);
+  }
+}
+
+// Registers feature overrides for the chosen experiment in the specified study.
+void RegisterFeatureOverrides(const ProcessedStudy& processed_study,
+                              base::FieldTrial* trial,
+                              base::FeatureList* feature_list) {
+  const std::string& group_name = trial->GetGroupNameWithoutActivation();
+  int experiment_index = processed_study.GetExperimentIndexByName(group_name);
+  // The field trial was defined from |study|, so the active experiment's name
+  // must be in the |study|.
+  DCHECK_NE(-1, experiment_index);
+
+  const Study_Experiment& experiment =
+      processed_study.study()->experiment(experiment_index);
+
+  // Process all the features to enable.
+  int feature_count = experiment.feature_association().enable_feature_size();
+  for (int i = 0; i < feature_count; ++i) {
+    feature_list->RegisterFieldTrialOverride(
+        experiment.feature_association().enable_feature(i),
+        base::FeatureList::OVERRIDE_ENABLE_FEATURE, trial);
+  }
+
+  // Process all the features to disable.
+  feature_count = experiment.feature_association().disable_feature_size();
+  for (int i = 0; i < feature_count; ++i) {
+    feature_list->RegisterFieldTrialOverride(
+        experiment.feature_association().disable_feature(i),
+        base::FeatureList::OVERRIDE_DISABLE_FEATURE, trial);
+  }

[Ilya Sherman, 2015/09/16 00:50:44]

It might be nice to verify that no feature is simultaneously overridden to be on
and to be off, either by one study or by multiple.

[Alexei Svitkine (slow), 2015/09/22 21:19:59]

This will be enforced server-side. Not sure what kind of verification you were
thinking of. The behavior is such that the first override take precedence.

Anyway, I've modified this to make RegisterOverride() internally in FeatureList
to return the result of the insert, and made RegisterFieldTrialOverride() check
that result and hit a NOTREACHED() if two trials tried to override the same
feature's state. Let me know if this is what you were thinking of.

[Ilya Sherman, 2015/09/24 01:11:32]

Yep, I like that.  That way, if we ever fail to catch something on the
server-side, at least we'll have a record of it client-side.

+}
+
 }  // namespace
 
 VariationsSeedProcessor::VariationsSeedProcessor() {
@@ -97,7 +143,8 @@ void VariationsSeedProcessor::CreateTrialsFromSeed(
     const std::string& hardware_class,
     const std::string& session_consistency_country,
     const std::string& permanent_consistency_country,
-    const UIStringOverrideCallback& override_callback) {
+    const UIStringOverrideCallback& override_callback,
+    base::FeatureList* feature_list) {
   std::vector<ProcessedStudy> filtered_studies;
   FilterAndValidateStudies(seed, locale, reference_date, version, channel,
                            form_factor, hardware_class,
@@ -105,12 +152,13 @@ void VariationsSeedProcessor::CreateTrialsFromSeed(
                            permanent_consistency_country, &filtered_studies);
 
   for (size_t i = 0; i < filtered_studies.size(); ++i)
-    CreateTrialFromStudy(filtered_studies[i], override_callback);
+    CreateTrialFromStudy(filtered_studies[i], override_callback, feature_list);
 }
 
 void VariationsSeedProcessor::CreateTrialFromStudy(
     const ProcessedStudy& processed_study,
-    const UIStringOverrideCallback& override_callback) {
+    const UIStringOverrideCallback& override_callback,
+    base::FeatureList* feature_list) {
   const Study& study = *processed_study.study();
 
   // Check if any experiments need to be forced due to a command line
@@ -120,28 +168,43 @@ void VariationsSeedProcessor::CreateTrialFromStudy(
     const Study_Experiment& experiment = study.experiment(i);
     if (experiment.has_forcing_flag() &&
         command_line->HasSwitch(experiment.forcing_flag())) {
-      scoped_refptr<base::FieldTrial> trial(
-          base::FieldTrialList::CreateFieldTrial(study.name(),
-                                                 experiment.name()));
-      // If |trial| is NULL, then there might already be a trial forced to a
+      base::FieldTrial* trial = base::FieldTrialList::CreateFieldTrial(
+          study.name(), experiment.name());
+      // If |trial| is null, then there might already be a trial forced to a
       // different group (e.g. via --force-fieldtrials). Break out of the loop,
       // but don't return, so that variation ids and params for the selected
       // group will still be picked up.
-      if (!trial.get())
+      if (!trial)
         break;
 
-      RegisterExperimentParams(study, experiment);
-      RegisterVariationIds(experiment, study.name());
-      if (study.activation_type() == Study_ActivationType_ACTIVATION_AUTO) {
-        trial->group();
-        // UI Strings can only be overridden from ACTIVATION_AUTO experiments.
-        ApplyUIStringOverrides(experiment, override_callback);
-      }
-
+      ForceExperimentState(study, experiment, override_callback, trial);
       DVLOG(1) << "Trial " << study.name() << " forced by flag: "
                << experiment.forcing_flag();

[Ilya Sherman, 2015/09/16 00:50:44]

Probably makes sense to either introduce similar logging below, or to remove
this logging.

[Alexei Svitkine (slow), 2015/09/22 21:19:59]

Done. Removed the logging, since don't remember a case when this was useful.

       return;
     }
+
+    base::FieldTrial* trial;
+    if (experiment.feature_association().has_forcing_feature_on()) {
+      trial = feature_list->AssociateReportingFieldTrial(
+          experiment.feature_association().forcing_feature_on(),
+          base::FeatureList::OVERRIDE_ENABLE_FEATURE, study.name(),
+          experiment.name());
+    } else if (experiment.feature_association().has_forcing_feature_off()) {
+      trial = feature_list->AssociateReportingFieldTrial(
+          experiment.feature_association().forcing_feature_off(),
+          base::FeatureList::OVERRIDE_DISABLE_FEATURE, study.name(),
+          experiment.name());
+    } else {
+      continue;
+    }
+
+    // If |trial| is null, then the specified feature has not been forced on or
+    // off on the FeatureList via the command-line, so safe to skip this entry.
+    if (trial) {
+      // Otherwise, force this experiment state and we're done with this study.
+      ForceExperimentState(study, experiment, override_callback, trial);
+      return;
+    }
   }
 
   uint32 randomization_seed = 0;
@@ -169,14 +232,18 @@ void VariationsSeedProcessor::CreateTrialFromStudy(
           randomization_seed, NULL));
 
   bool has_overrides = false;
+  bool controls_feature_state = false;
   for (int i = 0; i < study.experiment_size(); ++i) {
     const Study_Experiment& experiment = study.experiment(i);
     RegisterExperimentParams(study, experiment);
 
     // Groups with forcing flags have probability 0 and will never be selected.
     // Therefore, there's no need to add them to the field trial.
-    if (experiment.has_forcing_flag())
+    if (experiment.has_forcing_flag() ||
+        experiment.feature_association().has_forcing_feature_on() ||
+        experiment.feature_association().has_forcing_feature_off()) {
       continue;
+    }
 
     if (experiment.name() != study.default_experiment_name())
       trial->AppendGroup(experiment.name(), experiment.probability_weight());
@@ -184,9 +251,17 @@ void VariationsSeedProcessor::CreateTrialFromStudy(
     RegisterVariationIds(experiment, study.name());
 
     has_overrides = has_overrides || experiment.override_ui_string_size() > 0;
+    if (experiment.feature_association().enable_feature_size() != 0 ||
+        experiment.feature_association().disable_feature_size() != 0) {
+      controls_feature_state = true;
+    }
   }
 
   trial->SetForced();
+
+  if (controls_feature_state)

[Ilya Sherman, 2015/09/16 00:50:44]

nit: RegisterFeatureOverrides is safe to call even if there aren't any features
to override -- right?  If so, let's skip the boolean and this conditional, and
just always call RegisterFeatureOverrides()

[Alexei Svitkine (slow), 2015/09/22 21:19:59]

RegisterFeatureOverrides() uses GetExperimentIndexByName() which does a linear
iteration over experiments in the study to compare their names.

Sure, it's not a lot of work, but it's O(#studies * #experimentsInStudy *
#nameLength) operations. I think it's worth avoiding the extra work with the
simple if, given this is on Chrome's critical start up path.

[Ilya Sherman, 2015/09/24 01:11:32]

That sounds like we're saving on the order of thousands of ops, which seems
marginal compared to everything else we do even in just this function alone. 
But, I don't feel strongly about this.

If we do keep the if-stmt, I'd rename the variable to
"does_control_feature_state", because "controls" can be read as either a verb or
as a plural noun, and that garden path throws me off every time I read it :P

[Alexei Svitkine (slow), 2015/09/24 20:49:21]

Changed to |enables_or_disables_features|, as I found "does_" wording awkward.
Let me know if this works for you.

+    RegisterFeatureOverrides(processed_study, trial.get(), feature_list);
+
   if (processed_study.is_expired()) {
     trial->Disable();
   } else if (study.activation_type() == Study_ActivationType_ACTIVATION_AUTO) {