[Feature Switch][Media Router] Add required field trials to MR switch.

Add a generic required_field_trials to FeatureSwitch that takes the
place of a singular field_trial_name. When deciding whether to enable or
disable a feature with field trials, all field trials will be queried
(though short circuiting can occur):
- If user is in "Enabled" group for all field trials, then enable
feature.
- If user is in "Disabled group for any field trials, then disable
feature.
- Otherwise, fall back to default value.

Change the MR FeatureSwitches to depend on the EAR field trial. Per
discussion, we will change the interpretation of MR field trial to
be "X% of users that have EAR field trial enabled".

BUG=541315

Review URL: https://codereview.chromium.org/1680823004

Cr-Commit-Position: refs/heads/master@{#374834}

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());
+}

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_;

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_;