Allow overriding variation parameter via chrome://flags.

components/flags_ui/flags_state.cc

Index: components/flags_ui/flags_state.cc
diff --git a/components/flags_ui/flags_state.cc b/components/flags_ui/flags_state.cc
index 43185bc7c90a13dec1fe79c3f58ad6657718e10c..c5e99ff92022d7a4c3778bb5ba6f0c8b55e65254 100644
--- a/components/flags_ui/flags_state.cc
+++ b/components/flags_ui/flags_state.cc
@@ -8,6 +8,7 @@
 #include "base/feature_list.h"
 #include "base/logging.h"
 #include "base/macros.h"
+#include "base/metrics/field_trial.h"
 #include "base/stl_util.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
@@ -16,6 +17,7 @@
 #include "components/flags_ui/feature_entry.h"
 #include "components/flags_ui/flags_storage.h"
 #include "components/flags_ui/flags_ui_switches.h"
+#include "components/variations/variations_associated_data.h"
 #include "ui/base/l10n/l10n_util.h"
 
 namespace flags_ui {
@@ -96,8 +98,9 @@ void AddInternalName(const FeatureEntry& e, std::set<std::string>* names) {
     case FeatureEntry::MULTI_VALUE:
     case FeatureEntry::ENABLE_DISABLE_VALUE:
     case FeatureEntry::FEATURE_VALUE:
-      for (int i = 0; i < e.num_choices; ++i)
-        names->insert(e.NameForChoice(i));
+    case FeatureEntry::FEATURE_WITH_VARIATIONS_VALUE:
+      for (int i = 0; i < e.num_options; ++i)
+        names->insert(e.NameForOption(i));
       break;
   }
 }
@@ -108,17 +111,17 @@ bool ValidateFeatureEntry(const FeatureEntry& e) {
   switch (e.type) {
     case FeatureEntry::SINGLE_VALUE:
     case FeatureEntry::SINGLE_DISABLE_VALUE:
-      DCHECK_EQ(0, e.num_choices);
+      DCHECK_EQ(0, e.num_options);
       DCHECK(!e.choices);
       return true;
     case FeatureEntry::MULTI_VALUE:
-      DCHECK_GT(e.num_choices, 0);
+      DCHECK_GT(e.num_options, 0);
       DCHECK(e.choices);
-      DCHECK(e.choices[0].command_line_switch);
-      DCHECK_EQ('\0', e.choices[0].command_line_switch[0]);
+      DCHECK(e.ChoiceForOption(0).command_line_switch);
+      DCHECK_EQ('\0', e.ChoiceForOption(0).command_line_switch[0]);
       return true;
     case FeatureEntry::ENABLE_DISABLE_VALUE:
-      DCHECK_EQ(3, e.num_choices);
+      DCHECK_EQ(3, e.num_options);
       DCHECK(!e.choices);
       DCHECK(e.command_line_switch);
       DCHECK(e.command_line_value);
@@ -126,10 +129,17 @@ bool ValidateFeatureEntry(const FeatureEntry& e) {
       DCHECK(e.disable_command_line_value);
       return true;
     case FeatureEntry::FEATURE_VALUE:
-      DCHECK_EQ(3, e.num_choices);
+      DCHECK_EQ(3, e.num_options);
       DCHECK(!e.choices);
       DCHECK(e.feature);
       return true;
+    case FeatureEntry::FEATURE_WITH_VARIATIONS_VALUE:
+      DCHECK_GT(e.num_options, 2);
+      DCHECK(!e.choices);
+      DCHECK(e.feature);
+      DCHECK(e.feature_variations);
+      DCHECK(e.feature_trial_name);
+      return true;
   }
   NOTREACHED();
   return false;
@@ -145,8 +155,9 @@ bool IsDefaultValue(const FeatureEntry& entry,
     case FeatureEntry::MULTI_VALUE:
     case FeatureEntry::ENABLE_DISABLE_VALUE:
     case FeatureEntry::FEATURE_VALUE:
-      for (int i = 0; i < entry.num_choices; ++i) {
-        if (enabled_entries.count(entry.NameForChoice(i)) > 0)
+    case FeatureEntry::FEATURE_WITH_VARIATIONS_VALUE:
+      for (int i = 0; i < entry.num_options; ++i) {
+        if (enabled_entries.count(entry.NameForOption(i)) > 0)
           return false;
       }
       return true;
@@ -156,23 +167,50 @@ bool IsDefaultValue(const FeatureEntry& entry,
 }
 
 // Returns the Value representing the choice data in the specified entry.
-base::Value* CreateChoiceData(const FeatureEntry& entry,
-                              const std::set<std::string>& enabled_entries) {
+base::Value* CreateOptionsData(const FeatureEntry& entry,
+                               const std::set<std::string>& enabled_entries) {
   DCHECK(entry.type == FeatureEntry::MULTI_VALUE ||
          entry.type == FeatureEntry::ENABLE_DISABLE_VALUE ||
-         entry.type == FeatureEntry::FEATURE_VALUE);
+         entry.type == FeatureEntry::FEATURE_VALUE ||
+         entry.type == FeatureEntry::FEATURE_WITH_VARIATIONS_VALUE);
   base::ListValue* result = new base::ListValue;
-  for (int i = 0; i < entry.num_choices; ++i) {
+  for (int i = 0; i < entry.num_options; ++i) {
     base::DictionaryValue* value = new base::DictionaryValue;
-    const std::string name = entry.NameForChoice(i);
+    const std::string name = entry.NameForOption(i);
     value->SetString("internal_name", name);
-    value->SetString("description", entry.DescriptionForChoice(i));
+    value->SetString("description", entry.DescriptionForOption(i));
     value->SetBoolean("selected", enabled_entries.count(name) > 0);
     result->Append(value);
   }
   return result;
 }
 
+void RegisterFeatureVariationParameters(

[Alexei Svitkine (slow), 2016/06/10 15:46:49]

Add a comment please.

[jkrcal, 2016/06/14 10:04:22]

Done.

+    const std::string& feature_trial_name,
+    const FeatureEntry::FeatureVariation& feature_variation) {
+  std::map<std::string, std::string> params;
+  for (int i = 0; i < feature_variation.num_params; ++i) {
+    params[feature_variation.params[i].param_name] =
+        feature_variation.params[i].param_value;
+  }
+
+  bool success = variations::AssociateVariationParams(
+      feature_trial_name, kTrialGroupAboutFlags, params);
+  if (success) {
+    // Successful association also means that no group is created and
+    // selected for the trial. Thus, create the trial to make it active
+    // and to select the group.

[Alexei Svitkine (slow), 2016/06/10 15:46:49]

The code doesn't match the comment. "active" means something specific in
FieldTrial code - that is, the group() was queried. I don't think it makes sense
to do this here, not querying it is fine - but adjust the comment.

[jkrcal, 2016/06/14 10:04:22]

Done.

+    // This way, the parameters cannot get overwritten in later phases
+    // (such as from the server).
+    base::FieldTrial* trial = base::FieldTrialList::CreateFieldTrial(
+        feature_trial_name, kTrialGroupAboutFlags);

[Alexei Svitkine (slow), 2016/06/10 15:46:49]

So, this doesn't do any Feature association with the FieldTrial here.

Please mention this in the comment and document why this is fine.

[jkrcal, 2016/06/14 10:04:22]

No, it does not. Beyond the scope of this CL, IMO. 
I added a TODO down to RegisterAllFeatureVariationParameters.

+    if (!trial) {
+      DLOG(WARNING) << "Could not create the trial " << feature_trial_name
+                    << " with group " << kTrialGroupAboutFlags;
+    }
+  }
+}
+
 }  // namespace
 
 // Keeps track of affected switches for each FeatureEntry, based on which
@@ -221,27 +259,33 @@ void FlagsState::ConvertFlagsToSwitches(
                          e.command_line_value, &name_to_switch_map);
         break;
       case FeatureEntry::MULTI_VALUE:
-        for (int j = 0; j < e.num_choices; ++j) {
-          AddSwitchMapping(e.NameForChoice(j), e.choices[j].command_line_switch,
-                           e.choices[j].command_line_value,
-                           &name_to_switch_map);
+        for (int j = 0; j < e.num_options; ++j) {
+          AddSwitchMapping(
+              e.NameForOption(j), e.ChoiceForOption(j).command_line_switch,
+              e.ChoiceForOption(j).command_line_value, &name_to_switch_map);
         }
         break;
       case FeatureEntry::ENABLE_DISABLE_VALUE:
-        AddSwitchMapping(e.NameForChoice(0), std::string(), std::string(),
+        AddSwitchMapping(e.NameForOption(0), std::string(), std::string(),
                          &name_to_switch_map);
-        AddSwitchMapping(e.NameForChoice(1), e.command_line_switch,
+        AddSwitchMapping(e.NameForOption(1), e.command_line_switch,
                          e.command_line_value, &name_to_switch_map);
-        AddSwitchMapping(e.NameForChoice(2), e.disable_command_line_switch,
+        AddSwitchMapping(e.NameForOption(2), e.disable_command_line_switch,
                          e.disable_command_line_value, &name_to_switch_map);
         break;
       case FeatureEntry::FEATURE_VALUE:
-        AddFeatureMapping(e.NameForChoice(0), std::string(), false,
-                          &name_to_switch_map);
-        AddFeatureMapping(e.NameForChoice(1), e.feature->name, true,
-                          &name_to_switch_map);
-        AddFeatureMapping(e.NameForChoice(2), e.feature->name, false,
-                          &name_to_switch_map);
+      case FeatureEntry::FEATURE_WITH_VARIATIONS_VALUE:
+        for (int j = 0; j < e.num_options; ++j) {
+          FeatureEntry::State state = e.StateForOption(j);
+          if (state == FeatureEntry::State::DEFAULT) {
+            AddFeatureMapping(e.NameForOption(j), std::string(), false,
+                              &name_to_switch_map);
+          } else {
+            AddFeatureMapping(e.NameForOption(j), e.feature->name,
+                              state == FeatureEntry::State::ENABLED,
+                              &name_to_switch_map);
+          }
+        }
         break;
     }
   }
@@ -302,11 +346,11 @@ void FlagsState::SetFeatureEntryEnabled(FlagsStorage* flags_storage,
   } else {
     if (enable) {
       // Enable the first choice.
-      needs_restart_ |= enabled_entries.insert(e->NameForChoice(0)).second;
+      needs_restart_ |= enabled_entries.insert(e->NameForOption(0)).second;
     } else {
       // Find the currently enabled choice and disable it.
-      for (int i = 0; i < e->num_choices; ++i) {
-        std::string choice_name = e->NameForChoice(i);
+      for (int i = 0; i < e->num_options; ++i) {
+        std::string choice_name = e->NameForOption(i);
         if (enabled_entries.find(choice_name) != enabled_entries.end()) {
           needs_restart_ = true;
           enabled_entries.erase(choice_name);
@@ -377,6 +421,26 @@ void FlagsState::Reset() {
   appended_switches_.clear();
 }
 
+void FlagsState::RegisterAllFeatureVariationParameters(
+    FlagsStorage* flags_storage) {
+  std::set<std::string> enabled_entries;
+  GetSanitizedEnabledFlagsForCurrentPlatform(flags_storage, &enabled_entries);
+
+  for (size_t i = 0; i < num_feature_entries_; ++i) {
+    const FeatureEntry& e = feature_entries_[i];
+    if (e.type == FeatureEntry::FEATURE_WITH_VARIATIONS_VALUE) {
+      for (int j = 0; j < e.num_options; ++j) {
+        const FeatureEntry::FeatureVariation* variation =
+            e.VariationForOption(j);
+        if (enabled_entries.count(e.NameForOption(j)) && variation != nullptr) {

[Alexei Svitkine (slow), 2016/06/10 15:46:49]

Nit: Reverse the two checks, since != nullptr check is cheaper and the other one
doesn't need to be evaluated if the first isn't true.

[jkrcal, 2016/06/14 10:04:22]

Done.

+          // If the option is selected by the user & has variation, register it.
+          RegisterFeatureVariationParameters(e.feature_trial_name, *variation);
+        }
+      }
+    }
+  }
+}
+
 void FlagsState::GetFlagFeatureEntries(
     FlagsStorage* flags_storage,
     FlagAccess access,
@@ -418,7 +482,8 @@ void FlagsState::GetFlagFeatureEntries(
       case FeatureEntry::MULTI_VALUE:
       case FeatureEntry::ENABLE_DISABLE_VALUE:
       case FeatureEntry::FEATURE_VALUE:
-        data->Set("choices", CreateChoiceData(entry, enabled_entries));
+      case FeatureEntry::FEATURE_WITH_VARIATIONS_VALUE:
+        data->Set("options", CreateOptionsData(entry, enabled_entries));
         break;
     }