Owner flags storage should not save the flags names but the actual switches.

These are directly applied by the session manager which can not resolve them. 

It fixed an issue with the flags stored in the device settings being the flag names and not the actual switches as is required by the session manager causing the flags not to be applied to the login screen if the flag and the switch don't happen to coincide and to browser restarts even for the owner which should not happen.

BUG=280935
TEST=Unit tests + manual testing as described in the bug.

Review URL: https://chromiumcodereview.appspot.com/23983031

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

6 files changed, 63 insertions, 32 deletions

diff --git a/chrome/browser/about_flags.cc b/chrome/browser/about_flags.cc
index 89a478d..0058e2c 100644
--- a/chrome/browser/about_flags.cc
+++ b/chrome/browser/about_flags.cc
@@ -1709,7 +1709,8 @@ class FlagsState {
  public:
   FlagsState() : needs_restart_(false) {}
   void ConvertFlagsToSwitches(FlagsStorage* flags_storage,
-                              CommandLine* command_line);
+                              CommandLine* command_line,
+                              SentinelsMode sentinels);
   bool IsRestartNeededToCommitChanges();
   void SetExperimentEnabled(
       FlagsStorage* flags_storage,
@@ -1875,9 +1876,11 @@ string16 Experiment::DescriptionForChoice(int index) const {
 }
 
 void ConvertFlagsToSwitches(FlagsStorage* flags_storage,
-                            CommandLine* command_line) {
+                            CommandLine* command_line,
+                            SentinelsMode sentinels) {
   FlagsState::GetInstance()->ConvertFlagsToSwitches(flags_storage,
-                                                    command_line);
+                                                    command_line,
+                                                    sentinels);
 }
 
 bool AreSwitchesIdenticalToCurrentCommandLine(
@@ -2013,8 +2016,9 @@ void SetFlagToSwitchMapping(const std::string& key,
   (*name_to_switch_map)[key] = std::make_pair(switch_name, switch_value);
 }
 
-void FlagsState::ConvertFlagsToSwitches(
-    FlagsStorage* flags_storage, CommandLine* command_line) {
+void FlagsState::ConvertFlagsToSwitches(FlagsStorage* flags_storage,
+                                        CommandLine* command_line,
+                                        SentinelsMode sentinels) {
   if (command_line->HasSwitch(switches::kNoExperiments))
     return;
 
@@ -2047,10 +2051,12 @@ void FlagsState::ConvertFlagsToSwitches(
     }
   }
 
-  command_line->AppendSwitch(switches::kFlagSwitchesBegin);
-  flags_switches_.insert(
-      std::pair<std::string, std::string>(switches::kFlagSwitchesBegin,
-                                          std::string()));
+  if (sentinels == kAddSentinels) {
+    command_line->AppendSwitch(switches::kFlagSwitchesBegin);
+    flags_switches_.insert(
+        std::pair<std::string, std::string>(switches::kFlagSwitchesBegin,
+                                            std::string()));
+  }
   for (std::set<std::string>::iterator it = enabled_experiments.begin();
        it != enabled_experiments.end();
        ++it) {
@@ -2070,10 +2076,12 @@ void FlagsState::ConvertFlagsToSwitches(
                                     switch_and_value_pair.second);
     flags_switches_[switch_and_value_pair.first] = switch_and_value_pair.second;
   }
-  command_line->AppendSwitch(switches::kFlagSwitchesEnd);
-  flags_switches_.insert(
-      std::pair<std::string, std::string>(switches::kFlagSwitchesEnd,
-                                          std::string()));
+  if (sentinels == kAddSentinels) {
+    command_line->AppendSwitch(switches::kFlagSwitchesEnd);
+    flags_switches_.insert(
+        std::pair<std::string, std::string>(switches::kFlagSwitchesEnd,
+                                            std::string()));
+  }
 }
 
 bool FlagsState::IsRestartNeededToCommitChanges() {
diff --git a/chrome/browser/about_flags.h b/chrome/browser/about_flags.h
index 5d67e97..8ed2425 100644
--- a/chrome/browser/about_flags.h
+++ b/chrome/browser/about_flags.h
@@ -105,10 +105,15 @@ struct Experiment {
   string16 DescriptionForChoice(int index) const;
 };
 
+// A flag controlling the behavior of the |ConvertFlagsToSwitches| function -
+// whether it should add the sentinel switches around flags.
+enum SentinelsMode { kNoSentinels, kAddSentinels };
+
 // Reads the Labs |prefs| (called "Labs" for historical reasons) and adds the
 // commandline flags belonging to the active experiments to |command_line|.
 void ConvertFlagsToSwitches(FlagsStorage* flags_storage,
-                            CommandLine* command_line);
+                            CommandLine* command_line,
+                            SentinelsMode sentinels);
 
 // Compares a set of switches of the two provided command line objects and
 // returns true if they are the same and false otherwise.
diff --git a/chrome/browser/about_flags_unittest.cc b/chrome/browser/about_flags_unittest.cc
index 34219c3..a10bc35 100644
--- a/chrome/browser/about_flags_unittest.cc
+++ b/chrome/browser/about_flags_unittest.cc
@@ -217,10 +217,20 @@ TEST_F(AboutFlagsTest, ConvertFlagsToSwitches) {
   EXPECT_TRUE(command_line.HasSwitch("foo"));
   EXPECT_FALSE(command_line.HasSwitch(kSwitch1));
 
-  ConvertFlagsToSwitches(&flags_storage_, &command_line);
+  ConvertFlagsToSwitches(&flags_storage_, &command_line, kAddSentinels);
 
   EXPECT_TRUE(command_line.HasSwitch("foo"));
   EXPECT_TRUE(command_line.HasSwitch(kSwitch1));
+  EXPECT_TRUE(command_line.HasSwitch(switches::kFlagSwitchesBegin));
+  EXPECT_TRUE(command_line.HasSwitch(switches::kFlagSwitchesEnd));
+
+  CommandLine command_line2(CommandLine::NO_PROGRAM);
+
+  ConvertFlagsToSwitches(&flags_storage_, &command_line2, kNoSentinels);
+
+  EXPECT_TRUE(command_line2.HasSwitch(kSwitch1));
+  EXPECT_FALSE(command_line2.HasSwitch(switches::kFlagSwitchesBegin));
+  EXPECT_FALSE(command_line2.HasSwitch(switches::kFlagSwitchesEnd));
 }
 
 TEST_F(AboutFlagsTest, CompareSwitchesToCurrentCommandLine) {
@@ -230,12 +240,12 @@ TEST_F(AboutFlagsTest, CompareSwitchesToCurrentCommandLine) {
   command_line.AppendSwitch("foo");
 
   CommandLine new_command_line(CommandLine::NO_PROGRAM);
-  ConvertFlagsToSwitches(&flags_storage_, &new_command_line);
+  ConvertFlagsToSwitches(&flags_storage_, &new_command_line, kAddSentinels);
 
   EXPECT_FALSE(AreSwitchesIdenticalToCurrentCommandLine(new_command_line,
                                                         command_line));
 
-  ConvertFlagsToSwitches(&flags_storage_, &command_line);
+  ConvertFlagsToSwitches(&flags_storage_, &command_line, kAddSentinels);
 
   EXPECT_TRUE(AreSwitchesIdenticalToCurrentCommandLine(new_command_line,
                                                        command_line));
@@ -245,7 +255,7 @@ TEST_F(AboutFlagsTest, CompareSwitchesToCurrentCommandLine) {
   SetExperimentEnabled(&flags_storage_, kFlags2, true);
 
   CommandLine another_command_line(CommandLine::NO_PROGRAM);
-  ConvertFlagsToSwitches(&flags_storage_, &another_command_line);
+  ConvertFlagsToSwitches(&flags_storage_, &another_command_line, kAddSentinels);
 
   EXPECT_FALSE(AreSwitchesIdenticalToCurrentCommandLine(new_command_line,
                                                         another_command_line));
@@ -273,7 +283,7 @@ TEST_F(AboutFlagsTest, RemoveFlagSwitches) {
   // Call ConvertFlagsToSwitches(), then RemoveFlagsSwitches() again.
   CommandLine command_line(CommandLine::NO_PROGRAM);
   command_line.AppendSwitch("foo");
-  ConvertFlagsToSwitches(&flags_storage_, &command_line);
+  ConvertFlagsToSwitches(&flags_storage_, &command_line, kAddSentinels);
   RemoveFlagsSwitches(&switch_list);
 
   // Now the about:flags-related switch should have been removed.
@@ -292,7 +302,7 @@ TEST_F(AboutFlagsTest, PersistAndPrune) {
 
   // Convert the flags to switches. Experiment 3 shouldn't be among the switches
   // as it is not applicable to the current platform.
-  ConvertFlagsToSwitches(&flags_storage_, &command_line);
+  ConvertFlagsToSwitches(&flags_storage_, &command_line, kAddSentinels);
   EXPECT_TRUE(command_line.HasSwitch(kSwitch1));
   EXPECT_FALSE(command_line.HasSwitch(kSwitch3));
 
@@ -320,7 +330,7 @@ TEST_F(AboutFlagsTest, CheckValues) {
   EXPECT_FALSE(command_line.HasSwitch(kSwitch2));
 
   // Convert the flags to switches.
-  ConvertFlagsToSwitches(&flags_storage_, &command_line);
+  ConvertFlagsToSwitches(&flags_storage_, &command_line, kAddSentinels);
   EXPECT_TRUE(command_line.HasSwitch(kSwitch1));
   EXPECT_EQ(std::string(), command_line.GetSwitchValueASCII(kSwitch1));
   EXPECT_TRUE(command_line.HasSwitch(kSwitch2));
@@ -375,7 +385,7 @@ TEST_F(AboutFlagsTest, MultiValues) {
   // be set.
   {
     CommandLine command_line(CommandLine::NO_PROGRAM);
-    ConvertFlagsToSwitches(&flags_storage_, &command_line);
+    ConvertFlagsToSwitches(&flags_storage_, &command_line, kAddSentinels);
     EXPECT_FALSE(command_line.HasSwitch(kMultiSwitch1));
     EXPECT_FALSE(command_line.HasSwitch(kMultiSwitch2));
   }
@@ -384,7 +394,7 @@ TEST_F(AboutFlagsTest, MultiValues) {
   SetExperimentEnabled(&flags_storage_, experiment.NameForChoice(2), true);
   {
     CommandLine command_line(CommandLine::NO_PROGRAM);
-    ConvertFlagsToSwitches(&flags_storage_, &command_line);
+    ConvertFlagsToSwitches(&flags_storage_, &command_line, kAddSentinels);
     EXPECT_FALSE(command_line.HasSwitch(kMultiSwitch1));
     EXPECT_TRUE(command_line.HasSwitch(kMultiSwitch2));
     EXPECT_EQ(std::string(kValueForMultiSwitch2),
@@ -395,7 +405,7 @@ TEST_F(AboutFlagsTest, MultiValues) {
   SetExperimentEnabled(&flags_storage_, experiment.NameForChoice(0), true);
   {
     CommandLine command_line(CommandLine::NO_PROGRAM);
-    ConvertFlagsToSwitches(&flags_storage_, &command_line);
+    ConvertFlagsToSwitches(&flags_storage_, &command_line, kAddSentinels);
     EXPECT_FALSE(command_line.HasSwitch(kMultiSwitch1));
     EXPECT_FALSE(command_line.HasSwitch(kMultiSwitch2));
   }
@@ -408,7 +418,7 @@ TEST_F(AboutFlagsTest, EnableDisableValues) {
   // Nothing selected.
   {
     CommandLine command_line(CommandLine::NO_PROGRAM);
-    ConvertFlagsToSwitches(&flags_storage_, &command_line);
+    ConvertFlagsToSwitches(&flags_storage_, &command_line, kAddSentinels);
     EXPECT_FALSE(command_line.HasSwitch(kSwitch1));
     EXPECT_FALSE(command_line.HasSwitch(kSwitch2));
   }
@@ -417,7 +427,7 @@ TEST_F(AboutFlagsTest, EnableDisableValues) {
   SetExperimentEnabled(&flags_storage_, experiment.NameForChoice(1), true);
   {
     CommandLine command_line(CommandLine::NO_PROGRAM);
-    ConvertFlagsToSwitches(&flags_storage_, &command_line);
+    ConvertFlagsToSwitches(&flags_storage_, &command_line, kAddSentinels);
     EXPECT_TRUE(command_line.HasSwitch(kSwitch1));
     EXPECT_FALSE(command_line.HasSwitch(kSwitch2));
     EXPECT_EQ(kEnableDisableValue1, command_line.GetSwitchValueASCII(kSwitch1));
@@ -427,7 +437,7 @@ TEST_F(AboutFlagsTest, EnableDisableValues) {
   SetExperimentEnabled(&flags_storage_, experiment.NameForChoice(2), true);
   {
     CommandLine command_line(CommandLine::NO_PROGRAM);
-    ConvertFlagsToSwitches(&flags_storage_, &command_line);
+    ConvertFlagsToSwitches(&flags_storage_, &command_line, kAddSentinels);
     EXPECT_FALSE(command_line.HasSwitch(kSwitch1));
     EXPECT_TRUE(command_line.HasSwitch(kSwitch2));
     EXPECT_EQ(kEnableDisableValue2, command_line.GetSwitchValueASCII(kSwitch2));
@@ -437,7 +447,7 @@ TEST_F(AboutFlagsTest, EnableDisableValues) {
   SetExperimentEnabled(&flags_storage_, experiment.NameForChoice(0), true);
   {
     CommandLine command_line(CommandLine::NO_PROGRAM);
-    ConvertFlagsToSwitches(&flags_storage_, &command_line);
+    ConvertFlagsToSwitches(&flags_storage_, &command_line, kAddSentinels);
     EXPECT_FALSE(command_line.HasSwitch(kMultiSwitch1));
     EXPECT_FALSE(command_line.HasSwitch(kMultiSwitch2));
   }
diff --git a/chrome/browser/chrome_browser_main.cc b/chrome/browser/chrome_browser_main.cc
index d2b0169..12c6e95 100644
--- a/chrome/browser/chrome_browser_main.cc
+++ b/chrome/browser/chrome_browser_main.cc
@@ -851,7 +851,8 @@ int ChromeBrowserMainParts::PreCreateThreadsImpl() {
     about_flags::PrefServiceFlagsStorage flags_storage_(
         g_browser_process->local_state());
     about_flags::ConvertFlagsToSwitches(&flags_storage_,
-                                        CommandLine::ForCurrentProcess());
+                                        CommandLine::ForCurrentProcess(),
+                                        about_flags::kAddSentinels);
   }
 #endif
 
diff --git a/chrome/browser/chromeos/login/login_utils.cc b/chrome/browser/chromeos/login/login_utils.cc
index 552b5af..5c220fc 100644
--- a/chrome/browser/chromeos/login/login_utils.cc
+++ b/chrome/browser/chromeos/login/login_utils.cc
@@ -251,7 +251,8 @@ void LoginUtilsImpl::DoBrowserLaunch(Profile* profile,
 
   CommandLine user_flags(CommandLine::NO_PROGRAM);
   about_flags::PrefServiceFlagsStorage flags_storage_(profile->GetPrefs());
-  about_flags::ConvertFlagsToSwitches(&flags_storage_, &user_flags);
+  about_flags::ConvertFlagsToSwitches(&flags_storage_, &user_flags,
+                                      about_flags::kAddSentinels);
   // Only restart if needed and if not going into managed mode.
   // Don't restart browser if it is not first profile in session.
   if (UserManager::Get()->GetLoggedInUsers().size() == 1 &&
diff --git a/chrome/browser/chromeos/settings/owner_flags_storage.cc b/chrome/browser/chromeos/settings/owner_flags_storage.cc
index 38c2ba9..9c21c31 100644
--- a/chrome/browser/chromeos/settings/owner_flags_storage.cc
+++ b/chrome/browser/chromeos/settings/owner_flags_storage.cc
@@ -4,8 +4,10 @@
 
 #include "chrome/browser/chromeos/settings/owner_flags_storage.h"
 
+#include "base/command_line.h"
 #include "base/prefs/pref_service.h"
 #include "base/values.h"
+#include "chrome/browser/about_flags.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
 #include "chrome/browser/chromeos/settings/cros_settings_names.h"
@@ -50,8 +52,12 @@ bool OwnerFlagsStorage::SetFlags(std::set<std::string> flags) {
 
   base::ListValue experiments_list;
 
-  for (std::set<std::string>::const_iterator it = flags.begin();
-       it != flags.end(); ++it) {
+  CommandLine command_line(CommandLine::NO_PROGRAM);
+  ::about_flags::ConvertFlagsToSwitches(this, &command_line,
+                                        ::about_flags::kNoSentinels);
+  CommandLine::StringVector switches = command_line.argv();
+  for (CommandLine::StringVector::const_iterator it = switches.begin() + 1;
+       it != switches.end(); ++it) {
     experiments_list.Append(new base::StringValue(*it));
   }
   cros_settings_->Set(kStartUpFlags, experiments_list);