Remove modality of MULTI_VALUE choice types in AboutFlags

Previously the UI for multivalues in AboutFlags was confusing, since
you could "enable" or "disable" the feature, then select an option
from a combobox.  This change removes this modality, and instead, the
first option from the multivalue should be considered the "disabled"
version of the lab (we even assert that it has no command line flags).

This CL was originally http://codereview.chromium.org/6410082/ , but
cbentzel suggested we split it up into two CLs: this is the first of
them.

BUG=none
TEST=AboutFlagsTest.MultiValues

Review URL: http://codereview.chromium.org/6413037

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@74277 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 67 insertions, 34 deletions

diff --git a/chrome/browser/about_flags.cc b/chrome/browser/about_flags.cc
index e755ee9..6755aa2 100644
--- a/chrome/browser/about_flags.cc
+++ b/chrome/browser/about_flags.cc
@@ -62,9 +62,10 @@ const char kVerticalTabsExperimentName[] = "vertical-tabs";
 // distinct types of experiments:
 // . SINGLE_VALUE: experiment is either on or off. Use the SINGLE_VALUE_TYPE
 //   macro for this type supplying the command line to the macro.
-// . MULTI_VALUE: if enabled the command line of the selected choice is enabled.
-//   To specify this type of experiment use the macro MULTI_VALUE_TYPE supplying
-//   it the array of choices.
+// . MULTI_VALUE: a list of choices, the first of which should correspond to a
+//   deactivated state for this lab (i.e. no command line option).  To specify
+//   this type of experiment use the macro MULTI_VALUE_TYPE supplying it the
+//   array of choices.
 // See the documentation of Experiment for details on the fields.
 //
 // When adding a new choice, add it to the end of the list.
@@ -334,13 +335,35 @@ void AddInternalName(const Experiment& e, std::set<std::string>* names) {
   }
 }
 
+// Confirms that an experiment is valid, used in a DCHECK in
+// SanitizeList below.
+bool ValidateExperiment(const Experiment& e) {
+  switch (e.type) {
+    case Experiment::SINGLE_VALUE:
+      DCHECK_EQ(0, e.num_choices);
+      DCHECK(!e.choices);
+      break;
+    case Experiment::MULTI_VALUE:
+      DCHECK_GT(e.num_choices, 0);
+      DCHECK(e.choices);
+      DCHECK(e.choices[0].command_line);
+      DCHECK_EQ('\0', e.choices[0].command_line[0]);
+      break;
+    default:
+      NOTREACHED();
+  }
+  return true;
+}
+
 // Removes all experiments from prefs::kEnabledLabsExperiments that are
 // unknown, to prevent this list to become very long as experiments are added
 // and removed.
 void SanitizeList(PrefService* prefs) {
   std::set<std::string> known_experiments;
-  for (size_t i = 0; i < num_experiments; ++i)
+  for (size_t i = 0; i < num_experiments; ++i) {
+    DCHECK(ValidateExperiment(experiments[i]));
     AddInternalName(experiments[i], &known_experiments);
+  }
 
   std::set<std::string> enabled_experiments;
   GetEnabledFlags(prefs, &enabled_experiments);
@@ -386,10 +409,8 @@ void GetSanitizedEnabledFlagsForCurrentPlatform(
 }
 
 // Returns the Value representing the choice data in the specified experiment.
-// If one of the choices is enabled |is_one_selected| is set to true.
 Value* CreateChoiceData(const Experiment& experiment,
-                        const std::set<std::string>& enabled_experiments,
-                        bool* is_one_selected) {
+                        const std::set<std::string>& enabled_experiments) {
   DCHECK(experiment.type == Experiment::MULTI_VALUE);
   ListValue* result = new ListValue;
   for (int i = 0; i < experiment.num_choices; ++i) {
@@ -399,10 +420,7 @@ Value* CreateChoiceData(const Experiment& experiment,
     value->SetString("description",
                      l10n_util::GetStringUTF16(choice.description_id));
     value->SetString("internal_name", name);
-    bool is_selected = enabled_experiments.count(name) > 0;
-    if (is_selected)
-      *is_one_selected = true;
-    value->SetBoolean("selected", is_selected);
+    value->SetBoolean("selected", enabled_experiments.count(name) > 0);
     result->Append(value);
   }
   return result;
@@ -434,14 +452,19 @@ ListValue* GetFlagsExperimentsData(PrefService* prefs) {
                     l10n_util::GetStringUTF16(
                         experiment.visible_description_id));
 
-    bool enabled = enabled_experiments.count(experiment.internal_name) > 0;
-
-    if (experiment.type == Experiment::MULTI_VALUE) {
-      data->Set("choices", CreateChoiceData(experiment, enabled_experiments,
-                                            &enabled));
+    switch (experiment.type) {
+      case Experiment::SINGLE_VALUE:
+        data->SetBoolean(
+            "enabled",
+            enabled_experiments.count(experiment.internal_name) > 0);
+        break;
+      case Experiment::MULTI_VALUE:
+        data->Set("choices", CreateChoiceData(experiment, enabled_experiments));
+        break;
+      default:
+        NOTREACHED();
     }
 
-    data->SetBoolean("enabled", enabled);
     experiments_data->Append(data);
   }
   return experiments_data;
@@ -551,13 +574,16 @@ void FlagsState::SetExperimentEnabled(
     // We're being asked to enable a multi-choice experiment. Disable the
     // currently selected choice.
     DCHECK_NE(at_index, 0u);
-    SetExperimentEnabled(prefs, internal_name.substr(0, at_index), false);
+    const std::string experiment_name = internal_name.substr(0, at_index);
+    SetExperimentEnabled(prefs, experiment_name, false);
 
-    // And enable the new choice.
-    std::set<std::string> enabled_experiments;
-    GetSanitizedEnabledFlags(prefs, &enabled_experiments);
-    enabled_experiments.insert(internal_name);
-    SetEnabledFlags(prefs, enabled_experiments);
+    // And enable the new choice, if it is not the default first choice.
+    if (internal_name != experiment_name + "@0") {
+      std::set<std::string> enabled_experiments;
+      GetSanitizedEnabledFlags(prefs, &enabled_experiments);
+      enabled_experiments.insert(internal_name);
+      SetEnabledFlags(prefs, enabled_experiments);
+    }
     return;
   }
 
diff --git a/chrome/browser/about_flags.h b/chrome/browser/about_flags.h
index daa76df..1130b8c 100644
--- a/chrome/browser/about_flags.h
+++ b/chrome/browser/about_flags.h
@@ -29,9 +29,10 @@ struct Experiment {
     SINGLE_VALUE,
 
     // The experiment has multiple values only one of which is ever enabled.
-    // For MULTI_VALUE experiments the command_line of the Experiment is not
-    // used. If the experiment is enabled the command line of the selected
-    // Choice is enabled.
+    // The first of the values should correspond to a deactivated state for this
+    // lab (i.e. no command line option). For MULTI_VALUE experiments the
+    // command_line of the Experiment is not used. If the experiment is enabled
+    // the command line of the selected Choice is enabled.
     MULTI_VALUE,
   };
 
diff --git a/chrome/browser/about_flags_unittest.cc b/chrome/browser/about_flags_unittest.cc
index c3bbe95..f5c7242 100644
--- a/chrome/browser/about_flags_unittest.cc
+++ b/chrome/browser/about_flags_unittest.cc
@@ -26,6 +26,7 @@ const char kMultiSwitch2[] = "multi_switch2";
 namespace about_flags {
 
 const Experiment::Choice kMultiChoices[] = {
+  { IDS_PRODUCT_NAME, "" },
   { IDS_PRODUCT_NAME, kMultiSwitch1 },
   { IDS_PRODUCT_NAME, kMultiSwitch2 },
 };
@@ -222,22 +223,21 @@ TEST_F(AboutFlagsTest, PersistAndPrune) {
   EXPECT_EQ(arraysize(kExperiments) - 1, switch_prefs->GetSize());
 }
 
-// Tests enabling multi-value type experiments.
+// Tests multi-value type experiments.
 TEST_F(AboutFlagsTest, MultiValues) {
-  // Enable the multi value experiment, which should enable the first choice.
-  SetExperimentEnabled(&prefs_, kFlags4, true);
+  // Initially, the first "deactivated" option of the multi experiment should
+  // be set.
   {
     CommandLine command_line(CommandLine::NO_PROGRAM);
     ConvertFlagsToSwitches(&prefs_, &command_line);
-    EXPECT_TRUE(command_line.HasSwitch(kMultiSwitch1));
+    EXPECT_FALSE(command_line.HasSwitch(kMultiSwitch1));
     EXPECT_FALSE(command_line.HasSwitch(kMultiSwitch2));
   }
 
-  // Enable the 2nd choice of the multi-value, which should disable the first
-  // choice.
+  // Enable the 2nd choice of the multi-value.
   SetExperimentEnabled(&prefs_, std::string(kFlags4) +
                        std::string(testing::kMultiSeparator) +
-                       base::IntToString(1), true);
+                       base::IntToString(2), true);
   {
     CommandLine command_line(CommandLine::NO_PROGRAM);
     ConvertFlagsToSwitches(&prefs_, &command_line);
@@ -246,7 +246,9 @@ TEST_F(AboutFlagsTest, MultiValues) {
   }
 
   // Disable the multi-value experiment.
-  SetExperimentEnabled(&prefs_, kFlags4, false);
+  SetExperimentEnabled(&prefs_, std::string(kFlags4) +
+                       std::string(testing::kMultiSeparator) +
+                       base::IntToString(0), true);
   {
     CommandLine command_line(CommandLine::NO_PROGRAM);
     ConvertFlagsToSwitches(&prefs_, &command_line);
diff --git a/chrome/browser/resources/flags.html b/chrome/browser/resources/flags.html
index 5090084..203f425 100644
--- a/chrome/browser/resources/flags.html
+++ b/chrome/browser/resources/flags.html
@@ -176,6 +176,7 @@ var flagsExperimentsDataFormat = {
       'internal_name': 'Experiment ID string',
       'name': 'Experiment Name',
       'description': 'description',
+      /* enabled is only set if the experiment is single valued */
       'enabled': true,
       /* choices is only set if the experiment has multiple values */
       'choices': [
@@ -313,6 +314,9 @@ document.addEventListener('DOMContentLoaded', requestFlagsExperimentsData);
             </div>
           </div>
           <div class="experiment-actions">
+	    <!-- If enabled isn't set (i.e. in multi_type options),
+		 then both jsdisplay tests fail, and we get no
+		 rendering from this section. -->
             <span>
               <a
                 jsvalues=".internal_name:internal_name"