CommandLine refactoring and cleanup.

Consolidate most CommandLine code across platforms.
Significant refactoring with some notable behavior changes:
1. Switches are appended preceding existing arguments (after other swtiches).
2. (Windows) command_line_string() is generated and properly quoted/escaped.
3. Appended switches will retain their (optional) included prefixes (--,-,/).
Notable internal changes (shouldn't affect behavior):
1. (Windows) Generate the cl string, instead of storing&updating the original.
2. Explicitly retain switch prefixes (--,-,/) (was automatic in init*/ctor).
Update (obvious) code expecting switches to be appended antecedent to args.
Add Nico's test from: codereview.chromium.org/6728016/.

An intermediary CL landed between patch set 3 and 4, see:
http://codereview.chromium.org/6596020

BUG=73195, 67764
TEST=Commandline usage.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=85360

[evanm, 2011-02-24 01:01:05]

I appreciate any cleanup you do, but it's a bit hard to follow this change;
things are different all over but it's not clear to me why and where.

http://codereview.chromium.org/6526040/diff/2001/base/command_line.cc
File base/command_line.cc (left):

http://codereview.chromium.org/6526040/diff/2001/base/command_line.cc#oldcode83
base/command_line.cc:83: 
It's hard to follow what's gone on in this file.  Did you move this code, or
change it, or...?

It might be helpful to move it in one change (which is trivial to review) and
then change it in a follow-up.

http://codereview.chromium.org/6526040/diff/2001/base/command_line.h
File base/command_line.h (right):

http://codereview.chromium.org/6526040/diff/2001/base/command_line.h#newcode25
base/command_line.h:25: using std::string;
This is against Chrome style.  Typing out std:: everywhere drove me crazy my
first year and eventually I got used to it.

http://codereview.chromium.org/6526040/diff/2001/base/command_line.h#newcode34
base/command_line.h:34: typedef std::wstring StringType;
probably didn't mean to add that whitespace

http://codereview.chromium.org/6526040/diff/2001/base/command_line.h#newcode95
base/command_line.h:95: StringVector args() const;
I guess if it's not a simple getter it ought to be renamed. (GetArgs or
whatever.)

http://codereview.chromium.org/6526040/diff/2001/base/command_line.h#newcode149
base/command_line.h:149: CommandLine() { divider_ = 0; }
Why provide an impl if it's not to be run?

http://codereview.chromium.org/6526040/diff/2001/base/command_line.h#newcode161
base/command_line.h:161: // An index dividing switches from arguments in
|argv_|.
Might be clearer if you say what it is (the position of the first argument? the
position of the last switch?) in addition to what it does.

[Paweł Hajdan Jr., 2011-02-25 18:01:52]

Drive-by with testing-related comments. Please let me do another review before
committing.

http://codereview.chromium.org/6526040/diff/2001/base/command_line.h
File base/command_line.h (right):

http://codereview.chromium.org/6526040/diff/2001/base/command_line.h#newcode146
base/command_line.h:146: friend class InProcessBrowserTest;
Bonus points for getting rid of that friend declaration.

http://codereview.chromium.org/6526040/diff/2001/chrome/test/in_process_brows...
File chrome/test/in_process_browser_test.cc (right):

http://codereview.chromium.org/6526040/diff/2001/chrome/test/in_process_brows...
chrome/test/in_process_browser_test.cc:107: CommandLine* command_line =
CommandLine::current_process_commandline_;
nit: Accessing a private variable like that is ugly. Can you find a cleaner
solution?

[msw, 2011-05-10 23:18:43]

I finally made some progress; please consider this updated CL.

I worry that some obscure CommandLines rely on the previous behavior allowing
switches to follow arguments (see the test uses I've updated). I may just have
to go through every use of AppendSwitch*. Please let me know what you think;
thanks.

http://codereview.chromium.org/6526040/diff/2001/base/command_line.cc
File base/command_line.cc (left):

http://codereview.chromium.org/6526040/diff/2001/base/command_line.cc#oldcode83
base/command_line.cc:83: 
On 2011/02/24 01:01:05, evanm wrote:
> It's hard to follow what's gone on in this file.  Did you move this code, or
> change it, or...?
> 
> It might be helpful to move it in one change (which is trivial to review) and
> then change it in a follow-up.

Done (move was r76339, this change is cleaner).

http://codereview.chromium.org/6526040/diff/2001/base/command_line.h
File base/command_line.h (right):

http://codereview.chromium.org/6526040/diff/2001/base/command_line.h#newcode25
base/command_line.h:25: using std::string;
On 2011/02/24 01:01:05, evanm wrote:
> This is against Chrome style.  Typing out std:: everywhere drove me crazy my
> first year and eventually I got used to it.

Done.

http://codereview.chromium.org/6526040/diff/2001/base/command_line.h#newcode34
base/command_line.h:34: typedef std::wstring StringType;
On 2011/02/24 01:01:05, evanm wrote:
> probably didn't mean to add that whitespace

Done.

http://codereview.chromium.org/6526040/diff/2001/base/command_line.h#newcode95
base/command_line.h:95: StringVector args() const;
On 2011/02/24 01:01:05, evanm wrote:
> I guess if it's not a simple getter it ought to be renamed. (GetArgs or
> whatever.)

Added TODO (to keep this change smaller).

http://codereview.chromium.org/6526040/diff/2001/base/command_line.h#newcode146
base/command_line.h:146: friend class InProcessBrowserTest;
On 2011/02/25 18:01:53, Paweł Hajdan Jr. wrote:
> Bonus points for getting rid of that friend declaration.

Done in r76339.

http://codereview.chromium.org/6526040/diff/2001/base/command_line.h#newcode149
base/command_line.h:149: CommandLine() { divider_ = 0; }
On 2011/02/24 01:01:05, evanm wrote:
> Why provide an impl if it's not to be run?

Done, but it'd be cleaner to just replace the NO_PROGRAM ctor  with the default.
What do you think?

http://codereview.chromium.org/6526040/diff/2001/base/command_line.h#newcode161
base/command_line.h:161: // An index dividing switches from arguments in
|argv_|.
On 2011/02/24 01:01:05, evanm wrote:
> Might be clearer if you say what it is (the position of the first argument?
the
> position of the last switch?) in addition to what it does.

Done.

http://codereview.chromium.org/6526040/diff/2001/chrome/test/in_process_brows...
File chrome/test/in_process_browser_test.cc (right):

http://codereview.chromium.org/6526040/diff/2001/chrome/test/in_process_brows...
chrome/test/in_process_browser_test.cc:107: CommandLine* command_line =
CommandLine::current_process_commandline_;
On 2011/02/25 18:01:53, Paweł Hajdan Jr. wrote:
> nit: Accessing a private variable like that is ugly. Can you find a cleaner
> solution?

Done (r76339).

[Evan Martin, 2011-05-10 23:48:32]

http://codereview.chromium.org/6526040/diff/26001/base/command_line.cc
File base/command_line.cc (left):

http://codereview.chromium.org/6526040/diff/26001/base/command_line.cc#oldcod...
base/command_line.cc:278: #endif
Since you have to have this ifdef somewhere, why move it to the other location? 
It seems less readable to me.  (In reading the new code, it looks like we always
lowercase the switch).  If you really prefer this, consider renaming the
function LowercaseOnWindows or something.

http://codereview.chromium.org/6526040/diff/26001/base/command_line.cc
File base/command_line.cc (right):

http://codereview.chromium.org/6526040/diff/26001/base/command_line.cc#newcode37
base/command_line.cc:37: CommandLine::StringType Native(const std::string&
string) {
Can you make this name more descriptive?  Maybe just ToNative would be better. 
A comment probably would've helped too.  :)

http://codereview.chromium.org/6526040/diff/26001/base/command_line.cc#newcod...
base/command_line.cc:105: // TODO(msw): Proper POSIX quoting.
I don't think this can be done.  Quoting is always done relative to a program
that interprets the quoting: for example, in generating our .desktop files (used
to launch the app from an icon) the rules for quoting are different than the
rules for a command line that works in a bash script or the shell.

Similarly, the quoting we do here works for some Windows apps but not
necessarily all.  That is why I emphasized in the comment you deleted that it
works with CommandLineToArgvW() specifically.


In general, I dislike the idea of a generic "quote" function.  I would prefer
for callers to specifically request what sort of quoting they would like (for
example including it in the function name) or just expose API so that they can
do the quoting themselves (like we do for the .desktop files above).

http://codereview.chromium.org/6526040/diff/26001/base/command_line.cc#newcod...
base/command_line.cc:316: size_t prexfix_length =
GetSwitchPrefixLength(combined_switch_string);
typo: "prefix"

http://codereview.chromium.org/6526040/diff/26001/base/command_line.cc#newcod...
base/command_line.cc:366: DCHECK(IsStringUTF8(value));
It turns out this code (not your fault) is conceptually wrong -- the "native"
string type on POSIX is bytes, not UTF-8.  So you can safely delete this ifdef
block and the dcheck.

http://codereview.chromium.org/6526040/diff/26001/base/command_line.cc#newcod...
base/command_line.cc:403: LocalFree(args);
Nice refactoring!

http://codereview.chromium.org/6526040/diff/26001/base/command_line.h
File base/command_line.h (right):

http://codereview.chromium.org/6526040/diff/26001/base/command_line.h#newcode81
base/command_line.h:81: // TODO(msw): Rename GetCommandLineString.
As I discuss inline, I'd prefer to either remove this function or give it a more
descriptive name.

http://codereview.chromium.org/6526040/diff/26001/base/command_line_unittest.cc
File base/command_line_unittest.cc (left):

http://codereview.chromium.org/6526040/diff/26001/base/command_line_unittest....
base/command_line_unittest.cc:62: #endif
Isn't this false on non-Windows?  It's testing case-insensitivity.

http://codereview.chromium.org/6526040/diff/26001/base/command_line_unittest.cc
File base/command_line_unittest.cc (right):

http://codereview.chromium.org/6526040/diff/26001/base/command_line_unittest....
base/command_line_unittest.cc:126: EXPECT_EQ(kTricky,
cl.GetSwitchValueNative("quotes"));
This shouldn't pass on non-Windows, as there is no interpretation of quoting on
non-Windows.

http://codereview.chromium.org/6526040/diff/26001/base/command_line_unittest....
base/command_line_unittest.cc:132: EXPECT_EQ(FILE_PATH_LITERAL("flim"),
*(iter++));
I'm a little concerned that the EXPECT_EQ macro might evaluate its arguments
multiple times.  But I guess this must work for you...

http://codereview.chromium.org/6526040/diff/26001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_test.cc (right):

http://codereview.chromium.org/6526040/diff/26001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_test.cc:146: datafile.value());
Why not AppendSwitch for these?

http://codereview.chromium.org/6526040/diff/26001/chrome/common/switch_utils_...
File chrome/common/switch_utils_unittest.cc (right):

http://codereview.chromium.org/6526040/diff/26001/chrome/common/switch_utils_...
chrome/common/switch_utils_unittest.cc:35: #if defined(OS_WIN)
Maybe we should test this on all platforms?

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

http://codereview.chromium.org/6526040/diff/26001/chrome/installer/util/insta...
chrome/installer/util/install_util.cc:364: *command_line =
CommandLine::FromString(program + arguments);
This is a little bit terrifying to me.  How certain are you that it is correct?

I guess we don't expect our exe_path to be too crazy with the quotes...

http://codereview.chromium.org/6526040/diff/26001/chrome/test/live_sync/live_...
File chrome/test/live_sync/live_sync_test.cc (right):

http://codereview.chromium.org/6526040/diff/26001/chrome/test/live_sync/live_...
chrome/test/live_sync/live_sync_test.cc:408: Tokenize(server_cmdline_string,
delimiters, &server_cmdline_vector);
I wonder what this function is doing?  It seems like it should have been using
CommandLine APIs instead...

http://codereview.chromium.org/6526040/diff/26001/chrome/test/ui/ui_test.cc
File chrome/test/ui/ui_test.cc (right):

http://codereview.chromium.org/6526040/diff/26001/chrome/test/ui/ui_test.cc#n...
chrome/test/ui/ui_test.cc:560: root_directory.value());
Why not AppendSwitch?

[msw, 2011-05-11 02:28:11]

Addressed some comments; responded to others.
grt, please review: chrome/installer/util/install_util.cc

http://codereview.chromium.org/6526040/diff/26001/base/command_line.cc
File base/command_line.cc (left):

http://codereview.chromium.org/6526040/diff/26001/base/command_line.cc#oldcod...
base/command_line.cc:278: #endif
On 2011/05/10 23:48:32, Evan Martin wrote:
> Since you have to have this ifdef somewhere, why move it to the other
location? 
> It seems less readable to me.  (In reading the new code, it looks like we
always
> lowercase the switch).  If you really prefer this, consider renaming the
> function LowercaseOnWindows or something.

Done (reverted). I was naively going for fewer #ifdefs.

http://codereview.chromium.org/6526040/diff/26001/base/command_line.cc
File base/command_line.cc (right):

http://codereview.chromium.org/6526040/diff/26001/base/command_line.cc#newcode37
base/command_line.cc:37: CommandLine::StringType Native(const std::string&
string) {
On 2011/05/10 23:48:32, Evan Martin wrote:
> Can you make this name more descriptive?  Maybe just ToNative would be better.

> A comment probably would've helped too.  :)

Done; I renamed this function and added a TODO about string16.

http://codereview.chromium.org/6526040/diff/26001/base/command_line.cc#newcod...
base/command_line.cc:105: // TODO(msw): Proper POSIX quoting.
On 2011/05/10 23:48:32, Evan Martin wrote:
> I don't think this can be done.  Quoting is always done relative to a program
> that interprets the quoting: for example, in generating our .desktop files
(used
> to launch the app from an icon) the rules for quoting are different than the
> rules for a command line that works in a bash script or the shell.
> 
> Similarly, the quoting we do here works for some Windows apps but not
> necessarily all.  That is why I emphasized in the comment you deleted that it
> works with CommandLineToArgvW() specifically.
> 
> 
> In general, I dislike the idea of a generic "quote" function.  I would prefer
> for callers to specifically request what sort of quoting they would like (for
> example including it in the function name) or just expose API so that they can
> do the quoting themselves (like we do for the .desktop files above).

Thank you for some perspective on this issue. For now, I renamed this function,
pushed the OS_WIN filtering to its uses in command_line_string() (like
Lowercase), removed the TODO, and tweaked the function comment.

http://codereview.chromium.org/6526040/diff/26001/base/command_line.cc#newcod...
base/command_line.cc:316: size_t prexfix_length =
GetSwitchPrefixLength(combined_switch_string);
On 2011/05/10 23:48:32, Evan Martin wrote:
> typo: "prefix"

Done.

http://codereview.chromium.org/6526040/diff/26001/base/command_line.cc#newcod...
base/command_line.cc:366: DCHECK(IsStringUTF8(value));
On 2011/05/10 23:48:32, Evan Martin wrote:
> It turns out this code (not your fault) is conceptually wrong -- the "native"
> string type on POSIX is bytes, not UTF-8.  So you can safely delete this ifdef
> block and the dcheck.

Done. Should I also nix the OS_WIN IsStringUTF8 check in AppendArg?

http://codereview.chromium.org/6526040/diff/26001/base/command_line.cc#newcod...
base/command_line.cc:403: LocalFree(args);
On 2011/05/10 23:48:32, Evan Martin wrote:
> Nice refactoring!

:)

http://codereview.chromium.org/6526040/diff/26001/base/command_line.h
File base/command_line.h (right):

http://codereview.chromium.org/6526040/diff/26001/base/command_line.h#newcode81
base/command_line.h:81: // TODO(msw): Rename GetCommandLineString.
On 2011/05/10 23:48:32, Evan Martin wrote:
> As I discuss inline, I'd prefer to either remove this function or give it a
more
> descriptive name.

Sorry, I missed any other inline mentions. This seems unlikely to go away in the
immediate future, but I will rename it and eliminate low-hanging calls in a
follow-up CL. What name do you suggest?

http://codereview.chromium.org/6526040/diff/26001/base/command_line_unittest.cc
File base/command_line_unittest.cc (left):

http://codereview.chromium.org/6526040/diff/26001/base/command_line_unittest....
base/command_line_unittest.cc:62: #endif
On 2011/05/10 23:48:32, Evan Martin wrote:
> Isn't this false on non-Windows?  It's testing case-insensitivity.

Sorry for the confusing diff, but this code is in a OS_WIN-only test
(CommandLineFromString). I added the same check in an OS_WIN specific block of
the cross platform test (CommandLineConstructor).

http://codereview.chromium.org/6526040/diff/26001/base/command_line_unittest.cc
File base/command_line_unittest.cc (right):

http://codereview.chromium.org/6526040/diff/26001/base/command_line_unittest....
base/command_line_unittest.cc:126: EXPECT_EQ(kTricky,
cl.GetSwitchValueNative("quotes"));
On 2011/05/10 23:48:32, Evan Martin wrote:
> This shouldn't pass on non-Windows, as there is no interpretation of quoting
on
> non-Windows.

Same as above (OS_WIN-only test).

http://codereview.chromium.org/6526040/diff/26001/base/command_line_unittest....
base/command_line_unittest.cc:132: EXPECT_EQ(FILE_PATH_LITERAL("flim"),
*(iter++));
On 2011/05/10 23:48:32, Evan Martin wrote:
> I'm a little concerned that the EXPECT_EQ macro might evaluate its arguments
> multiple times.  But I guess this must work for you...

Done (reverted here and below). Scott Meyers' "Effective C++" taught me that
this was bad practice; shame on me for forgetting.

http://codereview.chromium.org/6526040/diff/26001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_test.cc (right):

http://codereview.chromium.org/6526040/diff/26001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_test.cc:146: datafile.value());
On 2011/05/10 23:48:32, Evan Martin wrote:
> Why not AppendSwitch for these?

Because Python syntax requires the script (testserver) to precede any arguments
to the script. I merely restored the CommandLine argv_ order for LaunchApp's use
of command_line_string() with Python to fix affected test(s).

http://codereview.chromium.org/6526040/diff/26001/chrome/common/switch_utils_...
File chrome/common/switch_utils_unittest.cc (right):

http://codereview.chromium.org/6526040/diff/26001/chrome/common/switch_utils_...
chrome/common/switch_utils_unittest.cc:35: #if defined(OS_WIN)
On 2011/05/10 23:48:32, Evan Martin wrote:
> Maybe we should test this on all platforms?

I exposed the argv-constructed CommandLine case to all platforms in the test
above (RemoveSwitches). This test (RemoveSwitchesFromString) is Windows-specific
because FromString is OS_WIN-only. Sorry, but I'm not sure what you are
requesting.

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

http://codereview.chromium.org/6526040/diff/26001/chrome/installer/util/insta...
chrome/installer/util/install_util.cc:364: *command_line =
CommandLine::FromString(program + arguments);
On 2011/05/10 23:48:32, Evan Martin wrote:
> This is a little bit terrifying to me.  How certain are you that it is
correct?
> 
> I guess we don't expect our exe_path to be too crazy with the quotes...

I agree that this isn't robust quoting/escaping, but I mostly maintained
existing behavior (except this also uses empty double quotes instead of
NO_PROGRAM/empty-string when |exe_path| AND |arguments| are empty). I would
update this to use the CommandLine::StringVector ctor (w/CommandLine quoting as
needed), but thought against naively splitting the |arguments| string, which is
retrieved from a registry entry (kUninstallArgumentsField) via
installer::ProductState::Initialize. Greg, what are your thoughts?

http://codereview.chromium.org/6526040/diff/26001/chrome/test/live_sync/live_...
File chrome/test/live_sync/live_sync_test.cc (right):

http://codereview.chromium.org/6526040/diff/26001/chrome/test/live_sync/live_...
chrome/test/live_sync/live_sync_test.cc:408: Tokenize(server_cmdline_string,
delimiters, &server_cmdline_vector);
On 2011/05/10 23:48:32, Evan Martin wrote:
> I wonder what this function is doing?  It seems like it should have been using
> CommandLine APIs instead...

Hmm, it appears this parent process uses its kSyncServerCommandLine switch for
the sync server commandline. It's a little meta, but should work as-is for
simple cases; shall I file a bug or contact the owner (rsimha)?

http://codereview.chromium.org/6526040/diff/26001/chrome/test/ui/ui_test.cc
File chrome/test/ui/ui_test.cc (right):

http://codereview.chromium.org/6526040/diff/26001/chrome/test/ui/ui_test.cc#n...
chrome/test/ui/ui_test.cc:560: root_directory.value());
On 2011/05/10 23:48:32, Evan Martin wrote:
> Why not AppendSwitch?

Same reasoning as SafeBrowsingTestServer::Start(). Python syntax requires the
script to precede these arguments to the script. I merely restored the
CommandLine argv_ order to fix affected test(s).

[Paweł Hajdan Jr., 2011-05-11 07:15:01]

Code I commented in the drive-by LGTM, thanks!

[grt (UTC plus 2), 2011-05-11 14:27:32]

http://codereview.chromium.org/6526040/diff/26003/base/command_line.cc
File base/command_line.cc (right):

http://codereview.chromium.org/6526040/diff/26003/base/command_line.cc#newcode39
base/command_line.cc:39: CommandLine::StringType GetNativeString(const
std::string& string) {
These two functions assume that |string| is or can be represented in ASCII. 
Please add comments about this to the docs and consider putting "ASCII" in their
names to make this even more explicit.

http://codereview.chromium.org/6526040/diff/26003/base/command_line.cc#newcode57
base/command_line.cc:57: // Lowercase a string for case-insensitivity of
switches.
consider wrapping these in #if defined(OS_WIN) to prevent someone from
accidentally using them in platform-general code.

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

http://codereview.chromium.org/6526040/diff/26003/chrome/installer/util/insta...
chrome/installer/util/install_util.cc:363: std::wstring
program(!exe_path.empty() ? L"\"" + exe_path + L"\" " : L"\"\"");
No whitespace is put between the pair of double quotes and the arguments when
|exe_path| is empty.  I'm not a fan of operator+ and strings, but I won't fight
the use of it.  If you like this simplified code, would it work to push it
further to:
*command_line = CommandLine::FromString(L"\"" + exe_path + L"\" " + arguments);
Does that do the right thing with all permutations of |exe_path| and |arguments|
being empty?
Also, please make sure that installer_util_unittests pass (there's a test for
this function in there).  Thanks.

[msw, 2011-05-12 00:37:47]

Thanks, Greg! Your feedback prompted some extra refactoring.
Evan and Greg, please consider my updated changes; thanks!

http://codereview.chromium.org/6526040/diff/26003/base/command_line.cc
File base/command_line.cc (right):

http://codereview.chromium.org/6526040/diff/26003/base/command_line.cc#newcode39
base/command_line.cc:39: CommandLine::StringType GetNativeString(const
std::string& string) {
On 2011/05/11 14:27:32, grt wrote:
> These two functions assume that |string| is or can be represented in ASCII. 
> Please add comments about this to the docs and consider putting "ASCII" in
their
> names to make this even more explicit.

I nixed GetNativeString and refactored, pushing out just two explicit
ASCIIToWide calls. GetNativeString had more use in earlier CL drafts.

http://codereview.chromium.org/6526040/diff/26003/base/command_line.cc#newcode57
base/command_line.cc:57: // Lowercase a string for case-insensitivity of
switches.
On 2011/05/11 14:27:32, grt wrote:
> consider wrapping these in #if defined(OS_WIN) to prevent someone from
> accidentally using them in platform-general code.

I nixed Lowercase for StringToLowerASCII, but wrapped
QuoteForCommandLineToArgvW.

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

http://codereview.chromium.org/6526040/diff/26003/chrome/installer/util/insta...
chrome/installer/util/install_util.cc:363: std::wstring
program(!exe_path.empty() ? L"\"" + exe_path + L"\" " : L"\"\"");
On 2011/05/11 14:27:32, grt wrote:
> No whitespace is put between the pair of double quotes and the arguments when
> |exe_path| is empty.  I'm not a fan of operator+ and strings, but I won't
fight
> the use of it.  If you like this simplified code, would it work to push it
> further to:
> *command_line = CommandLine::FromString(L"\"" + exe_path + L"\" " +
arguments);
> Does that do the right thing with all permutations of |exe_path| and
|arguments|
> being empty?
> Also, please make sure that installer_util_unittests pass (there's a test for
> this function in there).  Thanks.

Done and tests pass. This refactoring is entirely optional, so let me know if
you'd rather not have this change. Only the updates to
chrome/installer/util/product_state_unittest.cc are necessary.

[msw, 2011-05-13 15:52:08]

Ping! Sorry for the size and churn of this change, but feedback is highly
appreciated.

[Evan Martin, 2011-05-13 16:45:04]

http://codereview.chromium.org/6526040/diff/39005/base/command_line.h
File base/command_line.h (right):

http://codereview.chromium.org/6526040/diff/39005/base/command_line.h#newcode159
base/command_line.h:159: StringType program_string_;
Why do we track this separately when we're careful to keep it in argv_[0] as
well?

It appears from grepping this is actually vestigal, that you can implement
GetProgram() as just return argv_[0].

http://codereview.chromium.org/6526040/diff/39005/chrome/test/ui/ui_test.cc
File chrome/test/ui/ui_test.cc (right):

http://codereview.chromium.org/6526040/diff/39005/chrome/test/ui/ui_test.cc#n...
chrome/test/ui/ui_test.cc:553: cmd_line->AppendArg("--server=start");
I worry that, without a comment, someone will helpfully "refactor" this to use
AppendSwitchASCII.

http://codereview.chromium.org/6526040/diff/39005/chrome/test/ui_test_utils.cc
File chrome/test/ui_test_utils.cc (right):

http://codereview.chromium.org/6526040/diff/39005/chrome/test/ui_test_utils.c...
chrome/test/ui_test_utils.cc:754: cmd_line->AppendArg("--server=start");
I worry that, without a comment, someone will helpfully "refactor" this to use
AppendSwitchASCII.

[msw, 2011-05-13 18:54:49]

Done. PTAL, thanks.

http://codereview.chromium.org/6526040/diff/39005/base/command_line.h
File base/command_line.h (right):

http://codereview.chromium.org/6526040/diff/39005/base/command_line.h#newcode159
base/command_line.h:159: StringType program_string_;
On 2011/05/13 16:45:05, Evan Martin wrote:
> Why do we track this separately when we're careful to keep it in argv_[0] as
> well?
> 
> It appears from grepping this is actually vestigal, that you can implement
> GetProgram() as just return argv_[0].

Done.

http://codereview.chromium.org/6526040/diff/39005/chrome/test/ui/ui_test.cc
File chrome/test/ui/ui_test.cc (right):

http://codereview.chromium.org/6526040/diff/39005/chrome/test/ui/ui_test.cc#n...
chrome/test/ui/ui_test.cc:553: cmd_line->AppendArg("--server=start");
On 2011/05/13 16:45:05, Evan Martin wrote:
> I worry that, without a comment, someone will helpfully "refactor" this to use
> AppendSwitchASCII.

Done.

http://codereview.chromium.org/6526040/diff/39005/chrome/test/ui_test_utils.cc
File chrome/test/ui_test_utils.cc (right):

http://codereview.chromium.org/6526040/diff/39005/chrome/test/ui_test_utils.c...
chrome/test/ui_test_utils.cc:754: cmd_line->AppendArg("--server=start");
On 2011/05/13 16:45:05, Evan Martin wrote:
> I worry that, without a comment, someone will helpfully "refactor" this to use
> AppendSwitchASCII.

Done.

[Evan Martin, 2011-05-13 19:09:41]

LGTM

thanks for doing this!

http://codereview.chromium.org/6526040/diff/37005/base/command_line.h
File base/command_line.h (right):

http://codereview.chromium.org/6526040/diff/37005/base/command_line.h#newcode167
base/command_line.h:167: // The index after the program and switches, any
agruments start here.
typo: arguments

[grt (UTC plus 2), 2011-05-13 19:16:14]

LGTM with some nits.  Cheers,

Greg

http://codereview.chromium.org/6526040/diff/37005/base/command_line.cc
File base/command_line.cc (right):

http://codereview.chromium.org/6526040/diff/37005/base/command_line.cc#newcode58
base/command_line.cc:58: if(equals_position != CommandLine::StringType::npos)
style: add a space after "if"

http://codereview.chromium.org/6526040/diff/37005/base/command_line.cc#newcode73
base/command_line.cc:73: parse_switches &= arg != kSwitchTerminator;
readability: parens around arg != kSwitchTerminator

http://codereview.chromium.org/6526040/diff/37005/base/command_line.cc#newcod...
base/command_line.cc:248: #if defined(OS_WIN)
It seems like some of these conditional compilation blocks could be removed
fairly easily.  How about defining a platform-specific helper func that takes a
std::string and returns an appropriate switch type?

http://codereview.chromium.org/6526040/diff/37005/base/command_line.cc#newcod...
base/command_line.cc:330: for (size_t i = 0; i < count; ++i)
need braces here

http://codereview.chromium.org/6526040/diff/37005/base/command_line.cc#newcod...
base/command_line.cc:393: CHECK(args);
Please use the following (or something similar) so that we get nice diagnostics
in logs:

PLOG_IF(FATAL, !args) << "CommandLineToArgvW failed on command line: " <<
command_line;

[msw, 2011-05-13 20:28:02]

Done, will land after tests, and during quieter hours.

http://codereview.chromium.org/6526040/diff/37005/base/command_line.cc
File base/command_line.cc (right):

http://codereview.chromium.org/6526040/diff/37005/base/command_line.cc#newcode58
base/command_line.cc:58: if(equals_position != CommandLine::StringType::npos)
On 2011/05/13 19:16:14, grt wrote:
> style: add a space after "if"

Done.

http://codereview.chromium.org/6526040/diff/37005/base/command_line.cc#newcode73
base/command_line.cc:73: parse_switches &= arg != kSwitchTerminator;
On 2011/05/13 19:16:14, grt wrote:
> readability: parens around arg != kSwitchTerminator

Done.

http://codereview.chromium.org/6526040/diff/37005/base/command_line.cc#newcod...
base/command_line.cc:248: #if defined(OS_WIN)
On 2011/05/13 19:16:14, grt wrote:
> It seems like some of these conditional compilation blocks could be removed
> fairly easily.  How about defining a platform-specific helper func that takes
a
> std::string and returns an appropriate switch type?

Done, reincarnated Lowercase as LowerASCIIOnWindows.
Both string and wstring will eventually become string16, so I left those
preprocessor blocks alone for now.

http://codereview.chromium.org/6526040/diff/37005/base/command_line.cc#newcod...
base/command_line.cc:330: for (size_t i = 0; i < count; ++i)
On 2011/05/13 19:16:14, grt wrote:
> need braces here

Done.

http://codereview.chromium.org/6526040/diff/37005/base/command_line.cc#newcod...
base/command_line.cc:393: CHECK(args);
On 2011/05/13 19:16:14, grt wrote:
> Please use the following (or something similar) so that we get nice
diagnostics
> in logs:
> 
> PLOG_IF(FATAL, !args) << "CommandLineToArgvW failed on command line: " <<
> command_line;

Done.

http://codereview.chromium.org/6526040/diff/37005/base/command_line.h
File base/command_line.h (right):

http://codereview.chromium.org/6526040/diff/37005/base/command_line.h#newcode167
base/command_line.h:167: // The index after the program and switches, any
agruments start here.
On 2011/05/13 19:09:41, Evan Martin wrote:
> typo: arguments

Done.