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

extensions/common/feature_switch.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "extensions/common/feature_switch.h"
 
 #include "base/command_line.h"
 #include "base/lazy_instance.h"
 #include "base/metrics/field_trial.h"
 #include "base/strings/string_util.h"
 #include "build/build_config.h"
 #include "extensions/common/switches.h"
 
 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()
       : easy_off_store_install(nullptr, FeatureSwitch::DEFAULT_DISABLED),
         force_dev_mode_highlighting(switches::kForceDevModeHighlighting,
                                     FeatureSwitch::DEFAULT_DISABLED),
         prompt_for_external_extensions(
 #if defined(CHROMIUM_BUILD)
             switches::kPromptForExternalExtensions,
 #else
@@ skipping 11 matching lines @@
                                   kExtensionActionRedesignExperiment,
                                   FeatureSwitch::DEFAULT_DISABLED),
         extension_action_redesign_override(switches::kExtensionActionRedesign,
                                            FeatureSwitch::DEFAULT_ENABLED),
         scripts_require_action(switches::kScriptsRequireAction,
                                FeatureSwitch::DEFAULT_DISABLED),
         embedded_extension_options(switches::kEmbeddedExtensionOptions,
                                    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) {
   }
 
   // Enables extensions to be easily installed from sites other than the web
   // store.
   FeatureSwitch easy_off_store_install;
 
   FeatureSwitch force_dev_mode_highlighting;
 
   // Should we prompt the user before allowing external extensions to install?
   // Default is yes.
   FeatureSwitch prompt_for_external_extensions;
 
   FeatureSwitch error_console;
   FeatureSwitch enable_override_bookmarks_ui;
   FeatureSwitch extension_action_redesign;
   FeatureSwitch extension_action_redesign_override;
   FeatureSwitch scripts_require_action;
   FeatureSwitch embedded_extension_options;
   FeatureSwitch trace_app_source;
   FeatureSwitch media_router;
   FeatureSwitch media_router_with_cast_extension;
 };
 
 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() {
   return &g_common_switches.Get().force_dev_mode_highlighting;
 }
 FeatureSwitch* FeatureSwitch::easy_off_store_install() {
   return &g_common_switches.Get().easy_off_store_install;
 }
 FeatureSwitch* FeatureSwitch::prompt_for_external_extensions() {
   return &g_common_switches.Get().prompt_for_external_extensions;
 }
 FeatureSwitch* FeatureSwitch::error_console() {
   return &g_common_switches.Get().error_console;
 }
 FeatureSwitch* FeatureSwitch::enable_override_bookmarks_ui() {
   return &g_common_switches.Get().enable_override_bookmarks_ui;
 }
 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
   // 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;
 }
 FeatureSwitch* FeatureSwitch::scripts_require_action() {
   return &g_common_switches.Get().scripts_require_action;
 }
 FeatureSwitch* FeatureSwitch::embedded_extension_options() {
   return &g_common_switches.Get().embedded_extension_options;
 }
@@ skipping 24 matching lines @@
     : FeatureSwitch(base::CommandLine::ForCurrentProcess(),
                     switch_name,
                     default_value) {}
 
 FeatureSwitch::FeatureSwitch(const char* switch_name,
                              const char* field_trial_name,
                              DefaultValue default_value)
     : FeatureSwitch(base::CommandLine::ForCurrentProcess(),
                     switch_name,
                     field_trial_name,
+                    std::vector<const char*>(),
+                    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,
                              DefaultValue default_value)
-    : FeatureSwitch(command_line, switch_name, nullptr, 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,
-                             DefaultValue 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;
 
   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;
 
   if (switch_value == "0")
     return false;
 
   if (!default_value_ && command_line_->HasSwitch(GetLegacyEnableFlag()))

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

unrelated to this change, but these should really just be:
if (command_line_->HasSwitch())

Otherwise, we could improperly ignore an enable/disable switch if there is a
field trial active.

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

Ok, I will file a bug for it.

     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")
+    if (group_name == "Enabled" && DependentFieldTrialsEnabled())
       return true;
     if (group_name == "Disabled")
       return false;
   }
 
   return default_value_;
 }
 
 std::string FeatureSwitch::GetLegacyEnableFlag() const {
   DCHECK(switch_name_);
   return std::string("enable-") + switch_name_;
 }
 
 std::string FeatureSwitch::GetLegacyDisableFlag() const {
   DCHECK(switch_name_);
   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;
 }
 
 FeatureSwitch::OverrideValue FeatureSwitch::GetOverrideValue() const {
   return override_value_;
 }
 
 }  // namespace extensions