Enable localization of default downloaded filename....

Enable localization of default downloaded filename.

Instead of localizing "download" string in net_util.cc, make a caller, download_manger, provide a localized string.

BUG=25289
TEST=NetUtilTest.GetSuggestedFilename,DownloadManagerTest.TestDownloadFilename

[tony, 2009-10-28 17:08:31]

http://codereview.chromium.org/343014/diff/2001/3002
File net/base/net_util.cc (right):

http://codereview.chromium.org/343014/diff/2001/3002#newcode1059
Line 1059: return FilePath(UTF8ToFilePathString(
UTF8ToFilePathString is not correct on linux.  It assumes that the filesystem is
using a utf8 encoding, which it might not be.

I think this function should instead take a FilePath for |default_name|.  You
would use l10n_util::GetString() and on linux/mac have an extra step to call
SysWideToNativeMB before passing the string to the FilePath constructor.

[hayato-do-not-use-this-account, 2009-10-30 03:20:50]

Thank you for the review.

http://codereview.chromium.org/343014/diff/2001/3002
File net/base/net_util.cc (right):

http://codereview.chromium.org/343014/diff/2001/3002#newcode1059
Line 1059: return FilePath(UTF8ToFilePathString(
Thank you for the review.

I changed this function so that it receives FilaPath as a |default_name|. I
updated other files that depend on this function.  Could you review it?

On 2009/10/28 17:08:31, tony wrote:
> UTF8ToFilePathString is not correct on linux.  It assumes that the filesystem
is
> using a utf8 encoding, which it might not be.
> 
> I think this function should instead take a FilePath for |default_name|.  You
> would use l10n_util::GetString() and on linux/mac have an extra step to call
> SysWideToNativeMB before passing the string to the FilePath constructor.
>

[tony, 2009-10-30 17:19:31]

This LGTM.  Just a small suggestion that may avoid some #ifs.

http://codereview.chromium.org/343014/diff/15/1005
File net/base/net_util.cc (right):

http://codereview.chromium.org/343014/diff/15/1005#newcode1062
Line 1062: std::string filename = GetFileNameFromCD(content_disposition,
It might be easier to convert filename to a FilePath here to avoid the #if
OS_WIN's below.

[darin (slow to review), 2009-10-30 18:02:35]

http://codereview.chromium.org/343014/diff/15/1005
File net/base/net_util.cc (right):

http://codereview.chromium.org/343014/diff/15/1005#newcode1053
Line 1053: static const FilePath
kFinalFallbackName(FILE_PATH_LITERAL("download"));
please avoid static non-POD types.

this should be like this instead:

static const FilePath::CharType kFinalFallbackName[] =
FILE_PATH_LITERAL("download");

http://codereview.chromium.org/343014/diff/15/1005#newcode1117
Line 1117: std::string& file_path_string = filename;
nit: change this to be "const std::string&"

http://codereview.chromium.org/343014/diff/15/1010
File webkit/glue/weburlloader_impl.cc (left):

http://codereview.chromium.org/343014/diff/15/1010#oldcode174
Line 174: response->setSuggestedFileName(UTF8ToUTF16(
i think you should use webkit_glue::FilePathToWebString, and then it would be
unnecessary to #ifdef.  moveover, it would more correctly implement this
conversion since FilePath::value() does not necessarily return UTF8.

[hayato-do-not-use-this-account, 2009-11-02 13:30:56]

Thank you for the review.

I updated it.
I also changed some files, which now use '#elif defined(OS_POSIX)' instead of
just '#else' according to cording guide:
http://dev.chromium.org/developers/coding-style

http://codereview.chromium.org/343014/diff/15/1005
File net/base/net_util.cc (right):

http://codereview.chromium.org/343014/diff/15/1005#newcode1053
Line 1053: static const FilePath
kFinalFallbackName(FILE_PATH_LITERAL("download"));
On 2009/10/30 18:02:35, darin wrote:
> please avoid static non-POD types.
> 
> this should be like this instead:
> 
> static const FilePath::CharType kFinalFallbackName[] =
> FILE_PATH_LITERAL("download");
> 

Done.

http://codereview.chromium.org/343014/diff/15/1005#newcode1062
Line 1062: std::string filename = GetFileNameFromCD(content_disposition,
Thank you.  Instead of converting std::string to FilaPath here, I converted it
to FilePath::StringType here. Converting to StringType here might be better.

On 2009/10/30 17:19:31, tony wrote:
> It might be easier to convert filename to a FilePath here to avoid the #if
> OS_WIN's below.

http://codereview.chromium.org/343014/diff/15/1005#newcode1117
Line 1117: std::string& file_path_string = filename;
It can not be const because it is modified in the following
ReplaceIllegalCharactersInPath(&file_path_string, '-').

Anyway, I removed '&'. It seems we should use a reference only when it is const.

On 2009/10/30 18:02:35, darin wrote:
> nit: change this to be "const std::string&"

http://codereview.chromium.org/343014/diff/15/1010
File webkit/glue/weburlloader_impl.cc (left):

http://codereview.chromium.org/343014/diff/15/1010#oldcode174
Line 174: response->setSuggestedFileName(UTF8ToUTF16(
On 2009/10/30 18:02:35, darin wrote:
> i think you should use webkit_glue::FilePathToWebString, and then it would be
> unnecessary to #ifdef.  moveover, it would more correctly implement this
> conversion since FilePath::value() does not necessarily return UTF8.

Done. Thank you!

[tony, 2009-11-02 17:46:12]

LGTM!

[darin (slow to review), 2009-11-02 17:48:04]

LGTM

[hayato-do-not-use-this-account, 2009-11-04 03:14:10]

HI tony, darin,

Thank you for LGTM.
Could you commit the change instead of me?
I don't have a commit access.

[tony, 2009-11-04 18:35:07]

I will land now . . .

[tony, 2009-11-04 18:51:37]

r30971