CommandLine: add a CopySwitchesFrom() for copying from another CommandLine

CommandLine: add a CopySwitchesFrom() and AppendSwitchPath()

These are two common patterns in Chrome code: copying a
subset of switches from one CommandLine to another, and
appending a FilePath to a CommandLine.  This sets me up
to do a lot more deprecation in a follow-up change.

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

[Evan Martin, 2010-07-29 00:56:15]

sorry to pick you, but i'm trying to balance who i annoy with this

[Evan Martin, 2010-07-29 18:39:22]

ping.  let me know if i should find someone else.

[viettrungluu, 2010-07-29 18:41:35]

Oops. I started looking at it, and then forgot about it. Sorry about 
that. I'll get to it now....

evan@chromium.org wrote:
> ping.  let me know if i should find someone else.
> 
> http://codereview.chromium.org/3012021/show
> 

[viettrungluu, 2010-07-29 19:01:03]

Mostly looks good -- my biggest concern is the name/overloading of
AppendSwitch().

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

http://codereview.chromium.org/3012021/diff/2001/3002#newcode163
base/command_line.h:163: void AppendSwitch(const std::string& switch_string,
const FilePath& path);
Maybe you should call it AppendSwitchWithFilePath() or something like that? I
think our style guide frowns on overloading unless the functionality is
identical.

http://codereview.chromium.org/3012021/diff/2001/3002#newcode166
base/command_line.h:166: // TODO(evanm): deprecated, remove these.
s/these/this? (I assume it's just the wstring version which is deprecated,
right?)

http://codereview.chromium.org/3012021/diff/2001/3008
File chrome/browser/first_run_win.cc (right):

http://codereview.chromium.org/3012021/diff/2001/3008#newcode698
chrome/browser/first_run_win.cc:698:
ASCIIToWide(g_browser_process->GetApplicationLocale()));
Can you get rid of this ASCIIToWide() here too?

http://codereview.chromium.org/3012021/diff/2001/3012
File chrome/browser/nacl_host/nacl_broker_host_win.cc (right):

http://codereview.chromium.org/3012021/diff/2001/3012#newcode51
chrome/browser/nacl_host/nacl_broker_host_win.cc:51: channel_id());
This now fits on one line.

http://codereview.chromium.org/3012021/diff/2001/3013
File chrome/browser/nacl_host/nacl_process_host.cc (right):

http://codereview.chromium.org/3012021/diff/2001/3013#newcode93
chrome/browser/nacl_host/nacl_process_host.cc:93: channel_id());
This too.

http://codereview.chromium.org/3012021/diff/2001/3015
File chrome/browser/plugin_process_host.cc (right):

http://codereview.chromium.org/3012021/diff/2001/3015#newcode413
chrome/browser/plugin_process_host.cc:413:
PluginService::GetInstance()->GetChromePluginDataDir();
One more level indent (unless it all fits on one line).

http://codereview.chromium.org/3012021/diff/2001/3017
File chrome/browser/printing/printing_layout_uitest.cc (left):

http://codereview.chromium.org/3012021/diff/2001/3017#oldcode34
chrome/browser/printing/printing_layout_uitest.cc:34: L'"' + emf_path_.value() +
L'"');
Are you positive the quotes aren't required? I have no idea....

http://codereview.chromium.org/3012021/diff/2001/3034
File chrome/test/ui/ppapi_uitest.cc (right):

http://codereview.chromium.org/3012021/diff/2001/3034#newcode42
chrome/test/ui/ppapi_uitest.cc:42: FilePath(pepper_plugin));
Hrm. This is probably too hacky, since |pepper_plugin| is not really a path.

[Evan Martin, 2010-07-29 19:12:02]

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

http://codereview.chromium.org/3012021/diff/2001/3002#newcode163
base/command_line.h:163: void AppendSwitch(const std::string& switch_string,
const FilePath& path);
On 2010/07/29 19:01:03, viettrungluu wrote:
> Maybe you should call it AppendSwitchWithFilePath() or something like that? I
> think our style guide frowns on overloading unless the functionality is
> identical.

Yes, I considered this too.  But I think the functionality is identical, don't
you agree?

http://codereview.chromium.org/3012021/diff/2001/3002#newcode166
base/command_line.h:166: // TODO(evanm): deprecated, remove these.
On 2010/07/29 19:01:03, viettrungluu wrote:
> s/these/this? (I assume it's just the wstring version which is deprecated,
> right?)

Both need to be, actually.  The wstring one needs to die, and the other one
needs "ASCII" in its name to emphasize you can't stuff random bytes into it.

http://codereview.chromium.org/3012021/diff/2001/3034
File chrome/test/ui/ppapi_uitest.cc (right):

http://codereview.chromium.org/3012021/diff/2001/3034#newcode42
chrome/test/ui/ppapi_uitest.cc:42: FilePath(pepper_plugin));
On 2010/07/29 19:01:03, viettrungluu wrote:
> Hrm. This is probably too hacky, since |pepper_plugin| is not really a path.

I agree it is ugly, but I'm not sure of a better option.  I guess I could add an
AppendNativeSwitch() function that only accepts the current OS's string type.

[viettrungluu, 2010-07-29 19:44:01]

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

http://codereview.chromium.org/3012021/diff/2001/3002#newcode163
base/command_line.h:163: void AppendSwitch(const std::string& switch_string,
const FilePath& path);
On 2010/07/29 19:12:02, Evan Martin wrote:
> On 2010/07/29 19:01:03, viettrungluu wrote:
> > Maybe you should call it AppendSwitchWithFilePath() or something like that?
I
> > think our style guide frowns on overloading unless the functionality is
> > identical.
> 
> Yes, I considered this too.  But I think the functionality is identical, don't
> you agree?

I don't know how conservatively to interpret "identical". The precedent (on the
other side) is in AppendSwitchWithValue(). Left to my own devices I think I'd be
with you, but I think Chromium code errs on the side of being conservative.

http://codereview.chromium.org/3012021/diff/2001/3002#newcode166
base/command_line.h:166: // TODO(evanm): deprecated, remove these.
On 2010/07/29 19:12:02, Evan Martin wrote:
> On 2010/07/29 19:01:03, viettrungluu wrote:
> > s/these/this? (I assume it's just the wstring version which is deprecated,
> > right?)
> 
> Both need to be, actually.  The wstring one needs to die, and the other one
> needs "ASCII" in its name to emphasize you can't stuff random bytes into it.

Do you feel like adding one with "ASCII" while you're at it? (Or you can do that
next -- your choice.)

http://codereview.chromium.org/3012021/diff/2001/3034
File chrome/test/ui/ppapi_uitest.cc (right):

http://codereview.chromium.org/3012021/diff/2001/3034#newcode42
chrome/test/ui/ppapi_uitest.cc:42: FilePath(pepper_plugin));
On 2010/07/29 19:12:02, Evan Martin wrote:
> On 2010/07/29 19:01:03, viettrungluu wrote:
> > Hrm. This is probably too hacky, since |pepper_plugin| is not really a path.
> 
> I agree it is ugly, but I'm not sure of a better option.  I guess I could add
an
> AppendNativeSwitch() function that only accepts the current OS's string type.

I think I'd be happier with that. Mostly what I'm worried about is that FilePath
doesn't guarantee that it properly preserves the stuff which is appended.

[Evan Martin, 2010-07-29 21:47:53]

ok, please look again.

I unified into the pattern of having APIs like
  GetFooPath()
  GetFooNative()
and
  SetFooPath()
  SetFooNative()

For the different types, as recommended by the style guide.
I will be able to grep for "WithValue()" to remove all of the remaining (wrong)
callers.

[viettrungluu, 2010-07-29 22:04:11]

Thanks!

LGTM.

On 2010/07/29 21:47:53, Evan Martin wrote:
> ok, please look again.
> 
> I unified into the pattern of having APIs like
>   GetFooPath()
>   GetFooNative()
> and
>   SetFooPath()
>   SetFooNative()
> 
> For the different types, as recommended by the style guide.
> I will be able to grep for "WithValue()" to remove all of the remaining
(wrong)
> callers.