CommandLine API rework

Make CommandLine into a normal object, with some statics for getting at the current process's command line.

One explicit goal of this change is to *not* deal with the string/wstring issues at the API on POSIX; the functions are the same as before, which means they remain as broken as before.  (I did try to fix the internals, though, so migrating the callers is now possible by adding platform-appropriate hooks.)

[Evan Stade, 2009-01-14 23:33:29]

I don't understand why there can't be both (a) a global CommandLine object that
represents the command line that started this executable  (i.e. a singleton) and
(b) non-global CommandLine isntances that represent arbitrary command lines.

If we only want to use it as a singleton then why are there public constructors
for posix?

[Evan Martin, 2009-01-15 00:09:06]

On 2009/01/14 23:33:29, Evan Stade wrote:
> I don't understand why there can't be both (a) a global CommandLine object
that
> represents the command line that started this executable  (i.e. a singleton)
and
> (b) non-global CommandLine isntances that represent arbitrary command lines.

I leave this one to Darin.

> If we only want to use it as a singleton then why are there public
constructors
> for posix?

I believe the intent was it would be constructed from main(), and that's it.

[Evan Stade, 2009-01-15 00:10:59]

On 2009/01/15 00:09:06, Evan Martin wrote:
> On 2009/01/14 23:33:29, Evan Stade wrote:
> > I don't understand why there can't be both (a) a global CommandLine object
> that
> > represents the command line that started this executable  (i.e. a singleton)
> and
> > (b) non-global CommandLine isntances that represent arbitrary command lines.
> 
> I leave this one to Darin.
> 
> > If we only want to use it as a singleton then why are there public
> constructors
> > for posix?
> 
> I believe the intent was it would be constructed from main(), and that's it.

You don't need it then, as you can just call SetArgcArgv() from main followed by
CommandLine().

[Evan Stade, 2009-01-15 00:55:37]

It looks like the only place the non-default constructors are used is the
command line unit test. I guess that explains their existence. (how else to test
the parsing?)

I think the thing to do is to wrap up this change with your CommandLineBuilder
change, make the constructors that you need for that class private (with
CommandLineBuilder a friend), delete the current command line unittests and
replace them with CommandLineBuilder unittests.

[Evan Stade, 2009-01-15 01:00:25]

(there are actually a couple places the non-default constructors are used:
browser_init.cc, shell_integration.cc. Either these should be changed or
CommandLine is not just a singleton.)

[rvargas (doing something else), 2009-01-15 01:01:38]

I'm also missing what's the problem with using one of the constructors that has
arguments. It seems that this class only deals with the singleton and the actual
command line of this app when the default constructor is used.

On 2009/01/15 00:10:59, Evan Stade wrote:
> On 2009/01/15 00:09:06, Evan Martin wrote:
> > On 2009/01/14 23:33:29, Evan Stade wrote:
> > > I don't understand why there can't be both (a) a global CommandLine object
> > that
> > > represents the command line that started this executable  (i.e. a
singleton)
> > and
> > > (b) non-global CommandLine isntances that represent arbitrary command
lines.
> > 
> > I leave this one to Darin.
> > 
> > > If we only want to use it as a singleton then why are there public
> > constructors
> > > for posix?
> > 
> > I believe the intent was it would be constructed from main(), and that's it.
> 
> You don't need it then, as you can just call SetArgcArgv() from main followed
by
> CommandLine().

[Evan Martin, 2009-01-15 01:22:59]

On 2009/01/15 01:01:38, rvargas wrote:
> I'm also missing what's the problem with using one of the constructors that
has
> arguments. It seems that this class only deals with the singleton and the
actual
> command line of this app when the default constructor is used.

rvargas: the implementation of CommandLine is with a singleton, so you can't
have multiple different ones flying around in the same code.  But you need
multiple CommandLines to spawn renderers.

[rvargas (doing something else), 2009-01-15 01:28:25]

On 2009/01/15 01:22:59, Evan Martin wrote:
> On 2009/01/15 01:01:38, rvargas wrote:
> > I'm also missing what's the problem with using one of the constructors that
> has
> > arguments. It seems that this class only deals with the singleton and the
> actual
> > command line of this app when the default constructor is used.
> 
> rvargas: the implementation of CommandLine is with a singleton, so you can't
> have multiple different ones flying around in the same code.  But you need
> multiple CommandLines to spawn renderers.

that's what I meant... I don't see the implementation causing any trouble right
now because the singleton is not used with objects that are created with an
explicit argument (I'm sure I'm missing something)

[Evan Martin, 2009-01-15 22:51:44]

Ok, here's something that builds on Linux.  Gonna do the work on Windows too
now, but it should be enough for you to get a feeling for it.

I think I need to split command_line.cc into _win and _posix.

[Evan Martin, 2009-01-15 22:59:00]

updated with a better description of what i tried to do

[darin (slow to review), 2009-01-15 23:04:25]

http://codereview.chromium.org/18248/diff/29/31
File base/command_line.h (right):

http://codereview.chromium.org/18248/diff/29/31#newcode44
Line 44: CommandLine(const std::wstring& program);
doesn't this signature overlap with the OS_WIN one defined above?

http://codereview.chromium.org/18248/diff/29/31#newcode50
Line 50: static void Init(int argc, const char* const* argv);
how do i call this on windows?  it seems really strange for the windows version
to ignore these parameters.

[Evan Martin, 2009-01-15 23:10:14]

http://codereview.chromium.org/18248/diff/29/31
File base/command_line.h (right):

http://codereview.chromium.org/18248/diff/29/31#newcode44
Line 44: CommandLine(const std::wstring& program);
On 2009/01/15 23:04:25, darin wrote:
> doesn't this signature overlap with the OS_WIN one defined above?

Yeah.  :(
Will have to fix.  I guess I put a "program" setter on it or something.  I just
was trying to avoid the empty constructor so the compiler helps me find all
callers...

http://codereview.chromium.org/18248/diff/29/31#newcode50
Line 50: static void Init(int argc, const char* const* argv);
On 2009/01/15 23:04:25, darin wrote:
> how do i call this on windows?  it seems really strange for the windows
version
> to ignore these parameters.

From looking at test_shell_main, it seems we have argc and argv?  I agree it is
weird to ignore those parameters, but the alternative is for every caller to do
#if defined(OS_WIN)
  CommandLine::Init();
#else
  CommandLine::Init(argc, argv);
#endif

Maybe that's preferable?

[darin (slow to review), 2009-01-15 23:17:08]

http://codereview.chromium.org/18248/diff/29/31
File base/command_line.h (right):

http://codereview.chromium.org/18248/diff/29/31#newcode50
Line 50: static void Init(int argc, const char* const* argv);
Oh, welcome to windows...

In windows you can have CONSOLE apps and WINDOWS apps.  TestShell is a CONSOLE
app, which explains why you get a DOS prompt in addition to the TestShell
windows.  Chrome is not a CONSOLE app.  CONSOLE apps have a normal "main"
function with argc and argv, but WINDOWS apps have a WinMain function which is
just pass a null terminated wchar_t array.  The argc/argv passed to main is
actually generated by the CRT.

I agree that having an Init with arguments is better on Windows, or you could
allow a "const wchar_t*" argument, which can optionally be NULL to cause Init to
use the value returned by GetCommandLineW.

[Evan Martin, 2009-01-15 23:31:17]

btw, it looks like i nuked command_line.cc because i reindented some functions,
sorry.  :(

http://codereview.chromium.org/18248/diff/29/31
File base/command_line.h (right):

http://codereview.chromium.org/18248/diff/29/31#newcode92
Line 92: // Appends the given switch string (preceded by a space and a switch
these comments are now incorrect, will fix

[Evan Martin, 2009-01-16 02:33:54]

Ok, this basically works on all platforms, and I've re-uploaded with a merge
against trunk, so you should look at the patch afresh from here and I'll try to
make future changes small enough that you can look at the diffs from here on.

[Evan Stade, 2009-01-16 18:05:12]

mainly just nits. LG for the most part

http://codereview.chromium.org/18248/diff/246/247
File base/command_line.cc (right):

http://codereview.chromium.org/18248/diff/246/247#newcode173
Line 173: current_process_commandline_->ParseFromString(GetCommandLineW());
nit: :: in front of Windows system calls

http://codereview.chromium.org/18248/diff/246/247#newcode216
Line 216: return command_line_string_;
nit: feels like this should be in the header. Doesn't belong in this little
block since there's no posix equivalent. But I don't feel strongly.

http://codereview.chromium.org/18248/diff/246/247#newcode305
Line 305: argv_.push_back("--" + ascii_switch + '=' + ascii_value);
nit: use kSwitchPrefixes[0] (here and above) and kSwitchValueSeparator[0]

http://codereview.chromium.org/18248/diff/246/248
File base/command_line.h (right):

http://codereview.chromium.org/18248/diff/246/248#newcode44
Line 44: explicit CommandLine(const std::wstring& program);
why do you need to special case this? i.e. can't you just use the
ParseFromString() constructor with only a program name in the parameter (and
then you can change that one back to a constructor)

http://codereview.chromium.org/18248/diff/246/248#newcode55
Line 55: return current_process_commandline_;
since you always dereference the return value when you call this function, it
seems like it could return a CommandLine& and just dereference the pointer once,
here. That would be a lot of work to change now though, so it's no biggie.

Maybe there should be a DCHECK with warning message here to make sure
current_process_commandline_ is non-null?

http://codereview.chromium.org/18248/diff/246/248#newcode125
Line 125: // The argv array, with the program name in argv[0].
nit: argv_[0]

http://codereview.chromium.org/18248/diff/246/248#newcode136
Line 136: // if parameter_string represents a switch.
nit: |parameter_string|

http://codereview.chromium.org/18248/diff/246/248#newcode144
Line 144: // Extra command-line arguments.
nit: I don't get this comment. This is just non-switch arguments, right? "Extra"
makes them sound like arguments the program wasn't expecting.

[Evan Martin, 2009-01-16 23:47:24]

trybots pass this on mac and linux, and it was working for me on windows earlier
but i'm awaiting the trybots

http://codereview.chromium.org/18248/diff/246/247
File base/command_line.cc (right):

http://codereview.chromium.org/18248/diff/246/247#newcode173
Line 173: current_process_commandline_->ParseFromString(GetCommandLineW());
On 2009/01/16 18:05:12, Evan Stade wrote:
> nit: :: in front of Windows system calls

Done.

http://codereview.chromium.org/18248/diff/246/247#newcode216
Line 216: return command_line_string_;
On 2009/01/16 18:05:12, Evan Stade wrote:
> nit: feels like this should be in the header. Doesn't belong in this little
> block since there's no posix equivalent. But I don't feel strongly.

Done.

http://codereview.chromium.org/18248/diff/246/247#newcode305
Line 305: argv_.push_back("--" + ascii_switch + '=' + ascii_value);
On 2009/01/16 18:05:12, Evan Stade wrote:
> nit: use kSwitchPrefixes[0] (here and above) and kSwitchValueSeparator[0]

Done.

http://codereview.chromium.org/18248/diff/246/248
File base/command_line.h (right):

http://codereview.chromium.org/18248/diff/246/248#newcode44
Line 44: explicit CommandLine(const std::wstring& program);
On 2009/01/16 18:05:12, Evan Stade wrote:
> why do you need to special case this? i.e. can't you just use the
> ParseFromString() constructor with only a program name in the parameter (and
> then you can change that one back to a constructor)

The intent is that this one eventually will take a FilePath, while the
ParseFromString one is Windows-specific.  I'm not too sure on this, though...

http://codereview.chromium.org/18248/diff/246/248#newcode55
Line 55: return current_process_commandline_;
On 2009/01/16 18:05:12, Evan Stade wrote:
> since you always dereference the return value when you call this function, it
> seems like it could return a CommandLine& and just dereference the pointer
once,
> here. That would be a lot of work to change now though, so it's no biggie.
> 
> Maybe there should be a DCHECK with warning message here to make sure
> current_process_commandline_ is non-null?

Done.

http://codereview.chromium.org/18248/diff/246/248#newcode125
Line 125: // The argv array, with the program name in argv[0].
On 2009/01/16 18:05:12, Evan Stade wrote:
> nit: argv_[0]

Done.

http://codereview.chromium.org/18248/diff/246/248#newcode136
Line 136: // if parameter_string represents a switch.
On 2009/01/16 18:05:12, Evan Stade wrote:
> nit: |parameter_string|

Done.

http://codereview.chromium.org/18248/diff/246/248#newcode144
Line 144: // Extra command-line arguments.
On 2009/01/16 18:05:12, Evan Stade wrote:
> nit: I don't get this comment. This is just non-switch arguments, right?
"Extra"
> makes them sound like arguments the program wasn't expecting.

Done.

[Evan Stade, 2009-01-17 00:45:38]

I'm confused by AppendArguments(). It seems you only want it to be used with
CommandLine objects that have no program, but this is not documented anywhere
and seems easy to misuse.

I guess add a comment and a DCHECK() to make sure program() returns an empty
string?

http://codereview.chromium.org/18248/diff/137/601
File base/command_line.cc (right):

http://codereview.chromium.org/18248/diff/137/601#newcode291
Line 291: command_line_string_ += L" " + other.command_line_string_;
The comment in the header says this appends just the arguments from the other
command line, but this looks like it appends the program as well. (same below)

Also, how come you can get away without updating |switches_|?

http://codereview.chromium.org/18248/diff/137/602
File base/command_line.h (right):

http://codereview.chromium.org/18248/diff/137/602#newcode71
Line 71: const std::vector<std::wstring> GetLooseValues() const;
do you need the first const? does it have any meaning without &?

[Evan Martin, 2009-01-20 23:58:50]

Ok, now passes ui tests on windows.  I think this one is ready to commit,
barring some last-second failures that will likely be small changes.

http://codereview.chromium.org/18248/diff/137/601
File base/command_line.cc (right):

http://codereview.chromium.org/18248/diff/137/601#newcode291
Line 291: command_line_string_ += L" " + other.command_line_string_;
On 2009/01/17 00:45:38, Evan Stade wrote:
> The comment in the header says this appends just the arguments from the other
> command line, but this looks like it appends the program as well. (same below)
> 
> Also, how come you can get away without updating |switches_|?

Good points, fixed.

http://codereview.chromium.org/18248/diff/137/602
File base/command_line.h (right):

http://codereview.chromium.org/18248/diff/137/602#newcode71
Line 71: const std::vector<std::wstring> GetLooseValues() const;
On 2009/01/17 00:45:38, Evan Stade wrote:
> do you need the first const? does it have any meaning without &?

Nope, you're right, it's totally useless.  Fixed.

[Evan Stade, 2009-01-21 00:18:21]

http://codereview.chromium.org/18248/diff/335/1147
File base/command_line.cc (right):

http://codereview.chromium.org/18248/diff/335/1147#newcode325
Line 325: bool include_program) {
why don't you DCHECK() here as above in the windows code? argv_[0] can be empty,
can't it?

http://codereview.chromium.org/18248/diff/335/1147#newcode329
Line 329: for (i = other.switches_.begin(); i != other.switches_.end(); ++i)
missing |i| declaration (though I'm sure you'd have caught that)

[agl, 2009-01-21 00:28:46]

LGTM

[Evan Martin, 2009-01-21 00:32:31]

estade: fixed

[Evan Stade, 2009-01-21 00:33:49]

LGTM