Registering field trial for a feature overridden in chrome://flags.

Variation parameters can be overridden via chrome://flags. Internally, a
fresh trial group is created with the specified parameters. Previously,
the trial was not registered for the associated feature.

This CL registers the trial for the given feature.

BUG=625993

Review-Url: https://codereview.chromium.org/2129543002
Cr-Commit-Position: refs/heads/master@{#405496}

chrome/browser/about_flags.cc

@@ -2039,9 +2039,10 @@ void ConvertFlagsToSwitches(flags_ui::FlagsStorage* flags_storage,
 }
 
 void RegisterAllFeatureVariationParameters(
-    flags_ui::FlagsStorage* flags_storage) {
+    flags_ui::FlagsStorage* flags_storage,
+    base::FeatureList* feature_list) {
   FlagsStateSingleton::GetFlagsState()->RegisterAllFeatureVariationParameters(
-      flags_storage);
+      flags_storage, feature_list);
 }
 
 bool AreSwitchesIdenticalToCurrentCommandLine(

chrome/browser/about_flags.h

@@ -19,6 +19,7 @@
 #include "components/flags_ui/flags_state.h"
 
 namespace base {
+class FeatureList;
 class ListValue;
 }
 
@@ -34,10 +35,13 @@ void ConvertFlagsToSwitches(flags_ui::FlagsStorage* flags_storage,
                             base::CommandLine* command_line,
                             flags_ui::SentinelsMode sentinels);
 
-// Registers variations parameter values stored in |flags_storage| (previously
-// selected in about:flags).
+// Registers variations parameter values selected for features in about:flags.
+// The selected flags are retrieved from |flags_storage|, the registered
+// variation parameters are connected to their corresponding features in
+// |feature_list|.
 void RegisterAllFeatureVariationParameters(
-    flags_ui::FlagsStorage* flags_storage);
+    flags_ui::FlagsStorage* flags_storage,
+    base::FeatureList* feature_list);
 
 // Compares a set of switches of the two provided command line objects and
 // returns true if they are the same and false otherwise.

chrome/browser/chrome_browser_main.cc

@@ -727,12 +727,14 @@ void ChromeBrowserMainParts::SetupMetricsAndFieldTrials() {
     metrics->AddSyntheticTrialObserver(provider);
   }
 
+  std::unique_ptr<base::FeatureList> feature_list(new base::FeatureList);
+
   // Associate parameters chosen in about:flags and create trial/group for them.
   flags_ui::PrefServiceFlagsStorage flags_storage(
       g_browser_process->local_state());
-  about_flags::RegisterAllFeatureVariationParameters(&flags_storage);
+  about_flags::RegisterAllFeatureVariationParameters(&flags_storage,
+    feature_list.get());
 
-  std::unique_ptr<base::FeatureList> feature_list(new base::FeatureList);
   feature_list->InitializeFromCommandLine(
       command_line->GetSwitchValueASCII(switches::kEnableFeatures),
       command_line->GetSwitchValueASCII(switches::kDisableFeatures));

components/flags_ui/flags_state.cc

@@ -197,7 +197,7 @@ base::Value* CreateOptionsData(const FeatureEntry& entry,
 // 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|.
-void RegisterFeatureVariationParameters(
+base::FieldTrial* RegisterFeatureVariationParameters(
     const std::string& feature_trial_name,
     const FeatureEntry::FeatureVariation* feature_variation) {
   std::map<std::string, std::string> params;
@@ -211,18 +211,19 @@ void RegisterFeatureVariationParameters(
 
   bool success = variations::AssociateVariationParams(
       feature_trial_name, internal::kTrialGroupAboutFlags, params);
-  if (success) {
-    // 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);
-    if (!trial) {
-      DLOG(WARNING) << "Could not create the trial " << feature_trial_name
-                    << " with group " << internal::kTrialGroupAboutFlags;
-    }
+  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);
+  if (!trial) {
+    DLOG(WARNING) << "Could not create the trial " << feature_trial_name
+                  << " with group " << internal::kTrialGroupAboutFlags;
   }
+  return trial;
 }
 
 }  // namespace
@@ -436,7 +437,8 @@ void FlagsState::Reset() {
 }
 
 void FlagsState::RegisterAllFeatureVariationParameters(
-    FlagsStorage* flags_storage) {
+    FlagsStorage* flags_storage,
+    base::FeatureList* feature_list) {
   std::set<std::string> enabled_entries;
   GetSanitizedEnabledFlagsForCurrentPlatform(flags_storage, &enabled_entries);
 
@@ -448,20 +450,16 @@ void FlagsState::RegisterAllFeatureVariationParameters(
             e.VariationForOption(j);
         if (e.StateForOption(j) == FeatureEntry::FeatureState::ENABLED &&
             enabled_entries.count(e.NameForOption(j))) {
-          // If the option is enabled by the user, register it.
-          RegisterFeatureVariationParameters(e.feature_trial_name, variation);
-          // TODO(jkrcal) The code does not associate the feature with the field
-          // trial |e.feature_trial_name|. The reason is that features
-          // overridden in chrome://flags are translated to command-line flags
-          // and thus treated earlier in the initialization. The fix requires
-          // larger changes. As a result:
-          //  - the API calls to variations::GetVariationParamValueByFeature and
-          //    to variations::GetVariationParamsByFeature do not work; and
-          //  - the API call to base::FeatureList::IsEnabled does not mark the
-          //    field trial as active (and the trial does not appear in UMA).
-          // If the code calls variations::GetVariationParamValue or
-          // variations::GetVariationParams providing the trial name, everything
-          // should work fine.
+          // 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)
+            continue;
+          feature_list->RegisterFieldTrialOverride(
+              e.feature->name,
+              base::FeatureList::OverrideState::OVERRIDE_ENABLE_FEATURE,
+              field_trial);
         }
       }
     }

components/flags_ui/flags_state.h

@@ -16,6 +16,7 @@
 #include "base/macros.h"
 
 namespace base {
+class FeatureList;
 class ListValue;
 }
 
@@ -80,9 +81,12 @@ class FlagsState {
   void ResetAllFlags(FlagsStorage* flags_storage);
   void Reset();
 
-  // Registers variations parameter values stored in |flags_storage| (previously
-  // selected in about:flags).
-  void RegisterAllFeatureVariationParameters(FlagsStorage* flags_storage);
+  // Registers variations parameter values selected for features in about:flags.
+  // The selected flags are retrieved from |flags_storage|, the registered
+  // variation parameters are connected to their corresponding features in
+  // |feature_list|.
+  void RegisterAllFeatureVariationParameters(FlagsStorage* flags_storage,
+                                             base::FeatureList* feature_list);
 
   // Gets the list of feature entries. Entries that are available for the
   // current platform are appended to |supported_entries|; all other entries are

components/flags_ui/flags_state_unittest.cc

@@ -267,10 +267,13 @@ TEST_F(FlagsStateTest, ConvertFlagsToSwitches) {
 
 TEST_F(FlagsStateTest, RegisterAllFeatureVariationParameters) {
   const FeatureEntry& entry = kEntries[7];
+  std::unique_ptr<base::FeatureList> feature_list(new base::FeatureList);
+
   // Select the "Default" variation.
   flags_state_->SetFeatureEntryEnabled(&flags_storage_, entry.NameForOption(0),
                                        true);
-  flags_state_->RegisterAllFeatureVariationParameters(&flags_storage_);
+  flags_state_->RegisterAllFeatureVariationParameters(&flags_storage_,
+                                                      feature_list.get());
   // No value should be associated.
   EXPECT_EQ("", variations::GetVariationParamValue(kTestTrial, kTestParam));
   // The trial should not be created.
@@ -281,7 +284,8 @@ TEST_F(FlagsStateTest, RegisterAllFeatureVariationParameters) {
   flags_state_->SetFeatureEntryEnabled(&flags_storage_, entry.NameForOption(1),
                                        true);
 
-  flags_state_->RegisterAllFeatureVariationParameters(&flags_storage_);
+  flags_state_->RegisterAllFeatureVariationParameters(&flags_storage_,
+                                                      feature_list.get());
   // No value should be associated as this is the default option.
   EXPECT_EQ("",
             variations::GetVariationParamValue(kTestTrial, kTestParam));
@@ -295,7 +299,8 @@ TEST_F(FlagsStateTest, RegisterAllFeatureVariationParameters) {
   // Select the only one variation.
   flags_state_->SetFeatureEntryEnabled(&flags_storage_, entry.NameForOption(2),
                                        true);
-  flags_state_->RegisterAllFeatureVariationParameters(&flags_storage_);
+  flags_state_->RegisterAllFeatureVariationParameters(&flags_storage_,
+                                                      feature_list.get());
   // Associating for the second time should not change the value.
   EXPECT_EQ("",
             variations::GetVariationParamValue(kTestTrial, kTestParam));
@@ -303,13 +308,26 @@ TEST_F(FlagsStateTest, RegisterAllFeatureVariationParameters) {
 
 TEST_F(FlagsStateTest, RegisterAllFeatureVariationParametersNonDefault) {
   const FeatureEntry& entry = kEntries[7];
+  std::unique_ptr<base::FeatureList> feature_list(new base::FeatureList);
+
   // Select the only one variation.
   flags_state_->SetFeatureEntryEnabled(&flags_storage_, entry.NameForOption(2),
                                        true);
-  flags_state_->RegisterAllFeatureVariationParameters(&flags_storage_);
+  flags_state_->RegisterAllFeatureVariationParameters(&flags_storage_,
+                                                      feature_list.get());
+
+  // Set the feature_list as the main instance so that
+  // variations::GetVariationParamValueByFeature below works.
+  base::FeatureList::ClearInstanceForTesting();
+  base::FeatureList::SetInstance(std::move(feature_list));
+
   // The param should have the value predefined in this variation.
   EXPECT_EQ(kTestParamValue,
             variations::GetVariationParamValue(kTestTrial, kTestParam));
+
+  // The value should be associated also via the name of the feature.
+  EXPECT_EQ(kTestParamValue, variations::GetVariationParamValueByFeature(
+                                  kTestFeature2, kTestParam));
 }
 
 base::CommandLine::StringType CreateSwitch(const std::string& value) {