Rename CommandLine::GetCommandLineString().

chrome/installer/util/product.cc

 // Copyright (c) 2011 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/installer/util/product.h"
 
 #include <algorithm>
 
 #include "base/command_line.h"
 #include "base/logging.h"
@@ skipping 61 matching lines @@
                                   int32* exit_code) const {
   if (application_path.empty())
     return false;
 
   CommandLine cmd(application_path.Append(installer::kChromeExe));
   cmd.AppendArguments(options, false);
 
   bool success = false;
   STARTUPINFOW si = { sizeof(si) };
   PROCESS_INFORMATION pi = {0};
-  // Cast away constness of the command_line_string() since CreateProcess
+  // Create a writable copy of the command line string, since CreateProcess
   // might modify the string (insert \0 to separate the program from the
   // arguments).  Since we're not using the cmd variable beyond this point

[grt (UTC plus 2), 2011/07/19 03:19:34]

The "Since we're not using..." comment isn't relevant anymore, is it?  If this
is the case, please remove it.

[msw, 2011/07/19 07:14:26]

Done.

   // we don't care.
+  scoped_ptr_malloc<wchar_t> writable_command_line_string(

[grt (UTC plus 2), 2011/07/19 03:19:34]

Why not:
std::wstring writable_command_line_string(cmd.GetCommandLineString());
followed by &writable_command_line_string[0] in the call to CreateProcess?  I
find this more readable and it should be more efficient since it makes use of
the string's knowledge of its length (no scan for the terminator).

[msw, 2011/07/19 07:14:26]

Done.

[Evan Martin, 2011/07/19 14:35:05]

Oh yeah, should have remembered this earlier:
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/base/string_util.h&ex...

+      ::_wcsdup(cmd.GetCommandLineString().c_str()));
   if (!::CreateProcess(cmd.GetProgram().value().c_str(),
-                       const_cast<wchar_t*>(cmd.command_line_string().c_str()),
+                       writable_command_line_string.get(),
                        NULL, NULL, FALSE, CREATE_NO_WINDOW, NULL, NULL,
                        &si, &pi)) {
-    PLOG(ERROR) << "Failed to launch: " << cmd.command_line_string();
+    PLOG(ERROR) << "Failed to launch: " << writable_command_line_string.get();

[grt (UTC plus 2), 2011/07/19 03:19:34]

How about either cmd.GetCommandLineString() (to log the whole command line) or
cmd.GetProgram().value() (to log only the program) here to avoid the ambiguity
of what CreateProcess may or may not do to the string?

[msw, 2011/07/19 07:14:26]

Done.

   } else {
     ::CloseHandle(pi.hThread);
 
     DWORD ret = ::WaitForSingleObject(pi.hProcess, INFINITE);
     DLOG_IF(ERROR, ret != WAIT_OBJECT_0)
         << "Unexpected return value from WaitForSingleObject: " << ret;
     if (::GetExitCodeProcess(pi.hProcess, &ret)) {
       DCHECK(ret != STILL_ACTIVE);
       success = true;
       if (exit_code)
@@ skipping 42 matching lines @@
 
 void Product::AppendRenameFlags(CommandLine* command_line) const {
   operations_->AppendRenameFlags(options_, command_line);
 }
 
 bool Product::SetChannelFlags(bool set, ChannelInfo* channel_info) const {
   return operations_->SetChannelFlags(options_, set, channel_info);
 }
 
 }  // namespace installer