Changing the installer switches from wchar_t[] to char[]....

Changing the installer switches from wchar_t[] to char[].

Because of this I'm also refactoring some code that before
was using wstring to build command lines by hand instead of
using the CommandLine class.  Now we use CommandLine.

To get this to work correctly, I also needed to fix CommandLine::AppendArguments so I added a little test for it.

TEST=There should be no changes in functionality.  Run all installer tests.
BUG=61609
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=66088

[tommi (sloooow) - chröme, 2010-11-12 23:57:42]

evanm: Could you please take a look at the CommandLine change?

[robertshield, 2010-11-14 15:21:31]

lg, some nits

http://codereview.chromium.org/4928002/diff/71001/base/command_line_unittest.cc
File base/command_line_unittest.cc (right):

http://codereview.chromium.org/4928002/diff/71001/base/command_line_unittest....
base/command_line_unittest.cc:168: EXPECT_EQ(cl1.command_line_string(),
cl2.command_line_string());
Could you also test the new replacement behaviour of AppendArguments when both
CommandLines are not created with NO_PROGRAM?
e.g.
CommandLine c1("Program1");
c1.AppendSwitch("switch1");
CommandLine c2("Program2:);
c2.AppendSwitch("switch2");

c1.AppendArguments(c2, true);
EXPECT_EQ(c1.GetProgram.value(), c2.GetProgram.value());

http://codereview.chromium.org/4928002/diff/71001/chrome/installer/util/insta...
File chrome/installer/util/install_util.cc (right):

http://codereview.chromium.org/4928002/diff/71001/chrome/installer/util/insta...
chrome/installer/util/install_util.cc:45: if (params[0] == '"') {
Add a comment here along the lines of "include the quotes in the program
component". When I first glanced at it, I thought it would try to remove the
quotes.

http://codereview.chromium.org/4928002/diff/71001/chrome/installer/util/insta...
chrome/installer/util/install_util.cc:47: params =
params.substr(program.length() + 2);
Should we also DCHECK that the last character is a quote?

http://codereview.chromium.org/4928002/diff/71001/chrome/installer/util/shell...
File chrome/installer/util/shell_util.cc (right):

http://codereview.chromium.org/4928002/diff/71001/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:299: // That will show the user standard
Vista elevation prompt. If the user accepts
will show the user *the* standard Vista elevation prompt.

http://codereview.chromium.org/4928002/diff/71001/chrome/installer/util/util_...
File chrome/installer/util/util_constants.cc (right):

http://codereview.chromium.org/4928002/diff/71001/chrome/installer/util/util_...
chrome/installer/util/util_constants.cc:64: const char kLogFile[] = "log_file";
why change the flag name here? most other flags use '-'

[evanm, 2010-11-14 19:15:39]

http://codereview.chromium.org/4928002/diff/1/base/command_line_unittest.cc
File base/command_line_unittest.cc (right):

http://codereview.chromium.org/4928002/diff/1/base/command_line_unittest.cc#n...
base/command_line_unittest.cc:159: // command line strings are equal.
Maybe change the end of this sentece to "...is called with an empty CommandLine,
nothing changes".

http://codereview.chromium.org/4928002/diff/1/base/command_line_unittest.cc#n...
base/command_line_unittest.cc:166: cl2.AppendArguments(cl1, true);
Shouldn't this hit the DCHECK(), since it's saying "include the other guy's
program", but the other guy's program is empty?

[evanm, 2010-11-14 19:16:18]

http://codereview.chromium.org/4928002/diff/1/base/command_line_unittest.cc
File base/command_line_unittest.cc (right):

http://codereview.chromium.org/4928002/diff/1/base/command_line_unittest.cc#n...
base/command_line_unittest.cc:166: cl2.AppendArguments(cl1, true);
On 2010/11/14 19:15:39, evanm wrote:
> Shouldn't this hit the DCHECK(), since it's saying "include the other guy's
> program", but the other guy's program is empty?

Never mind, ready it backwards.  :(

[tommi (sloooow) - chröme, 2010-11-14 21:28:07]

thanks.  changes uploaded.

http://codereview.chromium.org/4928002/diff/1/base/command_line_unittest.cc
File base/command_line_unittest.cc (right):

http://codereview.chromium.org/4928002/diff/1/base/command_line_unittest.cc#n...
base/command_line_unittest.cc:159: // command line strings are equal.
On 2010/11/14 19:15:39, evanm wrote:
> Maybe change the end of this sentece to "...is called with an empty
CommandLine,
> nothing changes".

Changed the description after incorporating Robert's suggestions.

http://codereview.chromium.org/4928002/diff/71001/base/command_line_unittest....
base/command_line_unittest.cc:168: EXPECT_EQ(cl1.GetProgram(),
cl2.GetProgram());
On 2010/11/14 15:21:31, robertshield wrote:
> Could you also test the new replacement behaviour of AppendArguments when both
> CommandLines are not created with NO_PROGRAM?
> e.g.
> CommandLine c1("Program1");
> c1.AppendSwitch("switch1");
> CommandLine c2("Program2:);
> c2.AppendSwitch("switch2");
> 
> c1.AppendArguments(c2, true);
> EXPECT_EQ(c1.GetProgram.value(), c2.GetProgram.value());

Done.

http://codereview.chromium.org/4928002/diff/71001/chrome/installer/util/insta...
File chrome/installer/util/install_util.cc (right):

http://codereview.chromium.org/4928002/diff/71001/chrome/installer/util/insta...
chrome/installer/util/install_util.cc:45: if (params[0] == '"') {
On 2010/11/14 15:21:31, robertshield wrote:
> Add a comment here along the lines of "include the quotes in the program
> component". When I first glanced at it, I thought it would try to remove the
> quotes.

Not sure I follow... this part does remove the quotes.
I start by checking if the command line starts with a quote, if it does, then it
removes 2 more characters and DCHECKs that the part being removed matches the
program path which is being removed.

http://codereview.chromium.org/4928002/diff/71001/chrome/installer/util/insta...
chrome/installer/util/install_util.cc:47: params =
params.substr(program.length() + 2);
On 2010/11/14 15:21:31, robertshield wrote:
> Should we also DCHECK that the last character is a quote?

Done.

http://codereview.chromium.org/4928002/diff/71001/chrome/installer/util/shell...
File chrome/installer/util/shell_util.cc (right):

http://codereview.chromium.org/4928002/diff/71001/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:299: // That will show the user standard
Vista elevation prompt. If the user accepts
On 2010/11/14 15:21:31, robertshield wrote:
> will show the user *the* standard Vista elevation prompt.

Done.

http://codereview.chromium.org/4928002/diff/71001/chrome/installer/util/util_...
File chrome/installer/util/util_constants.cc (right):

http://codereview.chromium.org/4928002/diff/71001/chrome/installer/util/util_...
chrome/installer/util/util_constants.cc:64: const char kLogFile[] = "log_file";
On 2010/11/14 15:21:31, robertshield wrote:
> why change the flag name here? most other flags use '-'

Doh, bogus leftovers from a different patch.  Removed.

[robertshield, 2010-11-14 21:40:17]

LGTM

http://codereview.chromium.org/4928002/diff/71001/chrome/installer/util/insta...
File chrome/installer/util/install_util.cc (right):

http://codereview.chromium.org/4928002/diff/71001/chrome/installer/util/insta...
chrome/installer/util/install_util.cc:45: if (params[0] == '"') {
On 2010/11/14 21:28:07, tommi wrote:
> On 2010/11/14 15:21:31, robertshield wrote:
> > Add a comment here along the lines of "include the quotes in the program
> > component". When I first glanced at it, I thought it would try to remove the
> > quotes.
> 
> Not sure I follow... this part does remove the quotes.
> I start by checking if the command line starts with a quote, if it does, then
it
> removes 2 more characters and DCHECKs that the part being removed matches the
> program path which is being removed.

uh, yeah, sorry I misread it earlier. pebkac.