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

extensions/common/feature_switch.cc

Index: extensions/common/feature_switch.cc
diff --git a/extensions/common/feature_switch.cc b/extensions/common/feature_switch.cc
index d8c6b98e5ddecbc56ca176c8cb6e657c82964a46..d66458578d70bcf958d774470abd698943586a0c 100644
--- a/extensions/common/feature_switch.cc
+++ b/extensions/common/feature_switch.cc
@@ -15,11 +15,18 @@ 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* kEnableMediaRouterExperimentDependencies[] = {
+    kExtensionActionRedesignExperiment};
+
 class CommonSwitches {
  public:
   CommonSwitches()
@@ -51,14 +58,21 @@ 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,
-                     FeatureSwitch::DEFAULT_DISABLED),
+        media_router(
+            kMediaRouterFlag,
+            kEnableMediaRouterExperiment,
+            std::vector<const char*>(
+                kEnableMediaRouterExperimentDependencies,
+                kEnableMediaRouterExperimentDependencies +
+                    arraysize(kEnableMediaRouterExperimentDependencies)),
+            FeatureSwitch::DEFAULT_DISABLED),
         media_router_with_cast_extension(
-            "media-router",
+            kMediaRouterFlag,
             kEnableMediaRouterWithCastExtensionExperiment,
+            std::vector<const char*>(
+                kEnableMediaRouterExperimentDependencies,
+                kEnableMediaRouterExperimentDependencies +
+                    arraysize(kEnableMediaRouterExperimentDependencies)),
             FeatureSwitch::DEFAULT_DISABLED) {
   }
 
@@ -86,6 +100,14 @@ class CommonSwitches {
 base::LazyInstance<CommonSwitches> g_common_switches =
     LAZY_INSTANCE_INITIALIZER;
 
+std::string GetSwitchValue(const base::CommandLine* command_line,
+                           const std::string& switch_name) {
+  std::string temp = command_line->GetSwitchValueASCII(switch_name);
+  std::string switch_value;
+  base::TrimWhitespaceASCII(temp, base::TRIM_ALL, &switch_value);
+  return switch_value;
+}
+
 }  // namespace
 
 FeatureSwitch* FeatureSwitch::force_dev_mode_highlighting() {
@@ -108,7 +130,8 @@ FeatureSwitch* FeatureSwitch::extension_action_redesign() {
   // is enabled. Should be removed when the toolbar redesign is used by default.
   // See crbug.com/514694
   // TODO(kmarshall): Remove this override.
-  if (media_router()->IsEnabled())
+  if (GetSwitchValue(base::CommandLine::ForCurrentProcess(),
+                     kMediaRouterFlag) == "1")

[Devlin, 2016/02/09 18:45:14]

This doesn't account for the case of, e.g., --enable-media-router (i.e., the
"legacy" enable flag).  Why is it incorrect to just check media_router()?

[imcheng, 2016/02/09 21:35:39]

Checking media_router() isn't correct for users with cast extension -- we should
be checking media_router_with_cast_extension() instead. But to do so requires
providing an extra parameter to this function, which isn't good as this function
is called from many places. Checking the flag should still return the right
behavior here (i.e., MR enabled implies EAR enabled).

--enable-media-router was removed in the same release the FeatureSwitch was
added. M48 and onward use --media-router=1, and MR isn't officially supported in
M49 anyway. Do we really need to check for that?

[Devlin, 2016/02/10 18:08:33]

So we need to check media_router() for most users but
media_router_with_extension() for users with the cast extension?  Do we want to
force enable EAR for users of media_router() without the cast extension?  Or
could we just make this check media_router_with_cast_extension() all the time,
and not enable it for media_router() (if the user is only in the MR trial group,
e.g.)?

     return &g_common_switches.Get().extension_action_redesign_override;
 
   return &g_common_switches.Get().extension_action_redesign;
@@ -153,23 +176,44 @@ FeatureSwitch::FeatureSwitch(const char* switch_name,
     : FeatureSwitch(base::CommandLine::ForCurrentProcess(),
                     switch_name,
                     field_trial_name,
+                    std::vector<const char*>(),
                     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 char* field_trial_name,

[Devlin, 2016/02/09 18:45:14]

Could we instead just always take a vector of required field trials that
includes the base field trial?  I think that works for all the logic (just
iterate over each field trial and make sure it's present) but I might be missing
an edge case.

[imcheng, 2016/02/09 21:35:39]

I think it depends on how you interpret the required field trials. If we don't
need to make a distinction between the base field trial and others then it's
probably fine to do that. The new logic will be:

Enable by experiment if user is in Enabled group for all experiments
Disable by experiment if user is in Disabled group for any experiments
Use default value otherwise (i.e., Control group in at least 1 experiment, with
no Disabled group)

+    const std::vector<const char*>& dependent_field_trials,
+    DefaultValue default_value)
+    : FeatureSwitch(base::CommandLine::ForCurrentProcess(),
+                    switch_name,
+                    field_trial_name,
+                    dependent_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,
+                    nullptr,
+                    std::vector<const char*>(),
+                    default_value) {}
+
+FeatureSwitch::FeatureSwitch(
+    const base::CommandLine* command_line,
+    const char* switch_name,
+    const char* field_trial_name,
+    const std::vector<const char*>& dependent_field_trials,
+    DefaultValue default_value)
     : command_line_(command_line),
       switch_name_(switch_name),
       field_trial_name_(field_trial_name),
+      dependent_field_trials_(dependent_field_trials),
       default_value_(default_value == DEFAULT_ENABLED),
       override_value_(OVERRIDE_NONE) {}
 
+FeatureSwitch::~FeatureSwitch() = default;

[Devlin, 2016/02/09 18:45:14]

we don't really use this style anywhere else that I've seen.  Prefer the
standard empty braces.

[imcheng, 2016/02/09 21:35:39]

Done.

+
 bool FeatureSwitch::IsEnabled() const {
   if (override_value_ != OVERRIDE_NONE)
     return override_value_ == OVERRIDE_ENABLED;
@@ -177,10 +221,7 @@ bool FeatureSwitch::IsEnabled() const {
   if (!switch_name_)
     return default_value_;
 
-  std::string temp = command_line_->GetSwitchValueASCII(switch_name_);
-  std::string switch_value;
-  base::TrimWhitespaceASCII(temp, base::TRIM_ALL, &switch_value);
-
+  std::string switch_value = GetSwitchValue(command_line_, switch_name_);
   if (switch_value == "1")
     return true;
 
@@ -196,7 +237,7 @@ bool FeatureSwitch::IsEnabled() const {
   if (field_trial_name_) {
     std::string group_name =
         base::FieldTrialList::FindFullName(field_trial_name_);
-    if (group_name == "Enabled")
+    if (group_name == "Enabled" && DependentFieldTrialsEnabled())
       return true;
     if (group_name == "Disabled")
       return false;
@@ -215,6 +256,16 @@ std::string FeatureSwitch::GetLegacyDisableFlag() const {
   return std::string("disable-") + switch_name_;
 }
 
+bool FeatureSwitch::DependentFieldTrialsEnabled() const {
+  for (const std::string& dependent_field_trial : dependent_field_trials_) {
+    std::string group_name =
+        base::FieldTrialList::FindFullName(dependent_field_trial);
+    if (group_name != "Enabled")
+      return false;
+  }
+  return true;
+}
+
 void FeatureSwitch::SetOverrideValue(OverrideValue override_value) {
   override_value_ = override_value;
 }