CommandLine: eliminate wstring-accepting AppendLooseValue

CommandLine: eliminate wstring-accepting AppendLooseValue

Instead use AppendArg variants which accept a FilePath or an ASCII string.

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

[viettrungluu, 2010-08-11 19:53:56]

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

http://codereview.chromium.org/3134008/diff/1/3#newcode160
base/command_line.h:160: void AppendArg(const std::string& value);
What encoding does this std::string have? (In particular, what will it do on
Windows?)

Maybe these should parallel AppendSwitch...() and have
AppendArgPath()/AppendArgNative()/AppendArgASCII() variants?

[Evan Martin, 2010-08-11 19:55:06]

On 2010/08/11 19:53:56, viettrungluu wrote:
> base/command_line.h:160: void AppendArg(const std::string& value);
> What encoding does this std::string have? (In particular, what will it do on
> Windows?)

ASCII.  I realized that AppendArg(some native string type) won't work across
platforms, so it's hard to accidentally pass a non-ascii string on Linux.

[viettrungluu, 2010-08-11 19:58:14]

On 2010/08/11 19:55:06, Evan Martin wrote:
> On 2010/08/11 19:53:56, viettrungluu wrote:
> > base/command_line.h:160: void AppendArg(const std::string& value);
> > What encoding does this std::string have? (In particular, what will it do on
> > Windows?)
> 
> ASCII.  I realized that AppendArg(some native string type) won't work across
> platforms, so it's hard to accidentally pass a non-ascii string on Linux.

If it's restricted to ASCII, I think we should label as such (otherwise someone
might give it a UTF-8 string and expect it to do the right thing on Windows).

[Evan Martin, 2010-08-11 20:34:50]

On 2010/08/11 19:58:14, viettrungluu wrote:
> If it's restricted to ASCII, I think we should label as such (otherwise
someone
> might give it a UTF-8 string and expect it to do the right thing on Windows).

That would require explicitly converting a wstring to UTF-8, which is never used
on Windows.  Do you really think it's a problem?  I resist because if I do this,
then it's also weird that the other one doesn't have "Path" in its name, which
means the whole codebase gets uglified.  I think I'd prefer to remove the ASCII
variant in the other function -- we shouldn't have any functions in our codebase
that work in UTF-8.

[viettrungluu, 2010-08-12 02:35:08]

On 2010/08/11 20:34:50, Evan Martin wrote:
> That would require explicitly converting a wstring to UTF-8, which is never
used
> on Windows.  Do you really think it's a problem?

Probably not.

> I resist because if I do this,
> then it's also weird that the other one doesn't have "Path" in its name, which
> means the whole codebase gets uglified.  I think I'd prefer to remove the
ASCII
> variant in the other function -- we shouldn't have any functions in our
codebase
> that work in UTF-8.

There are a number of places where we use UTF-8 (or pass data via UTF-8). (This
seems to be especially the case with stuff coming from Javascript or similar.)

Mainly, I find the difference between AppendSwitch___() a bit jarring, and
prefer things to be consistent.

[Evan Martin, 2010-08-12 21:17:18]

Please take another look.

[Evan Martin, 2010-08-13 00:03:53]

Ping.

[viettrungluu, 2010-08-13 00:42:34]

I'm a little worried about passing URLs on the command-line (see below).

How bad would it be to assert that certain args must be UTF-8 (on POSIX)? (If
the user provides a non-ASCII, non-UTF-8 argument, then they'd be out of luck,
but that seems like it'd be an exceptional case.)

http://codereview.chromium.org/3134008/diff/3002/11006
File chrome/browser/chromeos/login/apply_services_customization.cc (right):

http://codereview.chromium.org/3134008/diff/3002/11006#newcode138
chrome/browser/chromeos/login/apply_services_customization.cc:138: // on browser
startup.
I'm trusting that our "partners" (whoever they may be) all have sensible ASCII
urls.

http://codereview.chromium.org/3134008/diff/3002/11007
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

http://codereview.chromium.org/3134008/diff/3002/11007#newcode337
chrome/browser/chromeos/login/existing_user_controller.cc:337:
CommandLine::ForCurrentProcess()->AppendArgASCII(start_url_.spec());
Apparently, what .spec() returns may be UTF-8. I'm a bit worried that this may
break some people. Then again, I don't know if this code path currently works
properly in that case.

Maybe we need to support Unicode args (converting to wide/UTF-8 depending on
platform)? Otherwise, we'd have to escape to ASCII.

http://codereview.chromium.org/3134008/diff/3002/11008
File chrome/browser/chromeos/login/login_screen.cc (right):

http://codereview.chromium.org/3134008/diff/3002/11008#newcode109
chrome/browser/chromeos/login/login_screen.cc:109:
CommandLine::ForCurrentProcess()->AppendArgASCII(start_url_.spec());
Ditto. :(

[Evan Martin, 2010-08-13 18:56:38]

On 2010/08/13 00:42:34, viettrungluu wrote:
> I'm a little worried about passing URLs on the command-line (see below).
> 
> How bad would it be to assert that certain args must be UTF-8 (on POSIX)? (If
> the user provides a non-ASCII, non-UTF-8 argument, then they'd be out of luck,
> but that seems like it'd be an exceptional case.)

It's bad in to the extent that I would prefer to not add yet more API. 
CommandLine is a morass as is, with each function being written in three
different variants.

I guess I could replace AppendArgASCII with AppendArgUTF8 but it means a lot of
search and replace.  At the end it maybe goes back to the question I had
earlier, which is whether a AppendArg(string) that expects UTF8 is acceptable.

> http://codereview.chromium.org/3134008/diff/3002/11006
> File chrome/browser/chromeos/login/apply_services_customization.cc (right):
> 
> http://codereview.chromium.org/3134008/diff/3002/11006#newcode138
> chrome/browser/chromeos/login/apply_services_customization.cc:138: // on
browser
> startup.
> I'm trusting that our "partners" (whoever they may be) all have sensible ASCII
> urls.
> 
> http://codereview.chromium.org/3134008/diff/3002/11007
> File chrome/browser/chromeos/login/existing_user_controller.cc (right):
> 
> http://codereview.chromium.org/3134008/diff/3002/11007#newcode337
> chrome/browser/chromeos/login/existing_user_controller.cc:337:
> CommandLine::ForCurrentProcess()->AppendArgASCII(start_url_.spec());
> Apparently, what .spec() returns may be UTF-8. I'm a bit worried that this may
> break some people. Then again, I don't know if this code path currently works
> properly in that case.
> 
> Maybe we need to support Unicode args (converting to wide/UTF-8 depending on
> platform)? Otherwise, we'd have to escape to ASCII.
> 
> http://codereview.chromium.org/3134008/diff/3002/11008
> File chrome/browser/chromeos/login/login_screen.cc (right):
> 
> http://codereview.chromium.org/3134008/diff/3002/11008#newcode109
> chrome/browser/chromeos/login/login_screen.cc:109:
> CommandLine::ForCurrentProcess()->AppendArgASCII(start_url_.spec());
> Ditto. :(

(For all of these) It seems .spec() is ASCII except for the reference fragment
part.  So this is only wrong when these urls contain a non-ASCII #foobar bit.

[viettrungluu, 2010-08-13 19:38:27]

On 2010/08/13 18:56:38, Evan Martin wrote:
> On 2010/08/13 00:42:34, viettrungluu wrote:
> > I'm a little worried about passing URLs on the command-line (see below).
> > 
> > How bad would it be to assert that certain args must be UTF-8 (on POSIX)?
(If
> > the user provides a non-ASCII, non-UTF-8 argument, then they'd be out of
luck,
> > but that seems like it'd be an exceptional case.)
> 
> It's bad in to the extent that I would prefer to not add yet more API. 
> CommandLine is a morass as is, with each function being written in three
> different variants.

For *specifying* command-line arguments, I think we should be able to accept
UTF-8 (which will also accept ASCII, obviously). (Internally, this should do
UTF8ToWide() on Windows.) There should be no need for any suffix for this case.
If anyone wants it (hopefully not), this could be overloaded to take string16.
There's no need for an ASCII variant.

For *reading* arguments, we should provide the ability to get a native string
(with a "Native" suffix), as well as UTF-8 (and maybe UTF-16) strings -- but the
latter should verify that the string is in fact Unicode. (E.g., we might have
|bool GetSwitchValueUTF8(std::string switch_name, std::string* out);| or
something like that. |switch_name| should be UTF-8, typically ASCII, and this
can be DCHECK()ed. This would return |true| if and only if the value was in fact
Unicode.)

Unfortunately, the CommandLine API is messy/bad for this....

> I guess I could replace AppendArgASCII with AppendArgUTF8 but it means a lot
of
> search and replace.  At the end it maybe goes back to the question I had
> earlier, which is whether a AppendArg(string) that expects UTF8 is acceptable.
> 
> > http://codereview.chromium.org/3134008/diff/3002/11006
> > File chrome/browser/chromeos/login/apply_services_customization.cc (right):
> > 
> > http://codereview.chromium.org/3134008/diff/3002/11006#newcode138
> > chrome/browser/chromeos/login/apply_services_customization.cc:138: // on
> browser
> > startup.
> > I'm trusting that our "partners" (whoever they may be) all have sensible
ASCII
> > urls.
> > 
> > http://codereview.chromium.org/3134008/diff/3002/11007
> > File chrome/browser/chromeos/login/existing_user_controller.cc (right):
> > 
> > http://codereview.chromium.org/3134008/diff/3002/11007#newcode337
> > chrome/browser/chromeos/login/existing_user_controller.cc:337:
> > CommandLine::ForCurrentProcess()->AppendArgASCII(start_url_.spec());
> > Apparently, what .spec() returns may be UTF-8. I'm a bit worried that this
may
> > break some people. Then again, I don't know if this code path currently
works
> > properly in that case.
> > 
> > Maybe we need to support Unicode args (converting to wide/UTF-8 depending on
> > platform)? Otherwise, we'd have to escape to ASCII.
> > 
> > http://codereview.chromium.org/3134008/diff/3002/11008
> > File chrome/browser/chromeos/login/login_screen.cc (right):
> > 
> > http://codereview.chromium.org/3134008/diff/3002/11008#newcode109
> > chrome/browser/chromeos/login/login_screen.cc:109:
> > CommandLine::ForCurrentProcess()->AppendArgASCII(start_url_.spec());
> > Ditto. :(
> 
> (For all of these) It seems .spec() is ASCII except for the reference fragment
> part.  So this is only wrong when these urls contain a non-ASCII #foobar bit.

Yeah. Maybe we should just let it accept UTF-8, do the conversion on Windows,
and remove the ASCII suffix?

Sorry about all the work (and back-and-forth), but thanks for doing it.

[Evan Martin, 2010-08-13 20:46:03]

ok, new patch up with UTF8 version

[viettrungluu, 2010-08-13 20:52:58]

LGTM with a couple of extra DCHECK()s. Thanks!

http://codereview.chromium.org/3134008/diff/17001/18001
File base/command_line.cc (right):

http://codereview.chromium.org/3134008/diff/17001/18001#newcode426
base/command_line.cc:426: AppendArgNative(UTF8ToWide(value));
Could you add a |DCHECK(IsStringUTF8(value));| here?

http://codereview.chromium.org/3134008/diff/17001/18001#newcode478
base/command_line.cc:478: AppendArgNative(value);
Ditto.