Prefix Chrome switches with the switch prefix.

Use CommandLine throughout rather than diddling with strings.
Use ScopedProcessInformation when calling CreateProcess.

BUG=151452
TEST=relaunch chrome via about:flags and hope not to see a tab navigated to "force-desktop".  also, hope that relaunching between desktop and metro works.

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

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

9 files changed, 215 insertions, 44 deletions

diff --git a/win8/delegate_execute/command_execute_impl.cc b/win8/delegate_execute/command_execute_impl.cc
index eeeae9b..4f57a82 100644
--- a/win8/delegate_execute/command_execute_impl.cc
+++ b/win8/delegate_execute/command_execute_impl.cc
@@ -13,12 +13,14 @@
 #include "base/win/registry.h"
 #include "base/win/scoped_co_mem.h"
 #include "base/win/scoped_handle.h"
+#include "base/win/scoped_process_information.h"
 #include "base/win/win_util.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/installer/util/util_constants.h"
 #include "win8/delegate_execute/chrome_util.h"
+#include "win8/delegate_execute/delegate_execute_util.h"
 
 // CommandExecuteImpl is resposible for activating chrome in Windows 8. The
 // flow is complicated and this tries to highlight the important events.
@@ -76,8 +78,9 @@
 // a slow way to start chrome.
 //
 CommandExecuteImpl::CommandExecuteImpl()
-    : integrity_level_(base::INTEGRITY_UNKNOWN),
+    : parameters_(CommandLine::NO_PROGRAM),
       launch_scheme_(INTERNET_SCHEME_DEFAULT),
+      integrity_level_(base::INTEGRITY_UNKNOWN),
       chrome_mode_(ECHUIM_SYSTEM_LAUNCHER) {
   memset(&start_info_, 0, sizeof(start_info_));
   start_info_.cb = sizeof(start_info_);
@@ -94,9 +97,7 @@ STDMETHODIMP CommandExecuteImpl::SetKeyState(DWORD key_state) {
 
 STDMETHODIMP CommandExecuteImpl::SetParameters(LPCWSTR params) {
   AtlTrace("In %hs [%S]\n", __FUNCTION__, params);
-  if (params) {
-    parameters_ = params;
-  }
+  parameters_ = delegate_execute::CommandLineFromParameters(params);
   return S_OK;
 }
 
@@ -352,32 +353,21 @@ HRESULT CommandExecuteImpl::LaunchDesktopChrome() {
       break;
   }
 
-  string16 command_line = L"\"";
-  command_line += chrome_exe_.value();
-  command_line += L"\"";
-
-  if (!parameters_.empty()) {
-    AtlTrace("Adding parameters %ls to command line\n", parameters_.c_str());
-    command_line += L" ";
-    command_line += parameters_.c_str();
-  }
-
-  if (!display_name.empty()) {
-    command_line += L" -- ";
-    command_line += display_name;
-  }
+  CommandLine chrome(
+      delegate_execute::MakeChromeCommandLine(chrome_exe_, parameters_,
+                                              display_name));
+  string16 command_line(chrome.GetCommandLineString());
 
   AtlTrace("Formatted command line is %ls\n", command_line.c_str());
 
-  PROCESS_INFORMATION proc_info = {0};
-  BOOL ret = CreateProcess(NULL, const_cast<LPWSTR>(command_line.c_str()),
+  base::win::ScopedProcessInformation proc_info;
+  BOOL ret = CreateProcess(chrome_exe_.value().c_str(),
+                           const_cast<LPWSTR>(command_line.c_str()),
                            NULL, NULL, FALSE, 0, NULL, NULL, &start_info_,
-                           &proc_info);
+                           proc_info.Receive());
   if (ret) {
-    AtlTrace("Process id is %d\n", proc_info.dwProcessId);
-    AllowSetForegroundWindow(proc_info.dwProcessId);
-    CloseHandle(proc_info.hProcess);
-    CloseHandle(proc_info.hThread);
+    AtlTrace("Process id is %d\n", proc_info.process_id());
+    AllowSetForegroundWindow(proc_info.process_id());
   } else {
     AtlTrace("Process launch failed, error %d\n", ::GetLastError());
   }
@@ -402,14 +392,14 @@ EC_HOST_UI_MODE CommandExecuteImpl::GetLaunchMode() {
     return launch_mode;
   }
 
-  if (parameters_ == ASCIIToWide(switches::kForceImmersive)) {
+  if (parameters_.HasSwitch(switches::kForceImmersive)) {
     launch_mode = ECHUIM_IMMERSIVE;
     launch_mode_determined = true;
-    parameters_.clear();
-  } else if (parameters_ == ASCIIToWide(switches::kForceDesktop)) {
+    parameters_ = CommandLine(CommandLine::NO_PROGRAM);
+  } else if (parameters_.HasSwitch(switches::kForceDesktop)) {
     launch_mode = ECHUIM_DESKTOP;
     launch_mode_determined = true;
-    parameters_.clear();
+    parameters_ = CommandLine(CommandLine::NO_PROGRAM);
   }
 
   base::win::RegKey reg_key;
diff --git a/win8/delegate_execute/command_execute_impl.h b/win8/delegate_execute/command_execute_impl.h
index 5a1a95c..8fd00b8 100644
--- a/win8/delegate_execute/command_execute_impl.h
+++ b/win8/delegate_execute/command_execute_impl.h
@@ -10,6 +10,7 @@
 
 #include <string>
 
+#include "base/command_line.h"
 #include "base/file_path.h"
 #include "base/process_util.h"
 #include "win8/delegate_execute/resource.h"       // main symbols
@@ -91,7 +92,7 @@ class ATL_NO_VTABLE DECLSPEC_UUID("A2DF06F9-A21A-44A8-8A99-8B9C84F29160")
   EC_HOST_UI_MODE GetLaunchMode();
 
   CComPtr<IShellItemArray> item_array_;
-  string16 parameters_;
+  CommandLine parameters_;
   FilePath chrome_exe_;
   STARTUPINFO start_info_;
   string16 verb_;
diff --git a/win8/delegate_execute/delegate_execute.cc b/win8/delegate_execute/delegate_execute.cc
index 9e17ecc..2cfe418 100644
--- a/win8/delegate_execute/delegate_execute.cc
+++ b/win8/delegate_execute/delegate_execute.cc
@@ -13,7 +13,6 @@
 #include "base/file_util.h"
 #include "base/process_util.h"
 #include "base/string16.h"
-#include "base/utf_string_conversions.h"
 #include "base/win/scoped_com_initializer.h"
 #include "base/win/scoped_handle.h"
 #include "chrome/common/chrome_switches.h"
@@ -89,12 +88,12 @@ int RelaunchChrome(const DelegateExecuteOperation& operation) {
 
   base::win::ScopedCOMInitializer com_initializer;
 
-  string16 flags(ASCIIToWide(operation.relaunch_flags()));
+  string16 relaunch_flags(operation.relaunch_flags());
   SHELLEXECUTEINFO sei = { sizeof(sei) };
   sei.fMask = SEE_MASK_FLAG_LOG_USAGE;
   sei.nShow = SW_SHOWNORMAL;
   sei.lpFile = operation.shortcut().value().c_str();
-  sei.lpParameters = flags.c_str();
+  sei.lpParameters = relaunch_flags.c_str();
 
   AtlTrace(L"Relaunching Chrome via shortcut [%ls]\n", sei.lpFile);
 
diff --git a/win8/delegate_execute/delegate_execute.gyp b/win8/delegate_execute/delegate_execute.gyp
index e4feff4..a2e2770 100644
--- a/win8/delegate_execute/delegate_execute.gyp
+++ b/win8/delegate_execute/delegate_execute.gyp
@@ -8,6 +8,13 @@
   'includes': [
     '../../build/win_precompile.gypi',
   ],
+  'target_defaults': {
+    'defines': [
+      # This define is required to pull in the new Win8 interfaces from system
+      # headers like ShObjIdl.h
+      'NTDDI_VERSION=0x06020000',
+    ],
+  },
   'targets': [
     {
       'target_name': 'delegate_execute',
@@ -29,18 +36,29 @@
         'delegate_execute.rgs',
         'delegate_execute_operation.cc',
         'delegate_execute_operation.h',
+        'delegate_execute_util.cc',
+        'delegate_execute_util.h',
         'resource.h',
       ],
-      'defines': [
-        # This define is required to pull in the new Win8 interfaces from
-        # system headers like ShObjIdl.h
-        'NTDDI_VERSION=0x06020000',
-      ],
       'msvs_settings': {
         'VCLinkerTool': {
           'SubSystem': '2',  # Set /SUBSYSTEM:WINDOWS
         },
       },
     },
+    {
+      'target_name': 'delegate_execute_unittests',
+      'type': 'executable',
+      'dependencies': [
+        '../../base/base.gyp:base',
+        '../../base/base.gyp:run_all_unittests',
+        '../../testing/gtest.gyp:gtest',
+      ],
+      'sources': [
+        'delegate_execute_util.cc',
+        'delegate_execute_util.h',
+        'delegate_execute_util_unittest.cc',
+      ],
+    },
   ],
 }
diff --git a/win8/delegate_execute/delegate_execute_operation.cc b/win8/delegate_execute/delegate_execute_operation.cc
index f2ba2c4..f4aaa41 100644
--- a/win8/delegate_execute/delegate_execute_operation.cc
+++ b/win8/delegate_execute/delegate_execute_operation.cc
@@ -8,12 +8,12 @@
 #include "base/string_number_conversions.h"
 #include "base/string_split.h"
 #include "chrome/common/chrome_switches.h"
+#include "win8/delegate_execute/delegate_execute_util.h"
 
 namespace delegate_execute {
 
 DelegateExecuteOperation::DelegateExecuteOperation()
-    : operation_type_(DELEGATE_EXECUTE),
-      relaunch_flags_("") {
+    : operation_type_(DELEGATE_EXECUTE) {
 }
 
 DelegateExecuteOperation::~DelegateExecuteOperation() {
@@ -30,10 +30,14 @@ bool DelegateExecuteOperation::Init(const CommandLine* cmd_line) {
   if (mutex_.empty())
     return false;
   // Add the mode forcing flags, if any.
+  const char* the_switch = NULL;
+
   if (cmd_line->HasSwitch(switches::kForceDesktop))
-    relaunch_flags_ = switches::kForceDesktop;
+    the_switch = switches::kForceDesktop;
   else if (cmd_line->HasSwitch(switches::kForceImmersive))
-    relaunch_flags_ = switches::kForceImmersive;
+    the_switch = switches::kForceImmersive;
+
+  relaunch_flags_ = ParametersFromSwitch(the_switch);
 
   operation_type_ = RELAUNCH_CHROME;
   return true;
diff --git a/win8/delegate_execute/delegate_execute_operation.h b/win8/delegate_execute/delegate_execute_operation.h
index f2f01cc..3fc1b00 100644
--- a/win8/delegate_execute/delegate_execute_operation.h
+++ b/win8/delegate_execute/delegate_execute_operation.h
@@ -49,7 +49,7 @@ class DelegateExecuteOperation {
     return operation_type_;
   }
 
-  const char* relaunch_flags() const {
+  const string16& relaunch_flags() const {
     return relaunch_flags_;
   }
 
@@ -66,7 +66,7 @@ class DelegateExecuteOperation {
 
  private:
   OperationType operation_type_;
-  const char* relaunch_flags_;
+  string16 relaunch_flags_;
   FilePath relaunch_shortcut_;
   string16 mutex_;
 
diff --git a/win8/delegate_execute/delegate_execute_util.cc b/win8/delegate_execute/delegate_execute_util.cc
new file mode 100644
index 0000000..d04c667
--- /dev/null
+++ b/win8/delegate_execute/delegate_execute_util.cc
@@ -0,0 +1,50 @@
+// Copyright (c) 2012 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 "win8/delegate_execute/delegate_execute_util.h"
+
+#include "base/file_path.h"
+#include "base/string_util.h"
+
+namespace delegate_execute {
+
+CommandLine CommandLineFromParameters(const wchar_t* params) {
+  CommandLine command_line(CommandLine::NO_PROGRAM);
+
+  if (params) {
+    string16 command_string(L"noprogram.exe ");
+    command_string.append(params);
+    command_line.ParseFromString(command_string);
+    command_line.SetProgram(FilePath());
+  }
+
+  return command_line;
+}
+
+CommandLine MakeChromeCommandLine(const FilePath& chrome_exe,
+                                  const CommandLine& params,
+                                  const string16& argument) {
+  CommandLine chrome_cmd(params);
+  chrome_cmd.SetProgram(chrome_exe);
+
+  if (!argument.empty())
+    chrome_cmd.AppendArgNative(argument);
+
+  return chrome_cmd;
+}
+
+string16 ParametersFromSwitch(const char* a_switch) {
+  if (!a_switch)
+    return string16();
+
+  CommandLine cmd_line(CommandLine::NO_PROGRAM);
+
+  cmd_line.AppendSwitch(a_switch);
+
+  string16 command_string(cmd_line.GetCommandLineString());
+  TrimWhitespace(command_string, TRIM_ALL, &command_string);
+  return command_string;
+}
+
+}  // namespace delegate_execute
diff --git a/win8/delegate_execute/delegate_execute_util.h b/win8/delegate_execute/delegate_execute_util.h
new file mode 100644
index 0000000..5f001ff
--- /dev/null
+++ b/win8/delegate_execute/delegate_execute_util.h
@@ -0,0 +1,30 @@
+// Copyright (c) 2012 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.
+
+#ifndef WIN8_DELEGATE_EXECUTE_DELEGATE_EXECUTE_UTIL_H_
+#define WIN8_DELEGATE_EXECUTE_DELEGATE_EXECUTE_UTIL_H_
+
+#include "base/command_line.h"
+#include "base/string16.h"
+
+class FilePath;
+
+namespace delegate_execute {
+
+// Returns a CommandLine with an empty program parsed from |params|.
+CommandLine CommandLineFromParameters(const wchar_t* params);
+
+// Returns a CommandLine to launch |chrome_exe| with all switches and arguments
+// from |params| plus an optional |argument|.
+CommandLine MakeChromeCommandLine(const FilePath& chrome_exe,
+                                  const CommandLine& params,
+                                  const string16& argument);
+
+// Returns a properly quoted command-line string less the program (argv[0])
+// containing |switch|.
+string16 ParametersFromSwitch(const char* a_switch);
+
+}  // namespace delegate_execute
+
+#endif  // WIN8_DELEGATE_EXECUTE_DELEGATE_EXECUTE_UTIL_H_
diff --git a/win8/delegate_execute/delegate_execute_util_unittest.cc b/win8/delegate_execute/delegate_execute_util_unittest.cc
new file mode 100644
index 0000000..360d8e7
--- /dev/null
+++ b/win8/delegate_execute/delegate_execute_util_unittest.cc
@@ -0,0 +1,79 @@
+// Copyright (c) 2012 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 "win8/delegate_execute/delegate_execute_util.h"
+
+#include <algorithm>
+
+#include "base/command_line.h"
+#include "base/file_path.h"
+#include "base/stringprintf.h"
+#include "base/utf_string_conversions.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+
+static const char kSomeSwitch[] = "some-switch";
+
+}  // namespace
+
+TEST(DelegateExecuteUtil, CommandLineFromParametersTest) {
+  CommandLine cl(CommandLine::NO_PROGRAM);
+
+  // Empty parameters means empty command-line string.
+  cl = delegate_execute::CommandLineFromParameters(NULL);
+  EXPECT_EQ(std::wstring(), cl.GetProgram().value());
+  EXPECT_EQ(CommandLine::StringType(), cl.GetCommandLineString());
+
+  // Parameters with a switch are parsed properly.
+  cl = delegate_execute::CommandLineFromParameters(
+      base::StringPrintf(L"--%ls", ASCIIToWide(kSomeSwitch).c_str()).c_str());
+  EXPECT_EQ(std::wstring(), cl.GetProgram().value());
+  EXPECT_TRUE(cl.HasSwitch(kSomeSwitch));
+}
+
+TEST(DelegateExecuteUtil, MakeChromeCommandLineTest) {
+  static const wchar_t kSomeArgument[] = L"http://some.url/";
+  static const wchar_t kOtherArgument[] = L"http://some.other.url/";
+  const FilePath this_exe(CommandLine::ForCurrentProcess()->GetProgram());
+
+  CommandLine cl(CommandLine::NO_PROGRAM);
+
+  // Empty params and argument contains only the exe.
+  cl = delegate_execute::MakeChromeCommandLine(
+      this_exe, delegate_execute::CommandLineFromParameters(NULL), string16());
+  EXPECT_EQ(1, cl.argv().size());
+  EXPECT_EQ(this_exe.value(), cl.GetProgram().value());
+
+  // Empty params with arg contains the arg.
+  cl = delegate_execute::MakeChromeCommandLine(
+      this_exe, delegate_execute::CommandLineFromParameters(NULL),
+      string16(kSomeArgument));
+  EXPECT_EQ(2, cl.argv().size());
+  EXPECT_EQ(this_exe.value(), cl.GetProgram().value());
+  EXPECT_EQ(1, cl.GetArgs().size());
+  EXPECT_EQ(string16(kSomeArgument), cl.GetArgs()[0]);
+
+  // Params with switchs and args plus arg contains the arg.
+  cl = delegate_execute::MakeChromeCommandLine(
+      this_exe, delegate_execute::CommandLineFromParameters(
+          base::StringPrintf(L"--%ls -- %ls", ASCIIToWide(kSomeSwitch).c_str(),
+                             kOtherArgument).c_str()),
+      string16(kSomeArgument));
+  EXPECT_EQ(5, cl.argv().size());
+  EXPECT_EQ(this_exe.value(), cl.GetProgram().value());
+  EXPECT_TRUE(cl.HasSwitch(kSomeSwitch));
+  CommandLine::StringVector args(cl.GetArgs());
+  EXPECT_EQ(2, args.size());
+  EXPECT_NE(args.end(),
+            std::find(args.begin(), args.end(), string16(kOtherArgument)));
+  EXPECT_NE(args.end(),
+            std::find(args.begin(), args.end(), string16(kSomeArgument)));
+}
+
+TEST(DelegateExecuteUtil, ParametersFromSwitchTest) {
+  EXPECT_EQ(string16(), delegate_execute::ParametersFromSwitch(NULL));
+  EXPECT_EQ(string16(L"--some-switch"),
+            delegate_execute::ParametersFromSwitch(kSomeSwitch));
+}