Store the command line in a more convenient format on non-windows platforms.

Store the command line in a more convenient format on non-windows platforms.


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

[Evan Stade, 2008-10-11 00:48:00]

These changes are necessary (or helpful) for porting LaunchApp without passing
the command line as a string. (related to issue 7202--but neither changelist is
dependent on the other)

[Amanda Walker, 2008-10-11 01:29:30]

Drive-by review: I'd be mildly more comfortable if the vector made copies of the
arguments (as the rest of the CommandLine class does) rather than relying on
argc and argv being untouched for the life of the process.  I won't hold it up
if the other reviewers don't mind, though.

[Evan Stade, 2008-10-14 17:39:45]

Any other opinions on this matter? Should the strings be copied or is it enough
to copy the pointers?

[Evan Martin, 2008-10-14 23:28:05]

http://codereview.chromium.org/7249/diff/6/8
File base/command_line.h (right):

http://codereview.chromium.org/7249/diff/6/8#newcode72
Line 72: std::vector<const char*> argv() const;
I agree with Amanda.  Stuff like GTK likes to mess with the argv array directly.

(PS: This should return a const reference, rather than copying the vector.)

http://codereview.chromium.org/7249/diff/6/8#newcode109
Line 109: private:
I think the spacing was correct before... ?

http://codereview.chromium.org/7249/diff/6/9
File base/command_line_unittest.cc (right):

http://codereview.chromium.org/7249/diff/6/9#newcode80
Line 80: const char ** argv_ = argv;
We typically use "foo_" for member variables.  I think this code could be made
more clear by just using a size_t to loop through the indices of the arrays.

http://codereview.chromium.org/7249/diff/6/9#newcode131
Line 131: argv.push_back(s1.c_str());
Here if argv were strings (instead of char*s) you wouldn't need all these
temporary s1, s2 variables.

http://codereview.chromium.org/7249/diff/6/9#newcode151
Line 151: // quotes get handled differently in the different CommandLine
constructors
Is this a bug?  A feature?  Can you describe how they're different in this
comment?

[Mark Mentovai, 2008-10-15 03:14:15]

http://codereview.chromium.org/7249/diff/401/11
File base/command_line.cc (right):

http://codereview.chromium.org/7249/diff/401/11#newcode128
Line 128: char* prog = new char[strlen(argv[0]) + 1];
This is all kind of nutty.  Can we just make argv_ be std::vector<std::string>? 
I don't love having to deal with all of these allocated arrays, doing the
alloc/copy, and all that stuff.

http://codereview.chromium.org/7249/diff/401/11#newcode140
Line 140: command_line_string_.append(arg);
(rant about existing code)

I hate command_line_string_ on POSIX platforms and hope we can kill it.

http://codereview.chromium.org/7249/diff/401/13
File base/command_line_unittest.cc (right):

http://codereview.chromium.org/7249/diff/401/13#newcode128
Line 128: WideToUTF8(CommandLine::PrefixedSwitchString(switch1));
Style nit: continuation lines should be indented four spaces relative to what
they're continuing.

http://codereview.chromium.org/7249/diff/401/13#newcode138
Line 138: switch4, value4.substr(1, value4.length() - 2)));
and you should do something better with the indentation here, it looks like you
just tried to make the last character flush with the right margin.

[Evan Stade, 2008-10-15 21:02:30]

http://codereview.chromium.org/7249/diff/6/8
File base/command_line.h (right):

http://codereview.chromium.org/7249/diff/6/8#newcode72
Line 72: std::vector<const char*> argv() const;
On 2008/10/14 23:28:05, Evan Martin wrote:
> I agree with Amanda.  Stuff like GTK likes to mess with the argv array
directly.
> 
> (PS: This should return a const reference, rather than copying the vector.)

Done.

http://codereview.chromium.org/7249/diff/6/8#newcode109
Line 109: private:
On 2008/10/14 23:28:05, Evan Martin wrote:
> I think the spacing was correct before... ?

whoops, unintentional

http://codereview.chromium.org/7249/diff/6/9
File base/command_line_unittest.cc (right):

http://codereview.chromium.org/7249/diff/6/9#newcode80
Line 80: const char ** argv_ = argv;
On 2008/10/14 23:28:05, Evan Martin wrote:
> We typically use "foo_" for member variables.  I think this code could be made
> more clear by just using a size_t to loop through the indices of the arrays.

Done.

http://codereview.chromium.org/7249/diff/6/9#newcode131
Line 131: argv.push_back(s1.c_str());
On 2008/10/14 23:28:05, Evan Martin wrote:
> Here if argv were strings (instead of char*s) you wouldn't need all these
> temporary s1, s2 variables.

but then the constructor on line 142 wouldn't work. Added a new constructor for
this reason.

http://codereview.chromium.org/7249/diff/6/9#newcode151
Line 151: // quotes get handled differently in the different CommandLine
constructors
On 2008/10/14 23:28:05, Evan Martin wrote:
> Is this a bug?  A feature?  Can you describe how they're different in this
> comment?

This is a feature, and not one that I added. It's because the windows
constructor uses CommandLineToArgvW(), which strips quotes (uses them to
tokenize). If the user of a posix CommandLine object wants to have a value
without quotes, they should strip them themselves.

So that's what I did (line 139).

http://codereview.chromium.org/7249/diff/401/11
File base/command_line.cc (right):

http://codereview.chromium.org/7249/diff/401/11#newcode128
Line 128: char* prog = new char[strlen(argv[0]) + 1];
On 2008/10/15 03:14:16, Mark Mentovai wrote:
> This is all kind of nutty.  Can we just make argv_ be
std::vector<std::string>? 
> I don't love having to deal with all of these allocated arrays, doing the
> alloc/copy, and all that stuff.

I was trying to improve compatibility with interfaces that may expect char**,
but general opinion seems to be the cleanliness of string is more important.
Changed.

http://codereview.chromium.org/7249/diff/401/13
File base/command_line_unittest.cc (right):

http://codereview.chromium.org/7249/diff/401/13#newcode128
Line 128: WideToUTF8(CommandLine::PrefixedSwitchString(switch1));
On 2008/10/15 03:14:16, Mark Mentovai wrote:
> Style nit: continuation lines should be indented four spaces relative to what
> they're continuing.

Done.

http://codereview.chromium.org/7249/diff/401/13#newcode138
Line 138: switch4, value4.substr(1, value4.length() - 2)));
On 2008/10/15 03:14:16, Mark Mentovai wrote:
> and you should do something better with the indentation here, it looks like
you
> just tried to make the last character flush with the right margin.

changed

[Evan Martin, 2008-10-15 21:54:39]

LGTM, one nit

http://codereview.chromium.org/7249/diff/18/403
File base/command_line.h (right):

http://codereview.chromium.org/7249/diff/18/403#newcode72
Line 72: // Returns the original command line string as a vector of c-strings.
This comment is no longer accurate.  ("c-strings" should be just "strings")

[Evan Stade, 2008-10-15 21:59:28]

On 2008/10/15 21:54:39, Evan Martin wrote:
> LGTM, one nit
> 
> http://codereview.chromium.org/7249/diff/18/403
> File base/command_line.h (right):
> 
> http://codereview.chromium.org/7249/diff/18/403#newcode72
> Line 72: // Returns the original command line string as a vector of c-strings.
> This comment is no longer accurate.  ("c-strings" should be just "strings")

fixed and committed