about:flags: Fix disabling experiments.

The problem was that the restart code would copy the switches added by about:flags to the restarted instance, so that e.g. --enable-expose-for-tabs would be passed on the the command line even though the experiment was disabled. Now the flags added by about:switches are explicitly removed before the restart.

Also wrap the switches added by about:flags with --start-flags-switches and --end-flags-switches switches. These unknown switches are ignored, but are useful to see on about:version.

Also add a bunch of unrelated unit tests.

BUG=56314
TEST=See bug.

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

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

7 files changed, 271 insertions, 29 deletions

diff --git a/chrome/browser/about_flags.cc b/chrome/browser/about_flags.cc
index 7824dee..3401cfb 100644
--- a/chrome/browser/about_flags.cc
+++ b/chrome/browser/about_flags.cc
@@ -11,6 +11,7 @@
 
 #include "app/l10n_util.h"
 #include "base/command_line.h"
+#include "base/singleton.h"
 #include "base/values.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/common/chrome_switches.h"
@@ -19,6 +20,8 @@
 
 namespace about_flags {
 
+namespace {
+
 enum { kOsMac = 1 << 0, kOsWin = 1 << 1, kOsLinux = 1 << 2 };
 
 unsigned kOsAll = kOsMac | kOsWin | kOsLinux;
@@ -165,6 +168,30 @@ const Experiment kExperiments[] = {
   }
 };
 
+// Stores and encapsulates the little state that about:flags has.
+class FlagsState {
+ public:
+  FlagsState() : needs_restart_(false) {}
+  void ConvertFlagsToSwitches(PrefService* prefs, CommandLine* command_line);
+  bool IsRestartNeededToCommitChanges();
+  void SetExperimentEnabled(
+    PrefService* prefs, const std::string& internal_name, bool enable);
+  void RemoveFlagsSwitches(
+      std::map<std::string, CommandLine::StringType>* switch_list);
+  void reset();
+
+  // Returns the singleton instance of this class
+  static FlagsState* instance() {
+    return Singleton<FlagsState>::get();
+  }
+
+ private:
+  bool needs_restart_;
+  std::set<std::string> flags_switches_;
+
+  DISALLOW_COPY_AND_ASSIGN(FlagsState);
+};
+
 // Extracts the list of enabled lab experiments from preferences and stores them
 // in a set.
 void GetEnabledFlags(const PrefService* prefs, std::set<std::string>* result) {
@@ -239,6 +266,8 @@ int GetCurrentPlatform() {
 #endif
 }
 
+}  // namespace
+
 bool IsEnabled() {
 #if defined(OS_CHROMEOS)
   // TODO(thakis): Port about:flags to chromeos -- http://crbug.com/57634
@@ -249,31 +278,7 @@ bool IsEnabled() {
 }
 
 void ConvertFlagsToSwitches(PrefService* prefs, CommandLine* command_line) {
-  if (!IsEnabled())
-    return;
-
-  if (command_line->HasSwitch(switches::kNoExperiments))
-    return;
-
-  std::set<std::string> enabled_experiments;
-  GetSanitizedEnabledFlags(prefs, &enabled_experiments);
-
-  std::map<std::string, const Experiment*> experiments;
-  for (size_t i = 0; i < arraysize(kExperiments); ++i)
-    experiments[kExperiments[i].internal_name] = &kExperiments[i];
-
-  for (std::set<std::string>::iterator it = enabled_experiments.begin();
-       it != enabled_experiments.end();
-       ++it) {
-    const std::string& experiment_name = *it;
-    std::map<std::string, const Experiment*>::iterator experiment =
-        experiments.find(experiment_name);
-    DCHECK(experiment != experiments.end());
-    if (experiment == experiments.end())
-      continue;
-
-    command_line->AppendSwitch(experiment->second->command_line);
-  }
+  FlagsState::instance()->ConvertFlagsToSwitches(prefs, command_line);
 }
 
 ListValue* GetFlagsExperimentsData(PrefService* prefs) {
@@ -303,14 +308,65 @@ ListValue* GetFlagsExperimentsData(PrefService* prefs) {
   return experiments_data;
 }
 
-static bool needs_restart_ = false;
-
 bool IsRestartNeededToCommitChanges() {
-  return needs_restart_;
+  return FlagsState::instance()->IsRestartNeededToCommitChanges();
 }
 
 void SetExperimentEnabled(
     PrefService* prefs, const std::string& internal_name, bool enable) {
+  FlagsState::instance()->SetExperimentEnabled(prefs, internal_name, enable);
+}
+
+void RemoveFlagsSwitches(
+    std::map<std::string, CommandLine::StringType>* switch_list) {
+  FlagsState::instance()->RemoveFlagsSwitches(switch_list);
+}
+
+//////////////////////////////////////////////////////////////////////////////
+// FlagsState implementation.
+
+namespace {
+
+void FlagsState::ConvertFlagsToSwitches(
+    PrefService* prefs, CommandLine* command_line) {
+  if (!IsEnabled())
+    return;
+
+  if (command_line->HasSwitch(switches::kNoExperiments))
+    return;
+
+  std::set<std::string> enabled_experiments;
+  GetSanitizedEnabledFlags(prefs, &enabled_experiments);
+
+  std::map<std::string, const Experiment*> experiments;
+  for (size_t i = 0; i < arraysize(kExperiments); ++i)
+    experiments[kExperiments[i].internal_name] = &kExperiments[i];
+
+  command_line->AppendSwitch(switches::kFlagSwitchesBegin);
+  flags_switches_.insert(switches::kFlagSwitchesBegin);
+  for (std::set<std::string>::iterator it = enabled_experiments.begin();
+       it != enabled_experiments.end();
+       ++it) {
+    const std::string& experiment_name = *it;
+    std::map<std::string, const Experiment*>::iterator experiment =
+        experiments.find(experiment_name);
+    DCHECK(experiment != experiments.end());
+    if (experiment == experiments.end())
+      continue;
+
+    command_line->AppendSwitch(experiment->second->command_line);
+    flags_switches_.insert(experiment->second->command_line);
+  }
+  command_line->AppendSwitch(switches::kFlagSwitchesEnd);
+  flags_switches_.insert(switches::kFlagSwitchesEnd);
+}
+
+bool FlagsState::IsRestartNeededToCommitChanges() {
+  return needs_restart_;
+}
+
+void FlagsState::SetExperimentEnabled(
+    PrefService* prefs, const std::string& internal_name, bool enable) {
   needs_restart_ = true;
 
   std::set<std::string> enabled_experiments;
@@ -324,4 +380,26 @@ void SetExperimentEnabled(
   SetEnabledFlags(prefs, enabled_experiments);
 }
 
+void FlagsState::RemoveFlagsSwitches(
+    std::map<std::string, CommandLine::StringType>* switch_list) {
+  for (std::set<std::string>::const_iterator it = flags_switches_.begin();
+       it != flags_switches_.end();
+       ++it) {
+    switch_list->erase(*it);
+  }
+}
+
+void FlagsState::reset() {
+  needs_restart_ = false;
+  flags_switches_.clear();
+}
+
+} // namespace
+
+namespace testing {
+void ClearState() {
+  FlagsState::instance()->reset();
+}
+}  // namespace testing
+
 }  // namespace about_flags
diff --git a/chrome/browser/about_flags.h b/chrome/browser/about_flags.h
index fd6d8e4..ad6eefb 100644
--- a/chrome/browser/about_flags.h
+++ b/chrome/browser/about_flags.h
@@ -6,9 +6,11 @@
 #define CHROME_BROWSER_ABOUT_FLAGS_H_
 #pragma once
 
+#include <map>
 #include <string>
 
-class CommandLine;
+#include "base/command_line.h"
+
 class ListValue;
 class PrefService;
 
@@ -31,6 +33,16 @@ bool IsRestartNeededToCommitChanges();
 void SetExperimentEnabled(
     PrefService* prefs, const std::string& internal_name, bool enable);
 
+// Removes all switches that were added to a command line by a previous call to
+// |ConvertFlagsToSwitches()|.
+void RemoveFlagsSwitches(
+    std::map<std::string, CommandLine::StringType>* switch_list);
+
+namespace testing {
+// Clears internal global state, for unit tests.
+void ClearState();
+}  // namespace testing
+
 }  // namespace about_flags
 
 #endif  // CHROME_BROWSER_ABOUT_FLAGS_H_
diff --git a/chrome/browser/about_flags_unittest.cc b/chrome/browser/about_flags_unittest.cc
new file mode 100644
index 0000000..a18f10f
--- /dev/null
+++ b/chrome/browser/about_flags_unittest.cc
@@ -0,0 +1,141 @@
+// Copyright (c) 2010 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 "chrome/browser/about_flags.h"
+
+#include "chrome/common/chrome_switches.h"
+#include "chrome/common/pref_names.h"
+#include "chrome/test/testing_pref_service.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+// These two should be two arbitrary, but valid names from about_flags.cc.
+const char kFlags1[] = "remoting";
+const char kFlags2[] = "print-preview";
+
+// The command line flag corresponding to |kFlags1|.
+const char* const kFlag1CommandLine = switches::kEnableRemoting;
+
+namespace about_flags {
+
+class AboutFlagsTest : public ::testing::Test {
+ protected:
+  AboutFlagsTest() {
+    prefs_.RegisterListPref(prefs::kEnabledLabsExperiments);
+    testing::ClearState();
+  }
+
+  TestingPrefService prefs_;
+};
+
+TEST_F(AboutFlagsTest, ChangeNeedsRestart) {
+  if (!IsEnabled())
+    return;
+
+  EXPECT_FALSE(IsRestartNeededToCommitChanges());
+  SetExperimentEnabled(&prefs_, kFlags1, true);
+  EXPECT_TRUE(IsRestartNeededToCommitChanges());
+}
+
+TEST_F(AboutFlagsTest, AddTwoFlagsRemoveOne) {
+  if (!IsEnabled())
+    return;
+
+  // Add two experiments, check they're there.
+  SetExperimentEnabled(&prefs_, kFlags1, true);
+  SetExperimentEnabled(&prefs_, kFlags2, true);
+
+  ListValue* experiments_list = prefs_.GetMutableList(
+      prefs::kEnabledLabsExperiments);
+  ASSERT_TRUE(experiments_list != NULL);
+
+  ASSERT_EQ(2u, experiments_list->GetSize());
+
+  std::string s0;
+  ASSERT_TRUE(experiments_list->GetString(0, &s0));
+  std::string s1;
+  ASSERT_TRUE(experiments_list->GetString(1, &s1));
+
+  EXPECT_TRUE(s0 == kFlags1 || s1 == kFlags1);
+  EXPECT_TRUE(s0 == kFlags2 || s1 == kFlags2);
+
+  // Remove one experiment, check the other's still around.
+  SetExperimentEnabled(&prefs_, kFlags2, false);
+
+  experiments_list = prefs_.GetMutableList(prefs::kEnabledLabsExperiments);
+  ASSERT_TRUE(experiments_list != NULL);
+  ASSERT_EQ(1u, experiments_list->GetSize());
+  ASSERT_TRUE(experiments_list->GetString(0, &s0));
+  EXPECT_TRUE(s0 == kFlags1);
+}
+
+TEST_F(AboutFlagsTest, AddTwoFlagsRemoveBoth) {
+  if (!IsEnabled())
+    return;
+
+  // Add two experiments, check the pref exists.
+  SetExperimentEnabled(&prefs_, kFlags1, true);
+  SetExperimentEnabled(&prefs_, kFlags2, true);
+  ListValue* experiments_list = prefs_.GetMutableList(
+      prefs::kEnabledLabsExperiments);
+  ASSERT_TRUE(experiments_list != NULL);
+
+  // Remove both, the pref should have been removed completely.
+  SetExperimentEnabled(&prefs_, kFlags1, false);
+  SetExperimentEnabled(&prefs_, kFlags2, false);
+  experiments_list = prefs_.GetMutableList(prefs::kEnabledLabsExperiments);
+  EXPECT_TRUE(experiments_list == NULL || experiments_list->GetSize() == 0);
+}
+
+TEST_F(AboutFlagsTest, ConvertFlagsToSwitches) {
+  if (!IsEnabled())
+    return;
+
+  SetExperimentEnabled(&prefs_, kFlags1, true);
+
+  CommandLine command_line(CommandLine::ARGUMENTS_ONLY);
+  command_line.AppendSwitch("foo");
+
+  EXPECT_TRUE(command_line.HasSwitch("foo"));
+  EXPECT_FALSE(command_line.HasSwitch(kFlag1CommandLine));
+
+  ConvertFlagsToSwitches(&prefs_, &command_line);
+
+  EXPECT_TRUE(command_line.HasSwitch("foo"));
+  EXPECT_TRUE(command_line.HasSwitch(kFlag1CommandLine));
+}
+
+TEST_F(AboutFlagsTest, RemoveFlagSwitches) {
+  if (!IsEnabled())
+    return;
+
+  std::map<std::string, CommandLine::StringType> switch_list;
+  switch_list[kFlag1CommandLine] = CommandLine::StringType();
+  switch_list[switches::kFlagSwitchesBegin] = CommandLine::StringType();
+  switch_list[switches::kFlagSwitchesEnd] = CommandLine::StringType();
+  switch_list["foo"] = CommandLine::StringType();
+
+  SetExperimentEnabled(&prefs_, kFlags1, true);
+
+  // This shouldn't do anything before ConvertFlagsToSwitches() wasn't called.
+  RemoveFlagsSwitches(&switch_list);
+  ASSERT_EQ(4u, switch_list.size());
+  EXPECT_TRUE(switch_list.find(kFlag1CommandLine) != switch_list.end());
+  EXPECT_TRUE(switch_list.find(switches::kFlagSwitchesBegin) !=
+              switch_list.end());
+  EXPECT_TRUE(switch_list.find(switches::kFlagSwitchesEnd) !=
+              switch_list.end());
+  EXPECT_TRUE(switch_list.find("foo") != switch_list.end());
+
+  // Call ConvertFlagsToSwitches(), then RemoveFlagsSwitches() again.
+  CommandLine command_line(CommandLine::ARGUMENTS_ONLY);
+  command_line.AppendSwitch("foo");
+  ConvertFlagsToSwitches(&prefs_, &command_line);
+  RemoveFlagsSwitches(&switch_list);
+
+  // Now the about:flags-related switch should have been removed.
+  ASSERT_EQ(1u, switch_list.size());
+  EXPECT_TRUE(switch_list.find("foo") != switch_list.end());
+}
+
+}  // namespace about_flags
diff --git a/chrome/browser/browser_shutdown.cc b/chrome/browser/browser_shutdown.cc
index 9e7d411..98e0af7 100644
--- a/chrome/browser/browser_shutdown.cc
+++ b/chrome/browser/browser_shutdown.cc
@@ -18,6 +18,7 @@
 #include "base/thread.h"
 #include "base/time.h"
 #include "build/build_config.h"
+#include "chrome/browser/about_flags.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/browser_thread.h"
 #include "chrome/browser/dom_ui/chrome_url_data_manager.h"
@@ -182,6 +183,7 @@ void Shutdown() {
     std::map<std::string, CommandLine::StringType> switches =
         old_cl.GetSwitches();
     // Remove the switches that shouldn't persist across restart.
+    about_flags::RemoveFlagsSwitches(&switches);
     switches::RemoveSwitchesForAutostart(&switches);
     // Append the old switches to the new command line.
     for (std::map<std::string, CommandLine::StringType>::const_iterator i =
diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi
index b34daff..7183c5b5 100644
--- a/chrome/chrome_tests.gypi
+++ b/chrome/chrome_tests.gypi
@@ -974,6 +974,7 @@
         'app/breakpad_mac_stubs.mm',
         'app/chrome_dll.rc',
         # All unittests in browser, common, renderer and service.
+        'browser/about_flags_unittest.cc',
         'browser/accessibility/browser_accessibility_win_unittest.cc',
         'browser/app_controller_mac_unittest.mm',
         'browser/autocomplete_history_manager_unittest.cc',
diff --git a/chrome/common/chrome_switches.cc b/chrome/common/chrome_switches.cc
index 65e7d9a..62b2690 100644
--- a/chrome/common/chrome_switches.cc
+++ b/chrome/common/chrome_switches.cc
@@ -587,6 +587,12 @@ const char kExtensionProcess[]              = "extension";
 // Frequency in seconds for Extensions auto-update.
 const char kExtensionsUpdateFrequency[]     = "extensions-update-frequency";
 
+// These two flags are added around the switches about:flags adds to the
+// command line. This is useful to see which switches were added by about:flags
+// on about:version. They don't have any effect.
+const char kFlagSwitchesBegin[]             = "flag-switches-begin";
+const char kFlagSwitchesEnd[]               = "flag-switches-end";
+
 // Alternative feedback server to use when submitting user feedback
 const char kFeedbackServer[]                = "feedback-server";
 
diff --git a/chrome/common/chrome_switches.h b/chrome/common/chrome_switches.h
index 9654e79..9cb8043 100644
--- a/chrome/common/chrome_switches.h
+++ b/chrome/common/chrome_switches.h
@@ -178,6 +178,8 @@ extern const char kExperimentalSpellcheckerFeatures[];
 extern const char kExplicitlyAllowedPorts[];
 extern const char kExtensionProcess[];
 extern const char kExtensionsUpdateFrequency[];
+extern const char kFlagSwitchesBegin[];
+extern const char kFlagSwitchesEnd[];
 extern const char kFeedbackServer[];
 extern const char kFileDescriptorLimit[];
 extern const char kFirstRun[];