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

components/flags_ui/flags_state.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/flags_ui/flags_state.h"
 
 #include <memory>
 #include <utility>
 
 #include "base/callback.h"
 #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"
 #include "base/values.h"
 #include "build/build_config.h"
 #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 {
 
 namespace internal {
-const char kTrialGroupAboutFlags[] = "AboutFlags";
+const char kDefaultGroupNameAboutFlags[] = "AboutFlags";
+const char kDefaultTrialNameAboutFlags[] = "AboutFlagsTrial";
 }  // namespace internal
 
 namespace {
 
 // Convert switch constants to proper CommandLine::StringType strings.
 base::CommandLine::StringType GetSwitchString(const std::string& flag) {
   base::CommandLine cmd_line(base::CommandLine::NO_PROGRAM);
   cmd_line.AppendSwitch(flag);
   DCHECK_EQ(2U, cmd_line.argv().size());
   return cmd_line.argv()[1];
@@ skipping 98 matching lines @@
     case FeatureEntry::FEATURE_VALUE:
       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;
 }
 
 // Returns true if none of this entry's options have been enabled.
 bool IsDefaultValue(const FeatureEntry& entry,
                     const std::set<std::string>& enabled_entries) {
   switch (entry.type) {
@@ skipping 26 matching lines @@
     std::unique_ptr<base::DictionaryValue> value(new base::DictionaryValue);
     const std::string name = entry.NameForOption(i);
     value->SetString("internal_name", name);
     value->SetString("description", entry.DescriptionForOption(i));
     value->SetBoolean("selected", enabled_entries.count(name) > 0);
     result->Append(std::move(value));
   }
   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
   // for the trial, yet. Thus, create the trial to select the group. This way,
   // 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;
 }
 
 }  // namespace
 
 struct FlagsState::SwitchEntry {
   // Corresponding base::Feature to toggle.
   std::string feature_name;
 
@@ skipping 199 matching lines @@
   flags_switches_.clear();
   appended_switches_.clear();
 }
 
 std::vector<std::string> FlagsState::RegisterAllFeatureVariationParameters(
     FlagsStorage* flags_storage,
     base::FeatureList* feature_list) {
   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);
+          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);
 
-          if (!field_trial)
+          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;
 }
 
 void FlagsState::GetFlagFeatureEntries(
     FlagsStorage* flags_storage,
     FlagAccess access,
     base::ListValue* supported_entries,
     base::ListValue* unsupported_entries,
     base::Callback<bool(const FeatureEntry&)> skip_feature_entry) {
   std::set<std::string> enabled_entries;
@@ skipping 292 matching lines @@
                               state == FeatureEntry::FeatureState::ENABLED,
                               name_to_switch_map);
           }
         }
         break;
     }
   }
 }
 
 }  // namespace flags_ui