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}
diff --git a/components/flags_ui/flags_state.cc b/components/flags_ui/flags_state.cc
index 441949bc..4e2b736 100644
--- a/components/flags_ui/flags_state.cc
+++ b/components/flags_ui/flags_state.cc
@@ -197,7 +197,7 @@
// 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 @@
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::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 @@
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);
}
}
}
diff --git a/components/flags_ui/flags_state.h b/components/flags_ui/flags_state.h
index 6f0ff1f..2be899c 100644
--- a/components/flags_ui/flags_state.h
+++ b/components/flags_ui/flags_state.h
@@ -16,6 +16,7 @@
#include "base/macros.h"
namespace base {
+class FeatureList;
class ListValue;
}
@@ -80,9 +81,12 @@
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
diff --git a/components/flags_ui/flags_state_unittest.cc b/components/flags_ui/flags_state_unittest.cc
index cc0a206..f8a908fd 100644
--- a/components/flags_ui/flags_state_unittest.cc
+++ b/components/flags_ui/flags_state_unittest.cc
@@ -267,10 +267,13 @@
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 @@
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 @@
// 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, 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) {
