[chrome://flags] Let features override params in the same trial

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 7f039633e7a8a258dee0fc149f8e6838c52d76ef..c408902bd38ddc8ff4ef683f5076e69f494f5a24 100644
--- a/components/flags_ui/flags_state.cc
+++ b/components/flags_ui/flags_state.cc
@@ -26,7 +26,8 @@
 namespace flags_ui {
 
 namespace internal {
-const char kTrialGroupAboutFlags[] = "AboutFlags";
+const char kDefaultGroupNameAboutFlags[] = "AboutFlags";
+const char kDefaultTrialNameAboutFlags[] = "AboutFlagsTrial";
 }  // namespace internal
 
 namespace {
@@ -145,7 +146,6 @@ bool ValidateFeatureEntry(const FeatureEntry& e) {
       DCHECK(!e.choices);
       DCHECK(e.feature);
       DCHECK(e.feature_variations);
-      DCHECK(e.feature_trial_name);
       return true;
   }
   NOTREACHED();
@@ -192,25 +192,17 @@ base::Value* CreateOptionsData(const FeatureEntry& entry,
   return result;
 }
 
-// Registers variation parameters specified by |feature_variation| for the field
-// trial named |feature_trial_name|, unless a group for this trial has already
-// been created (e.g. via command-line switches that take precedence over
-// about:flags). In the trial, the function creates a new constant group called
-// |kTrialGroupAboutFlags|.
+// Registers variation parameters specified by |feature_variation_params| for
+// the field trial named |feature_trial_name|, unless a group for this trial has
+// already been created (e.g. via command-line switches that take precedence
+// over about:flags). In the trial, the function creates a new constant group
+// called |kDefaultGroupNameAboutFlags|.
 base::FieldTrial* RegisterFeatureVariationParameters(
     const std::string& feature_trial_name,
-    const FeatureEntry::FeatureVariation* feature_variation) {
-  std::map<std::string, std::string> params;
-  if (feature_variation) {
-    // Copy the parameters for non-null variations.
-    for (int i = 0; i < feature_variation->num_params; ++i) {
-      params[feature_variation->params[i].param_name] =
-          feature_variation->params[i].param_value;
-    }
-  }
-
+    const std::map<std::string, std::string>& feature_variation_params) {
   bool success = variations::AssociateVariationParams(
-      feature_trial_name, internal::kTrialGroupAboutFlags, params);
+      feature_trial_name, internal::kDefaultGroupNameAboutFlags,
+      feature_variation_params);
   if (!success)
     return nullptr;
   // Successful association also means that no group is created and selected
@@ -218,10 +210,10 @@ base::FieldTrial* RegisterFeatureVariationParameters(
   // the parameters cannot get overwritten in later phases (such as from the
   // server).
   base::FieldTrial* trial = base::FieldTrialList::CreateFieldTrial(
-      feature_trial_name, internal::kTrialGroupAboutFlags);
+      feature_trial_name, internal::kDefaultGroupNameAboutFlags);
   if (!trial) {
     DLOG(WARNING) << "Could not create the trial " << feature_trial_name
-                  << " with group " << internal::kTrialGroupAboutFlags;
+                  << " with group " << internal::kDefaultGroupNameAboutFlags;
   }
   return trial;
 }
@@ -441,32 +433,64 @@ std::vector<std::string> FlagsState::RegisterAllFeatureVariationParameters(
   std::set<std::string> enabled_entries;
   GetSanitizedEnabledFlagsForCurrentPlatform(flags_storage, &enabled_entries);
   std::vector<std::string> variation_ids;
+  std::map<std::string, std::set<std::string>> enabled_features_by_trial_name;
+  std::map<std::string, std::map<std::string, std::string>>
+      params_by_trial_name;
 
+  // First collect all the data for each trial.
   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 (e.StateForOption(j) == FeatureEntry::FeatureState::ENABLED &&
             enabled_entries.count(e.NameForOption(j))) {
-          // If the option is selected by the user & has variation, register it.
-          base::FieldTrial* field_trial = RegisterFeatureVariationParameters(
-              e.feature_trial_name, variation);
-
-          if (!field_trial)
+          std::string trial_name = e.feature_trial_name
+                                       ? e.feature_trial_name
+                                       : internal::kDefaultTrialNameAboutFlags;

[Alexei Svitkine (slow), 2017/02/24 16:18:20]

What happens if the user toggles multiple about:flags entries that use the
default trial name?

[jkrcal, 2017/02/27 08:04:36]

All the features from these multiple about:flags will get associated to this
default trial. The params from all these flags will get merged into the default
trial (with a DCHECK that there is no conflicting parameter mentioned in
multiple flags). 

Does it make sense to you?

[Alexei Svitkine (slow), 2017/02/28 18:45:35]

I'm not a fan of this. This could cause conflicts between parameters of
different studies.

It's nice that you're checking it here so that there's no issue if both are
specified and then conflict - so it will be caught.

However, there's still a problem where two studies have the same param names but
one of them doesn't specify it in its about flags config - e.g. relying on its
default value. Suddenly, it would now pick up the param from the other trial.

I don't think this failure mode is acceptable. I would rather a unique trial
name be generated or something like that - or just not supporting not specifying
a trial name - than introducing this failure mode.

[jkrcal, 2017/03/01 18:35:53]

Fair point. Removed the support of empty trial name.

+          // The user has chosen to enable the feature by this option.
+          enabled_features_by_trial_name[trial_name].insert(e.feature->name);
+
+          const FeatureEntry::FeatureVariation* variation =
+              e.VariationForOption(j);
+          if (!variation)
             continue;
-          feature_list->RegisterFieldTrialOverride(
-              e.feature->name,
-              base::FeatureList::OverrideState::OVERRIDE_ENABLE_FEATURE,
-              field_trial);
 
-          if (variation && variation->variation_id)
+          // The selected variation is non-default, collect its params & id.
+
+          for (int i = 0; i < variation->num_params; ++i) {
+            auto insert_result = params_by_trial_name[trial_name].insert(
+                std::make_pair(variation->params[i].param_name,
+                               variation->params[i].param_value));
+            DCHECK(insert_result.second)

[vitaliii, 2017/02/21 15:43:27]

Drive-by nit:

As of now the message looks like

"Check failed: insert_result.second. multiple values for the same parameter
'variation->params[i].param_name'specified in chrome://flags!"

Please 
1) s/multiple/Multiple
2) s/name'specified/name' specified // (space is missing)
3) probably add "are" before "specified", "values ... are specified".

[jkrcal, 2017/03/01 18:35:53]

Done.

+                << "multiple values for the same parameter '"
+                << variation->params[i].param_name
+                << "'specified in chrome://flags!";
+          }
+          if (variation->variation_id)
             variation_ids.push_back(variation->variation_id);
         }
       }
     }
   }
+
+  // Now create the trials and associate the features to them.
+  for (const auto& kv : enabled_features_by_trial_name) {
+    const std::string& trial_name = kv.first;
+    const std::set<std::string>& trial_features = kv.second;
+
+    base::FieldTrial* field_trial = RegisterFeatureVariationParameters(
+        trial_name, params_by_trial_name[trial_name]);
+    if (!field_trial)
+      continue;
+
+    for (const std::string& feature_name : trial_features) {
+      feature_list->RegisterFieldTrialOverride(
+          feature_name,
+          base::FeatureList::OverrideState::OVERRIDE_ENABLE_FEATURE,
+          field_trial);
+    }
+  }
+
   return variation_ids;
 }