diff options
| -rw-r--r-- | chrome/common/extensions/feature_switch_unittest.cc | 76 | ||||
| -rw-r--r-- | extensions/common/feature_switch.cc | 85 | ||||
| -rw-r--r-- | extensions/common/feature_switch.h | 15 |
3 files changed, 137 insertions, 39 deletions
diff --git a/chrome/common/extensions/feature_switch_unittest.cc b/chrome/common/extensions/feature_switch_unittest.cc index 91c5633..1f5e608 100644 --- a/chrome/common/extensions/feature_switch_unittest.cc +++ b/chrome/common/extensions/feature_switch_unittest.cc @@ -16,9 +16,10 @@ namespace { const char kSwitchName[] = "test-switch"; const char kFieldTrialName[] = "field-trial"; -// Create and register a field trial that will always return the given -// |group_name|. -scoped_refptr<base::FieldTrial> CreateFieldTrial( +// Create and register a field trial named |field_trial_name| that will always +// return the given |group_name|. +scoped_refptr<base::FieldTrial> CreateFieldTrialWithName( + const std::string& field_trial_name, const std::string& group_name) { const int kTotalProbability = 10; // Note: This code will probably fail in the year 5000. But all the cycles we @@ -26,12 +27,19 @@ scoped_refptr<base::FieldTrial> CreateFieldTrial( // worth it. scoped_refptr<base::FieldTrial> trial = base::FieldTrialList::FactoryGetFieldTrial( - kFieldTrialName, kTotalProbability, "default", 5000, 1, 1, + field_trial_name, kTotalProbability, "default", 5000, 1, 1, base::FieldTrial::SESSION_RANDOMIZED, nullptr); trial->AppendGroup(group_name, kTotalProbability); return trial; } +// Create and register a field trial that will always return the given +// |group_name|. +scoped_refptr<base::FieldTrial> CreateFieldTrial( + const std::string& group_name) { + return CreateFieldTrialWithName(kFieldTrialName, group_name); +} + template<FeatureSwitch::DefaultValue T> class FeatureSwitchTest : public testing::Test { public: @@ -156,9 +164,10 @@ TEST_F(FeatureSwitchEnabledTest, TrueFieldTrialValue) { scoped_refptr<base::FieldTrial> enabled_trial = CreateFieldTrial("Enabled"); { // A default-enabled switch should be enabled (naturally). - FeatureSwitch default_enabled_switch(&command_line_, kSwitchName, - kFieldTrialName, - FeatureSwitch::DEFAULT_ENABLED); + FeatureSwitch default_enabled_switch( + &command_line_, kSwitchName, + std::vector<std::string>(1, kFieldTrialName), + FeatureSwitch::DEFAULT_ENABLED); EXPECT_TRUE(default_enabled_switch.IsEnabled()); // Scoped overrides override everything. FeatureSwitch::ScopedOverride scoped_override(&default_enabled_switch, @@ -168,9 +177,10 @@ TEST_F(FeatureSwitchEnabledTest, TrueFieldTrialValue) { { // A default-disabled switch should be enabled because of the field trial. - FeatureSwitch default_disabled_switch(&command_line_, kSwitchName, - kFieldTrialName, - FeatureSwitch::DEFAULT_DISABLED); + FeatureSwitch default_disabled_switch( + &command_line_, kSwitchName, + std::vector<std::string>(1, kFieldTrialName), + FeatureSwitch::DEFAULT_DISABLED); EXPECT_TRUE(default_disabled_switch.IsEnabled()); // Scoped overrides override everything. FeatureSwitch::ScopedOverride scoped_override(&default_disabled_switch, @@ -185,9 +195,10 @@ TEST_F(FeatureSwitchEnabledTest, FalseFieldTrialValue) { scoped_refptr<base::FieldTrial> disabled_trial = CreateFieldTrial("Disabled"); { // A default-enabled switch should be disabled because of the field trial. - FeatureSwitch default_enabled_switch(&command_line_, kSwitchName, - kFieldTrialName, - FeatureSwitch::DEFAULT_ENABLED); + FeatureSwitch default_enabled_switch( + &command_line_, kSwitchName, + std::vector<std::string>(1, kFieldTrialName), + FeatureSwitch::DEFAULT_ENABLED); EXPECT_FALSE(default_enabled_switch.IsEnabled()); // Scoped overrides override everything. FeatureSwitch::ScopedOverride scoped_override(&default_enabled_switch, @@ -197,9 +208,10 @@ TEST_F(FeatureSwitchEnabledTest, FalseFieldTrialValue) { { // A default-disabled switch should remain disabled. - FeatureSwitch default_disabled_switch(&command_line_, kSwitchName, - kFieldTrialName, - FeatureSwitch::DEFAULT_DISABLED); + FeatureSwitch default_disabled_switch( + &command_line_, kSwitchName, + std::vector<std::string>(1, kFieldTrialName), + FeatureSwitch::DEFAULT_DISABLED); EXPECT_FALSE(default_disabled_switch.IsEnabled()); // Scoped overrides override everything. FeatureSwitch::ScopedOverride scoped_override(&default_disabled_switch, @@ -207,3 +219,35 @@ TEST_F(FeatureSwitchEnabledTest, FalseFieldTrialValue) { EXPECT_TRUE(default_disabled_switch.IsEnabled()); } } + +TEST_F(FeatureSwitchEnabledTest, + TrueFieldTrialValueAndTrueRequiredFieldTrialValue) { + std::vector<std::string> required_trials; + base::FieldTrialList field_trials(nullptr); + scoped_refptr<base::FieldTrial> enabled_trial = CreateFieldTrial("Enabled"); + required_trials.push_back(kFieldTrialName); + const char* required_trial_name = "required-trial"; + scoped_refptr<base::FieldTrial> enabled_required_trial = + CreateFieldTrialWithName(required_trial_name, "Enabled"); + required_trials.push_back(required_trial_name); + FeatureSwitch trial_enabled_switch(&command_line_, kSwitchName, + required_trials, + FeatureSwitch::DEFAULT_DISABLED); + EXPECT_TRUE(trial_enabled_switch.IsEnabled()); +} + +TEST_F(FeatureSwitchEnabledTest, + TrueFieldTrialValueAndFalseRequiredFieldTrialValue) { + std::vector<std::string> required_trials; + base::FieldTrialList field_trials(nullptr); + scoped_refptr<base::FieldTrial> enabled_trial = CreateFieldTrial("Enabled"); + required_trials.push_back(kFieldTrialName); + const char* required_trial_name = "required-trial"; + scoped_refptr<base::FieldTrial> enabled_required_trial = + CreateFieldTrialWithName(required_trial_name, "Disabled"); + required_trials.push_back(required_trial_name); + FeatureSwitch trial_enabled_switch(&command_line_, kSwitchName, + required_trials, + FeatureSwitch::DEFAULT_DISABLED); + EXPECT_FALSE(trial_enabled_switch.IsEnabled()); +} diff --git a/extensions/common/feature_switch.cc b/extensions/common/feature_switch.cc index d8c6b98..ca73db0 100644 --- a/extensions/common/feature_switch.cc +++ b/extensions/common/feature_switch.cc @@ -15,11 +15,21 @@ namespace extensions { namespace { +// The switch media-router is defined in chrome/common/chrome_switches.cc, but +// we can't depend on chrome here. +const char kMediaRouterFlag[] = "media-router"; + const char kEnableMediaRouterExperiment[] = "EnableMediaRouter"; const char kEnableMediaRouterWithCastExtensionExperiment[] = "EnableMediaRouterWithCastExtension"; const char kExtensionActionRedesignExperiment[] = "ExtensionActionRedesign"; +const char* kMediaRouterRequiredExperiments[] = { + kEnableMediaRouterExperiment, kExtensionActionRedesignExperiment}; +const char* kMediaRouterWithCastExtensionRequiredExperiments[] = { + kEnableMediaRouterWithCastExtensionExperiment, + kExtensionActionRedesignExperiment}; + class CommonSwitches { public: CommonSwitches() @@ -51,14 +61,19 @@ class CommonSwitches { FeatureSwitch::DEFAULT_DISABLED), trace_app_source(switches::kTraceAppSource, FeatureSwitch::DEFAULT_ENABLED), - // The switch media-router is defined in - // chrome/common/chrome_switches.cc, but we can't depend on chrome here. - media_router("media-router", - kEnableMediaRouterExperiment, + media_router(kMediaRouterFlag, + std::vector<std::string>( + kMediaRouterRequiredExperiments, + kMediaRouterRequiredExperiments + + arraysize(kMediaRouterRequiredExperiments)), FeatureSwitch::DEFAULT_DISABLED), media_router_with_cast_extension( - "media-router", - kEnableMediaRouterWithCastExtensionExperiment, + kMediaRouterFlag, + std::vector<std::string>( + kMediaRouterWithCastExtensionRequiredExperiments, + kMediaRouterWithCastExtensionRequiredExperiments + + arraysize( + kMediaRouterWithCastExtensionRequiredExperiments)), FeatureSwitch::DEFAULT_DISABLED) { } @@ -107,6 +122,9 @@ FeatureSwitch* FeatureSwitch::extension_action_redesign() { // Force-enable the redesigned extension action toolbar when the Media Router // is enabled. Should be removed when the toolbar redesign is used by default. // See crbug.com/514694 + // Note that if Media Router is enabled by experiment, it implies that the + // extension action redesign is also enabled by experiment. Thus it is fine + // to return the override switch. // TODO(kmarshall): Remove this override. if (media_router()->IsEnabled()) return &g_common_switches.Get().extension_action_redesign_override; @@ -152,24 +170,39 @@ FeatureSwitch::FeatureSwitch(const char* switch_name, DefaultValue default_value) : FeatureSwitch(base::CommandLine::ForCurrentProcess(), switch_name, - field_trial_name, + std::vector<std::string>(1, field_trial_name), default_value) {} -FeatureSwitch::FeatureSwitch(const base::CommandLine* command_line, - const char* switch_name, - DefaultValue default_value) - : FeatureSwitch(command_line, switch_name, nullptr, default_value) {} +FeatureSwitch::FeatureSwitch( + const char* switch_name, + const std::vector<std::string>& required_field_trials, + DefaultValue default_value) + : FeatureSwitch(base::CommandLine::ForCurrentProcess(), + switch_name, + required_field_trials, + default_value) {} FeatureSwitch::FeatureSwitch(const base::CommandLine* command_line, const char* switch_name, - const char* field_trial_name, DefaultValue default_value) + : FeatureSwitch(command_line, + switch_name, + std::vector<std::string>(), + default_value) {} + +FeatureSwitch::FeatureSwitch( + const base::CommandLine* command_line, + const char* switch_name, + const std::vector<std::string>& required_field_trials, + DefaultValue default_value) : command_line_(command_line), switch_name_(switch_name), - field_trial_name_(field_trial_name), + required_field_trials_(required_field_trials), default_value_(default_value == DEFAULT_ENABLED), override_value_(OVERRIDE_NONE) {} +FeatureSwitch::~FeatureSwitch(){}; + bool FeatureSwitch::IsEnabled() const { if (override_value_ != OVERRIDE_NONE) return override_value_ == OVERRIDE_ENABLED; @@ -187,19 +220,33 @@ bool FeatureSwitch::IsEnabled() const { if (switch_value == "0") return false; + // TODO(imcheng): Don't check |default_value_|. Otherwise, we could improperly + // ignore an enable/disable switch if there is a field trial active. + // crbug.com/585569 if (!default_value_ && command_line_->HasSwitch(GetLegacyEnableFlag())) return true; if (default_value_ && command_line_->HasSwitch(GetLegacyDisableFlag())) return false; - if (field_trial_name_) { - std::string group_name = - base::FieldTrialList::FindFullName(field_trial_name_); - if (group_name == "Enabled") - return true; - if (group_name == "Disabled") + if (!required_field_trials_.empty()) { + bool enabled_by_field_trial = true; + bool disabled_by_field_trial = false; + for (const std::string& field_trial_name : required_field_trials_) { + std::string group_name = + base::FieldTrialList::FindFullName(field_trial_name); + if (group_name != "Enabled") { + enabled_by_field_trial = false; + if (group_name == "Disabled") { + disabled_by_field_trial = true; + break; + } + } + } + if (disabled_by_field_trial) return false; + if (enabled_by_field_trial) + return true; } return default_value_; diff --git a/extensions/common/feature_switch.h b/extensions/common/feature_switch.h index c170f9c..b39cc8c 100644 --- a/extensions/common/feature_switch.h +++ b/extensions/common/feature_switch.h @@ -6,6 +6,7 @@ #define EXTENSIONS_COMMON_FEATURE_SWITCH_H_ #include <string> +#include <vector> #include "base/macros.h" @@ -26,8 +27,10 @@ namespace extensions { // the finch config). // 3. If there is a switch name, and the switch is present in the command line, // the command line value will be used. -// 4. If there is a finch experiment associated and applicable to the machine, -// the finch value will be used. +// 4. If there are field trials associated with the feature, and the machine +// is in the "Enabled" group for all field trials, then the feature is +// enabled. If the machine is in the "Disabled" group for any field trials, +// the feature is disabled. // 5. Otherwise, the default value is used. class FeatureSwitch { public: @@ -72,13 +75,17 @@ class FeatureSwitch { FeatureSwitch(const char* switch_name, const char* field_trial_name, DefaultValue default_value); + FeatureSwitch(const char* switch_name, + const std::vector<std::string>& required_field_trials, + DefaultValue default_value); FeatureSwitch(const base::CommandLine* command_line, const char* switch_name, DefaultValue default_value); FeatureSwitch(const base::CommandLine* command_line, const char* switch_name, - const char* field_trial_name, + const std::vector<std::string>& required_field_trials, DefaultValue default_value); + ~FeatureSwitch(); // Consider using ScopedOverride instead. void SetOverrideValue(OverrideValue value); @@ -92,7 +99,7 @@ class FeatureSwitch { const base::CommandLine* command_line_; const char* switch_name_; - const char* field_trial_name_; + std::vector<std::string> required_field_trials_; bool default_value_; OverrideValue override_value_; |