Extract arguments reconstruction logic out of GetCommandLineString() into GetArgumentsString().

Extract arguments reconstruction logic out of GetCommandLineString() into GetArgumentsString().


BUG=To get arguments we had to GetCommandLineString() and piece out the program: http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/installer/util/install_util.cc&exact_package=chromium&q=GetCommandLineString&type=cs&l=125
TEST=base_unittests.exe --gtest_filter=CommandLineTest*


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

[gab, 2012-10-26 18:57:59]

Sir, what say you :)?

http://codereview.chromium.org/11305010/diff/1/base/command_line.h
File base/command_line.h (right):

http://codereview.chromium.org/11305010/diff/1/base/command_line.h#newcode82
base/command_line.h:82: // CAUTION! This should be avoided on POSIX because
quoting behavior is
This CAUTION message was inserted a whillleee back
(http://src.chromium.org/viewvc/chrome/trunk/src/base/command_line.cc?r1=76339...);
back when command_line_string was still a simple getter. I think it's ok now for
OS_WIN; not sure for POSIX, so I'll leave the warning for POSIX, but we use this
in production on Windows... so it better work...!

[robertshield, 2012-10-29 02:04:43]

lgtm, this changes looks fine to me with a small amount of extra testing as per
comments. 

This said, run it by a base owner and see what they say. Cleaning up the current
pattern of creating a secondary CommandLine object to extract the arguments
seems worthwhile to me, hopefully the OWNER will agree.

http://codereview.chromium.org/11305010/diff/1/base/command_line.h
File base/command_line.h (right):

http://codereview.chromium.org/11305010/diff/1/base/command_line.h#newcode82
base/command_line.h:82: // CAUTION! This should be avoided on POSIX because
quoting behavior is
On 2012/10/26 18:57:59, gab wrote:
> This CAUTION message was inserted a whillleee back
>
(http://src.chromium.org/viewvc/chrome/trunk/src/base/command_line.cc?r1=76339...;
> back when command_line_string was still a simple getter. I think it's ok now
for
> OS_WIN; not sure for POSIX, so I'll leave the warning for POSIX, but we use
this
> in production on Windows... so it better work...!

I believe the comment is still valid for POSIX systems, or at least no one who
knows about POSIX command line quoting has come along to invalidate it :)

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

http://codereview.chromium.org/11305010/diff/1/base/command_line_unittest.cc#...
base/command_line_unittest.cc:192: cl.AppendSwitchPath(kSecondArgName,
FilePath(kPath2));
should have something in this test that exercises AppendArg() or one of its
variants as well.

[gab, 2012-10-29 15:50:44]

Thanks,

@willchan for OWNER approval.

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

http://codereview.chromium.org/11305010/diff/1/base/command_line_unittest.cc#...
base/command_line_unittest.cc:192: cl.AppendSwitchPath(kSecondArgName,
FilePath(kPath2));
On 2012/10/29 02:04:43, robertshield wrote:
> should have something in this test that exercises AppendArg() or one of its
> variants as well.

Done.

[willchan no longer on Chromium, 2012-10-29 17:31:24]

Tests seem to fail.

[gab, 2012-10-29 17:40:36]

On 2012/10/29 17:31:24, willchan wrote:
> Tests seem to fail.

They did, but that and has already been fixed in patch set 3, PTAL.

Thanks,
Gab

[willchan no longer on Chromium, 2012-10-29 17:48:06]

LGTM if tests pass

[commit-bot: I haz the power, 2012-10-29 18:07:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/11305010/15001

[commit-bot: I haz the power, 2012-10-29 21:31:33]

Change committed as 164740