Add AppendASCII

Adds AppendASCII which will append an ASCII path component.
Since this is safe to do on all platform path encodings (even Linux), this allows path components to be taken from ASCII sources without #ifdefs for the caller.


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

[Mark Mentovai, 2009-01-16 01:38:46]

http://codereview.chromium.org/18134/diff/6/208
File base/file_path.h (right):

http://codereview.chromium.org/18134/diff/6/208#newcode193
Line 193: // be able to handle UTF-8 strings transparently as well.
You should mention that this is only the case not just because StringType is
wstring but because its encoding is specified as UTF-16.

The problem I have with this is that the API tempts developers (who only develop
and test on Windows) to mix UTF-8 strings with FilePaths.  Given std::string
my_utf8_string which can come from anywhere and contain the entire gamut of
UTF-8 data, someone's going to write file_path.Append(my_utf8_string) and have
it work on Windows, and in this case, even on the Mac and some Linux systems. 
But it's wrong on a Linux system whose filesystem encoding is, say, KOI8-*. 
Because you're writing this as an overload to Append, it invites bugs of this
variety.

On the other hand, it's probably more reasonable to assume that ASCII is a
subset of any filesystem encoding.  So I think that perhaps the right approach
here might be to add an AppendASCII method to FilePath, which would be
implemented the same as Append on Mac and Linux (possibly with a DCHECK to
ensure that ASCII input is being passed in), and would be implemented in terms
of an ASCII-to-UTF16 conversion on Windows (again, not supporting conversion of
characters outside of the ASCII range).  We also avoid the confusing overload by
making it a distinct method.  Before we do this, I think we should open it up to
discussion and at least try to solicit input from Jungshik, who's the expert in
this area.

Do you need to have an Append that handles arbitrary UTF-8 input on Windows, or
would ASCII be enough?

[Erik does not do reviews, 2009-01-20 23:04:47]

On 2009/01/16 01:38:46, Mark Mentovai wrote:
> http://codereview.chromium.org/18134/diff/6/208
> File base/file_path.h (right):
> 
> http://codereview.chromium.org/18134/diff/6/208#newcode193
> Line 193: // be able to handle UTF-8 strings transparently as well.
> You should mention that this is only the case not just because StringType is
> wstring but because its encoding is specified as UTF-16.
> 
> The problem I have with this is that the API tempts developers (who only
develop
> and test on Windows) to mix UTF-8 strings with FilePaths.  Given std::string
> my_utf8_string which can come from anywhere and contain the entire gamut of
> UTF-8 data, someone's going to write file_path.Append(my_utf8_string) and have
> it work on Windows, and in this case, even on the Mac and some Linux systems. 
> But it's wrong on a Linux system whose filesystem encoding is, say, KOI8-*. 
> Because you're writing this as an overload to Append, it invites bugs of this
> variety.
> 
> On the other hand, it's probably more reasonable to assume that ASCII is a
> subset of any filesystem encoding.  So I think that perhaps the right approach
> here might be to add an AppendASCII method to FilePath, which would be
> implemented the same as Append on Mac and Linux (possibly with a DCHECK to
> ensure that ASCII input is being passed in), and would be implemented in terms
> of an ASCII-to-UTF16 conversion on Windows (again, not supporting conversion
of
> characters outside of the ASCII range).  We also avoid the confusing overload
by
> making it a distinct method.  Before we do this, I think we should open it up
to
> discussion and at least try to solicit input from Jungshik, who's the expert
in
> this area.
> 
> Do you need to have an Append that handles arbitrary UTF-8 input on Windows,
or
> would ASCII be enough?

In my case, I have a JSON manifest file defining an extension.  We'd like to
store the extension in a human-readable directory name defined by the extension
author.  The JSON is UTF-8.  However, your arguments have convinced me that
ASCII is really the only reasonable thing here, so I'll enforce ASCII values in
our JSON file.  I've uploaded a new snapshot.

[darin (slow to review), 2009-01-20 23:09:39]

http://codereview.chromium.org/18134/diff/210/10
File base/file_path.cc (right):

http://codereview.chromium.org/18134/diff/210/10#newcode256
Line 256: if (!IsStringASCII(component))
maybe this should just be a {D}CHECK?  it doesn't seem that helpful to silently
return junk.  if that happens in the field, we'll get no report of it.  it is
unlikely that a developer here will test on a system involving an incompatible
filesystem encoding so a CHECK seems warranted to me.

[Erik does not do reviews, 2009-01-20 23:13:24]

http://codereview.chromium.org/18134/diff/210/10
File base/file_path.cc (right):

http://codereview.chromium.org/18134/diff/210/10#newcode256
Line 256: if (!IsStringASCII(component))
On 2009/01/20 23:09:39, darin wrote:
> maybe this should just be a {D}CHECK?  it doesn't seem that helpful to
silently
> return junk.  if that happens in the field, we'll get no report of it.  it is
> unlikely that a developer here will test on a system involving an incompatible
> filesystem encoding so a CHECK seems warranted to me.

In my case, the path component I'm appending is coming from a user-created JSON
file, so a DCHECK won't help.  The fact that it returns an empty path in this
case is something that the caller can check and error out based on.

Incidentally, this pattern is followed in a couple of other FilePath methods
(search for return FilePath()).

[Mark Mentovai, 2009-01-20 23:16:12]

LGTM.  I agree with Darin about the CHECK.

http://codereview.chromium.org/18134/diff/210/10
File base/file_path.cc (right):

http://codereview.chromium.org/18134/diff/210/10#newcode261
Line 261: #else if defined(OS_POSIX)
#elif?

http://codereview.chromium.org/18134/diff/210/11
File base/file_path.h (right):

http://codereview.chromium.org/18134/diff/210/11#newcode191
Line 191: // Although Windows StringType is std::wstring, it's reasonable for it
to
Blank line before, this is a new method...

http://codereview.chromium.org/18134/diff/210/11#newcode192
Line 192: // be able to handle ASCII strings transparently as well.  On Linux,
it should
say "because the encoding used for its paths is defined" or something.

http://codereview.chromium.org/18134/diff/210/11#newcode194
Line 194: // being used.
Expand this idea a bit more so that the problem and our approach is obvious to a
reader.  We need to convey something like "ASCII can reasonably be assumed to be
a subset of any 8-bit encoding used for paths."

http://codereview.chromium.org/18134/diff/210/9
File base/file_path_unittest.cc (right):

http://codereview.chromium.org/18134/diff/210/9#newcode301
Line 301: #else if defined(OS_POSIX)
#elif again?

[Erik does not do reviews, 2009-01-20 23:32:37]

Regarding the (D)CHECK, are you agreeing with him before or after reading my
reply to him?

http://codereview.chromium.org/18134/diff/210/10
File base/file_path.cc (right):

http://codereview.chromium.org/18134/diff/210/10#newcode261
Line 261: #else if defined(OS_POSIX)
On 2009/01/20 23:16:12, Mark Mentovai wrote:
> #elif?

Already fixed (the try bot caught it).  irritating that msdev doesn't flag that
as bogus syntax.

[darin (slow to review), 2009-01-21 06:14:46]

If you are worried about that problem, then you could also just add an IsASCII
test before calling a method named AppendASCII?  We'll never test all of the
combinations of character encodings and system configurations to get this right,
and if someone doesn't check the result for an empty FilePath, then they may end
up with nasty bugs (e.g., writing a file into the wrong part of the filesystem,
potentially causing security bugs, etc.).  Fail hard, phone home is good?

[Dean McNamee, 2009-01-21 14:21:16]

LG.  This change would be great to reduce our uses of Append(FILE_PATH_LITERAL).
 This would meaning cutting down on some wchar_t literals on Windows, when the
small cost to convert is probably worthwhile.

Make sure to update the CL description, it's gone stale.

http://codereview.chromium.org/18134/diff/26/28
File base/file_path.cc (right):

http://codereview.chromium.org/18134/diff/26/28#newcode257
Line 257: return FilePath();
ASCIIToWide already does a DCHECK(IsStringASCII).  Do we really want this check
to not be a crash?  It seems like it would be better to catch the error than
return an empty filepath.

http://codereview.chromium.org/18134/diff/26/28#newcode260
Line 260: return Append(wcomponent);
return Append(ASCIIToWide(component)); ?