Reorganize CommandLine code.

Reorganize CommandLine code.
The majority of these changes are purely moving code.
Conducted some minor (and hopefully straightforward) refactoring.

BUG=73195
TEST=none

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

[msw, 2011-02-27 16:30:12]

Please take a look.
This is just the first step of my planned cleanup.
The end goal is akin to the change that this supersedes:
http://codereview.chromium.org/6526040/

[Paweł Hajdan Jr., 2011-02-28 11:11:40]

Thank you. I only reviewed the testing-related code, and deferred full review to
Evan.

http://codereview.chromium.org/6596020/diff/4001/base/command_line.h
File base/command_line.h (right):

http://codereview.chromium.org/6596020/diff/4001/base/command_line.h#newcode40
base/command_line.h:40: // Enum & constructor for CommandLines that only carry
switches and arguments.
nit: I think we avoid any kind of "abbreviations", and that might include "&" in
context of "and". I suggest to replace it with just "and".

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

http://codereview.chromium.org/6596020/diff/4001/chrome/test/in_process_brows...
chrome/test/in_process_browser_test.cc:107: CommandLine* command_line =
CommandLine::ForCurrentProcess();
nit: If you take a look at the TODO above, I think this logic can be safely
removed. You're doing a cleanup already, so I'd appreciate including that in the
CL.

It's no longer needed because we run all InProcessBrowserTests in separate
processes, so we don't have to save the original command line for the "next
test", because no "next" test will run in the same address space.

[msw, 2011-02-28 19:42:03]

Comments addressed; win&linux HeapProfiler failures were on r76221 mainline,
this change wasn't at fault.

http://codereview.chromium.org/6596020/diff/4001/base/command_line.h
File base/command_line.h (right):

http://codereview.chromium.org/6596020/diff/4001/base/command_line.h#newcode40
base/command_line.h:40: // Enum & constructor for CommandLines that only carry
switches and arguments.
On 2011/02/28 11:11:41, Paweł Hajdan Jr. wrote:
> nit: I think we avoid any kind of "abbreviations", and that might include "&"
in
> context of "and". I suggest to replace it with just "and".

Done.

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

http://codereview.chromium.org/6596020/diff/4001/chrome/test/in_process_brows...
chrome/test/in_process_browser_test.cc:107: CommandLine* command_line =
CommandLine::ForCurrentProcess();
On 2011/02/28 11:11:41, Paweł Hajdan Jr. wrote:
> nit: If you take a look at the TODO above, I think this logic can be safely
> removed. You're doing a cleanup already, so I'd appreciate including that in
the
> CL.
> 
> It's no longer needed because we run all InProcessBrowserTests in separate
> processes, so we don't have to save the original command line for the "next
> test", because no "next" test will run in the same address space.

Done. Is trybot coverage (including IN_PROC_BROWSER_TEST_F tests) sufficient, or
should we test this change in another way? Also, does this mean that
original_single_process_ is unnecessary?

[Paweł Hajdan Jr., 2011-02-28 21:01:16]

chrome/test code LGTM with a comment.

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

http://codereview.chromium.org/6596020/diff/4001/chrome/test/in_process_brows...
chrome/test/in_process_browser_test.cc:107: CommandLine* command_line =
CommandLine::ForCurrentProcess();
On 2011/02/28 19:42:03, msw wrote:
> Done. Is trybot coverage (including IN_PROC_BROWSER_TEST_F tests) sufficient,
or
> should we test this change in another way? Also, does this mean that
> original_single_process_ is unnecessary?

Trybots are sufficient IMO. Yeah, original_single_process_ should also be
unnecessary now, good catch. Please remove it.

[evanm, 2011-02-28 21:04:19]

I think this is generally good, just have a few questions.

http://codereview.chromium.org/6596020/diff/2005/base/command_line.cc
File base/command_line.cc (right):

http://codereview.chromium.org/6596020/diff/2005/base/command_line.cc#newcode53
base/command_line.cc:53: CommandLine::StringType WindowsStyleQuote(const
CommandLine::StringType& arg) {
Here's another case where I think the typedef makes it harder to read.  The type
of this function is the same regardless of any compile-time settings: it's a
Windows quote function, so it takes a Windows string.

http://codereview.chromium.org/6596020/diff/2005/base/command_line.cc#newcode330
base/command_line.cc:330: #endif
Is the intent that these code paths will become more similar in the future? 
Because otherwise it might be easier to follow if the functions were distinct,
since their bodies are totally different.

http://codereview.chromium.org/6596020/diff/2005/base/command_line.h
File base/command_line.h (left):

http://codereview.chromium.org/6596020/diff/2005/base/command_line.h#oldcode201
base/command_line.h:201: // We allow copy constructors, because a common pattern
is to grab a
I think this comment was ok to leave in.  It's saying why we don't have the
DISALLOW_COPY_AND_ASSIGN that is normally required on all classes.

(FWIW, I'd prefer to get rid of the copy ctor anyway.)

http://codereview.chromium.org/6596020/diff/2005/base/command_line.h
File base/command_line.h (right):

http://codereview.chromium.org/6596020/diff/2005/base/command_line.h#newcode70
base/command_line.h:70: static CommandLine FromString(const StringType&
command_line);
Could use std::wstring here, might be clearer.  Like how you use char* below for
argv.

Edit: I see you're switching to StringType throughout.  I guess it might make
switching to string16 easier.  I feel like it's usually clearer to not need to
read through typedefs for this kind of thing but I'll defer to your judgement.

http://codereview.chromium.org/6596020/diff/2005/base/command_line.h#newcode79
base/command_line.h:79: // Returns the represented command line string.
Can you stick a note on this about how it shouldn't be used because the quoting
is unclear?

http://codereview.chromium.org/6596020/diff/2005/base/command_line.h#newcode101
base/command_line.h:101: size_t GetSwitchCount() const;
Ugh, I wish we didn't have all this API.  This could just be
GetSwitches().size(); any caller that needs to know the switch count probably
wants to iterate through the map anyway.  Something to change later.

http://codereview.chromium.org/6596020/diff/2005/base/command_line.h#newcode107
base/command_line.h:107: // CAUTION! Appending a switch after the "--"
kSwitchTerminator is futile!
It appears that kSwitchTerminator doesn't exist in this header.  Might be
clearer to just call it "switch terminator" or somethign.

http://codereview.chromium.org/6596020/diff/2005/base/command_line.h#newcode138
base/command_line.h:138: //Common for debuggers, like "valgrind" or "gdb
--args".
Space after //

[msw, 2011-02-28 23:44:47]

Addressed comments.

Evan, everything will use string16 via subsequent change(s) this week. Failing
that, I think using the StringType/CharType typedefs *in cross-platform code* is
desirable. What are your thoughts on that plan, and do you dislike the
StringVector/SwitchMap typedefs?

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

http://codereview.chromium.org/6596020/diff/4001/chrome/test/in_process_brows...
chrome/test/in_process_browser_test.cc:107: CommandLine* command_line =
CommandLine::ForCurrentProcess();
On 2011/02/28 21:01:16, Paweł Hajdan Jr. wrote:
> On 2011/02/28 19:42:03, msw wrote:
> > Done. Is trybot coverage (including IN_PROC_BROWSER_TEST_F tests)
sufficient,
> or
> > should we test this change in another way? Also, does this mean that
> > original_single_process_ is unnecessary?
> 
> Trybots are sufficient IMO. Yeah, original_single_process_ should also be
> unnecessary now, good catch. Please remove it.

Done.

http://codereview.chromium.org/6596020/diff/2005/base/command_line.cc
File base/command_line.cc (right):

http://codereview.chromium.org/6596020/diff/2005/base/command_line.cc#newcode53
base/command_line.cc:53: CommandLine::StringType WindowsStyleQuote(const
CommandLine::StringType& arg) {
On 2011/02/28 21:04:19, evanm wrote:
> Here's another case where I think the typedef makes it harder to read.  The
type
> of this function is the same regardless of any compile-time settings: it's a
> Windows quote function, so it takes a Windows string.

Reverted for now; will become string16 in another change.

http://codereview.chromium.org/6596020/diff/2005/base/command_line.cc#newcode330
base/command_line.cc:330: #endif
On 2011/02/28 21:04:19, evanm wrote:
> Is the intent that these code paths will become more similar in the future? 
> Because otherwise it might be easier to follow if the functions were distinct,
> since their bodies are totally different.

Yes, the code will become a single cross-platform block in my subsequent change;
I left this alone for now.

http://codereview.chromium.org/6596020/diff/2005/base/command_line.h
File base/command_line.h (left):

http://codereview.chromium.org/6596020/diff/2005/base/command_line.h#oldcode201
base/command_line.h:201: // We allow copy constructors, because a common pattern
is to grab a
On 2011/02/28 21:04:19, evanm wrote:
> I think this comment was ok to leave in.  It's saying why we don't have the
> DISALLOW_COPY_AND_ASSIGN that is normally required on all classes.
> 
> (FWIW, I'd prefer to get rid of the copy ctor anyway.)

Done. Was moved above; but I see this is where we normally put
DISALLOW_COPY_AND_ASSIGN, etc. So I moved it back here.

http://codereview.chromium.org/6596020/diff/2005/base/command_line.h
File base/command_line.h (right):

http://codereview.chromium.org/6596020/diff/2005/base/command_line.h#newcode70
base/command_line.h:70: static CommandLine FromString(const StringType&
command_line);
On 2011/02/28 21:04:19, evanm wrote:
> Could use std::wstring here, might be clearer.  Like how you use char* below
for
> argv.
> 
> Edit: I see you're switching to StringType throughout.  I guess it might make
> switching to string16 easier.  I feel like it's usually clearer to not need to
> read through typedefs for this kind of thing but I'll defer to your judgement.

I'll use string16 everywhere and remove typedefs (PKasting's suggestion) in
another change. I reverted this function for now. What are your thoughts on that
plan, and do you dislike StringVector/SwitchMap?

http://codereview.chromium.org/6596020/diff/2005/base/command_line.h#newcode79
base/command_line.h:79: // Returns the represented command line string.
On 2011/02/28 21:04:19, evanm wrote:
> Can you stick a note on this about how it shouldn't be used because the
quoting
> is unclear?

Done.

http://codereview.chromium.org/6596020/diff/2005/base/command_line.h#newcode101
base/command_line.h:101: size_t GetSwitchCount() const;
On 2011/02/28 21:04:19, evanm wrote:
> Ugh, I wish we didn't have all this API.  This could just be
> GetSwitches().size(); any caller that needs to know the switch count probably
> wants to iterate through the map anyway.  Something to change later.

Agreed; added // TODO(msw): Remove unnecessary API.

http://codereview.chromium.org/6596020/diff/2005/base/command_line.h#newcode107
base/command_line.h:107: // CAUTION! Appending a switch after the "--"
kSwitchTerminator is futile!
On 2011/02/28 21:04:19, evanm wrote:
> It appears that kSwitchTerminator doesn't exist in this header.  Might be
> clearer to just call it "switch terminator" or somethign.

Done.

http://codereview.chromium.org/6596020/diff/2005/base/command_line.h#newcode138
base/command_line.h:138: //Common for debuggers, like "valgrind" or "gdb
--args".
On 2011/02/28 21:04:19, evanm wrote:
> Space after //

Done.

[evanm, 2011-03-01 00:06:53]

LGTM

I like StringVector etc.

[msw, 2011-03-01 01:26:05]

ExtensionDevToolsBrowserTest.ProcessRefCounting looks like a flake, passes
locally.