Do some cleanup of file path name handling.

Do some cleanup of file path name handling.

This started trying to cleanup DownloadManager::GenerateFilename which asserts
if your system locale isn't UTF-8 (I ran into this when mine got messed up).
The solution is to have GetSuggestedFilename return a FilePath rather than
calling FromWStringHack.

The rest of the patch is a result of trying to write GetSuggestedFilename in a
reasonable way. I changed ReplaceIllegalCharacters to work on a
FilePath::StringType.

Some places in the code calling these functions got cleaner, some got messier.
I think overall the ones that got messier are the ones doing sketchy things
with paths and the ones that got cleaner are the ones doing things more
properly.

The only code here that gets called a nontrivial number of times is the
weburlloader, and I think the new code does about the same number of string
conversions overall (though on certain platforms the number will be higher or
lower).

BUG=none
TEST=none

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

[Evan Martin, 2009-10-12 19:02:49]

http://codereview.chromium.org/271056/diff/1/11
File base/i18n/file_util_icu.cc (left):

http://codereview.chromium.org/271056/diff/1/11#oldcode170
Line 170: #endif
Beautiful.

http://codereview.chromium.org/271056/diff/1/15
File chrome/browser/shell_integration_linux.cc (right):

http://codereview.chromium.org/271056/diff/1/15#newcode301
Line 301: WideToUTF8(chrome::kBrowserProcessExecutableName) + "-" + url.spec();
Should this be WideToASCII?  (There aren't any Unicode chars in the executable
name anyway, but that's more correct.)

http://codereview.chromium.org/271056/diff/1/14
File chrome/browser/views/new_profile_dialog.cc (right):

http://codereview.chromium.org/271056/diff/1/14#newcode68
Line 68: TrimWhitespace(profile_name, TRIM_ALL, &profile_name);
This new code no longer blocks illegal chars.  Is that ok?  I guess this whole
class is getting killed.

http://codereview.chromium.org/271056/diff/1/9
File net/base/net_util.cc (right):

http://codereview.chromium.org/271056/diff/1/9#newcode995
Line 995: filename =
UTF16ToUTF8(FilePath(UTF8ToUTF16(filename)).BaseName().value());
Yikes.

http://codereview.chromium.org/271056/diff/1/9#newcode1017
Line 1017: if (!default_name || !default_name[0]) {
You have inverted this test.  The previous test was true when it was !empty, now
you true when it is NULL.

(I think this doesn't matter since the input is always non-NULL, but it would be
good to make this correct.)

http://codereview.chromium.org/271056/diff/1/7
File printing/printed_document.cc (right):

http://codereview.chromium.org/271056/diff/1/7#newcode388
Line 388: std::string narrow_filename = WideToUTF16(filename);
This shouldn't compile.  (?)

http://codereview.chromium.org/271056/diff/1/6
File webkit/glue/plugins/webplugin_delegate_impl.cc (right):

http://codereview.chromium.org/271056/diff/1/6#newcode23
Line 23: #include "webkit/glue/plugins/webplugin_delegate_pepper_impl.h"
is this just extra leakage?

http://codereview.chromium.org/271056/diff/1/5
File webkit/glue/webplugin_delegate.h (right):

http://codereview.chromium.org/271056/diff/1/5#newcode9
Line 9: #include <vector>
leakage?

http://codereview.chromium.org/271056/diff/1/3
File webkit/webkit.gyp (right):

http://codereview.chromium.org/271056/diff/1/3#newcode534
Line 534: 'glue/plugins/webplugin_delegate_pepper_impl.h',
leakage?

[brettw, 2009-10-12 19:22:48]

http://codereview.chromium.org/271056/diff/1/11
File base/i18n/file_util_icu.cc (right):

http://codereview.chromium.org/271056/diff/1/11#newcode142
Line 142: U8_NEXT(file_name->data(), cursor, file_name->length(), code_point);
Actually, I have a question about this. Posix file paths aren't UTF-8, right? So
this would actually corrupt non-UTF-8 filenames. However, this code is
effectively what the old one was doing with all the conversions. What's the
right thing to do here?

[Evan Martin, 2009-10-12 19:36:00]

http://codereview.chromium.org/271056/diff/1/11
File base/i18n/file_util_icu.cc (right):

http://codereview.chromium.org/271056/diff/1/11#newcode142
Line 142: U8_NEXT(file_name->data(), cursor, file_name->length(), code_point);
On 2009/10/12 19:22:48, brettw wrote:
> Actually, I have a question about this. Posix file paths aren't UTF-8, right?
So
> this would actually corrupt non-UTF-8 filenames. However, this code is
> effectively what the old one was doing with all the conversions. What's the
> right thing to do here?

That's true.  In theory you can't know anything about the characters, but we
assume throughout that you're using a superset of ASCII and sometimes use
ASCII-based functions in these sorts of places.  Another option is that if
you're working user-visible strings that you work with string16s throughout and
then convert to a path at the end.  Depends on who uses the function.

[brettw, 2009-10-14 16:46:36]

Please take another look. GetSuggestedFilename has been modified to do no UTF
validation on Linux, and I changed around the unit tests to support this. There
is also a new change in tab_contents_view_win.cc where there was a merge
conflict.

http://codereview.chromium.org/271056/diff/1/9
File net/base/net_util.cc (right):

http://codereview.chromium.org/271056/diff/1/9#newcode995
Line 995: filename =
UTF16ToUTF8(FilePath(UTF8ToUTF16(filename)).BaseName().value());
On 2009/10/12 19:02:49, Evan Martin wrote:
> Yikes.

Indeed. I'd love it if we had a better story for some of this stuff.

[Evan Martin, 2009-10-14 17:21:59]

My substr observation makes me wonder whether this should be using string16
instead of FilePath.   :(

http://codereview.chromium.org/271056/diff/7003/3042
File chrome/browser/views/new_profile_dialog.cc (right):

http://codereview.chromium.org/271056/diff/7003/3042#newcode72
Line 72: profile_name = UTf8ToWide(profile_name_narrow);
typo here makes me think this won't compile

http://codereview.chromium.org/271056/diff/7003/3041
File chrome/browser/views/tab_contents/tab_contents_view_win.cc (right):

http://codereview.chromium.org/271056/diff/7003/3041#newcode145
Line 145: 0, MAX_PATH - drop_data.file_extension.size() - 2));
Is it possible for this to substr into the middle of a UTF-8 multibyte bit?

[brettw, 2009-10-15 15:44:02]

New snap up

http://codereview.chromium.org/271056/diff/7003/3041
File chrome/browser/views/tab_contents/tab_contents_view_win.cc (right):

http://codereview.chromium.org/271056/diff/7003/3041#newcode145
Line 145: 0, MAX_PATH - drop_data.file_extension.size() - 2));
This is a Windows-only file, so it will be 16-bit characters. Windows file
handling doesn't even know about UTF-16 surrogates, so I think this is probably
OK.

[brettw, 2009-10-19 17:10:54]

New new snap up with the bug exposed by the mac unittests fixed.

[Evan Martin, 2009-10-19 17:13:05]

OK

(hard to review diffs across 12 iterations of a patch)

[Evan Martin, 2009-10-19 17:13:20]

On 2009/10/19 17:13:05, Evan Martin wrote:
> (hard to review diffs across 12 iterations of a patch)

^^^ by that i mean, i feel sorry for you and impressed you persevered