Fix the local file listing and FTP file listing. For the former, use the OS f...

Fix the local directory listing, FTP directory listing and the local file handling (drag'n'drop and opening from the file list).

For the local file listing, use the OS file system encoding. 

For the FTP directory listing, use ICU's encoding detector.GetDirectoryListingEntry and GetDirectoryLisingHeader were changed to accept string16 for file/directory names. To the former, a new parameter (|raw_bytes|) was added. It can be used to make a FTP request to a file with a non-ASCII name encoded in a legacy encoding.

For the local file handling on Windows, get rid of the code for 'doubly converted' UTF-8 in FileURLToFilePath, which led to issue 4619 and add a few cases to NetUtil*.FileURLConversion* test.

In addition, add
CodepageToUTF16 and UTF16ToCodepage along with a new unittest (ConvertBetweenCodepageAndUTF16) that shares the same set of case as ConvertBetweenCodepageAndWide. The test cases were expanded and revised a bit. 


BUG=2939, 13229, 4619
http://crbug.com/2939  http://crbug.com/13229 http://crbug.com/4619

TEST=1. Pass URLRequest*.FTP* (net_unittests)
     2. Pass StringUtiltTest.ConvertBetweenCode*
     3. Pass NetUtil*.GetDirectoryLis* (net_unittests)     
     4. Open a local directory containing files with non-ASCII names and they're displayed correctly in the directory list. On Windows and Mac OS X, it should always work. On Linux, your locale encoding (as returned by nl_langinfo(CODESET)) should match the actual encoding used in your filename. 
     5a. Pass NetUtil*.FileURL* (net_unittests) with the default codepage set to 1252 and 932. 
     5b. Make a file named 'café.txt' on Windows and see if it can be opened both by clicking in the directory listing page of Chrome and by drag'n'drop. Test this with the default OS code pages set to Windows-1252, Windows-1251 (Russian) and Windows-932 (Japanese). 

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

[jungshik at Google, 2009-07-02 15:02:02]

Asking for review before going further. 

Currently, I haven't been able to test with --new-ftp because even without this
patch, connecting to a ftp site crashes chrome (on linux and win).  Aside from
ftp, this fixes the local file listing and opening problems.

[brettw, 2009-07-02 21:37:22]

http://codereview.chromium.org/151065/diff/1087/128
File base/string_util.h (right):

http://codereview.chromium.org/151065/diff/1087/128#newcode186
Line 186: #define LITERAL_UTF16(s) ASCIIToUTF16(s)
I think somebody tried to add a macro like this before and we decided against
it. There are several problems.

The two versions of the macro result in different types. The first one generates
a wchar_t*, the second one a string16. So the only time you can use this macro
is for a function taking a string16, and ASCIIToUTF16 should be almost as fast
as copying the literal wchar_t into a new buffer.

Last, I think this encourages people to write global string16s which we want to
avoid.

http://codereview.chromium.org/151065/diff/1087/128#newcode243
Line 243: //  this is the same as SKIP.
Extra space at the beginning.

http://codereview.chromium.org/151065/diff/1087/128#newcode270
Line 270: return WideToCodepage(UTF16ToWideHack(utf16),
I think we should implement this function properly. The current WideToCodepage
implementation converts to UTF-16. So On Linux, your version converts
  UTF16 -> UCS32 -> UTF16 -> codepage

Seems like you should move the code (minus the conversions) from the existing
WideToCodepage into your function, and keep the conversion stuff in the existing
wide version that calls yours.

Same comment below.

http://codereview.chromium.org/151065/diff/1087/127
File net/base/net_util.h (right):

http://codereview.chromium.org/151065/diff/1087/127#newcode154
Line 154: // raw_bytes is used to generate the path part a url in the html.
Can you add to the beginning of this function what the function actually does?

http://codereview.chromium.org/151065/diff/1087/127#newcode159
Line 159: const base::Time& modified);
You can go ahead and pass Time by value, since it's just a number. You can keep
it this way if you want, though.

http://codereview.chromium.org/151065/diff/1087/132
File net/base/net_util_win.cc (right):

http://codereview.chromium.org/151065/diff/1087/132#newcode63
Line 63: // If the check passes, we converted the result to UTF-8.
passes -> passed
treat -> treated

http://codereview.chromium.org/151065/diff/1087/123
File net/url_request/url_request_file_dir_job.cc (right):

http://codereview.chromium.org/151065/diff/1087/123#newcode9
Line 9: #include "base/sys_string_conversions.h"
This should go below to be in order.

http://codereview.chromium.org/151065/diff/1087/125
File net/url_request/url_request_new_ftp_job.cc (right):

http://codereview.chromium.org/151065/diff/1087/125#newcode22
Line 22: namespace {
Put a blank line after namespace boundaries.

http://codereview.chromium.org/151065/diff/1087/125#newcode44
Line 44: const std::string encoding) {
This should be a reference.

http://codereview.chromium.org/151065/diff/1087/125#newcode48
Line 48: // Try the detected encoding before falling back to the native
codepage.
This indenting is messed up.

http://codereview.chromium.org/151065/diff/1087/125#newcode193
Line 193: encoding_ = DetectEncoding(buf->data(), bytes_read);
Can you put a blank line after this, and also before your comment a few lines
up? The block of code is getting out of control.

http://codereview.chromium.org/151065/diff/1087/122
File net/url_request/url_request_new_ftp_job.h (right):

http://codereview.chromium.org/151065/diff/1087/122#newcode63
Line 63: 
Delete this extra line.

[jungshik at Google, 2009-07-06 18:09:45]

Thank you for the review. I've addressed all your concerns except for
CodepageToUTF16/UTF16ToCodepage that I'm working on now. I'll upload a new patch
once they're properly implemented.

http://codereview.chromium.org/151065/diff/1087/128
File base/string_util.h (right):

http://codereview.chromium.org/151065/diff/1087/128#newcode186
Line 186: #define LITERAL_UTF16(s) ASCIIToUTF16(s)
On 2009/07/02 21:37:22, brettw wrote:
> I think somebody tried to add a macro like this before and we decided against
> it. There are several problems.
> 
> The two versions of the macro result in different types. The first one
generates
> a wchar_t*, the second one a string16. So the only time you can use this macro
> is for a function taking a string16, and ASCIIToUTF16 should be almost as fast
> as copying the literal wchar_t into a new buffer.
> 
> Last, I think this encourages people to write global string16s which we want
to
> avoid.

I agree. It's not very useful either. Removed it. I wish we could use  UTF-16/32
literals (u"foo" and U"foo". It's added to the C++ 0x standard and implemented
recently in gcc. 
http://blogs.oracle.com/ezannoni/2008/04/utf16_and_utf32_support_in_gcc.html ).

http://codereview.chromium.org/151065/diff/1087/128#newcode243
Line 243: //  this is the same as SKIP.
On 2009/07/02 21:37:22, brettw wrote:
> Extra space at the beginning.

Done.

http://codereview.chromium.org/151065/diff/1087/128#newcode270
Line 270: return WideToCodepage(UTF16ToWideHack(utf16),
On 2009/07/02 21:37:22, brettw wrote:
> I think we should implement this function properly. The current WideToCodepage
> implementation converts to UTF-16. So On Linux, your version converts
>   UTF16 -> UCS32 -> UTF16 -> codepage
> 
> Seems like you should move the code (minus the conversions) from the existing
> WideToCodepage into your function, and keep the conversion stuff in the
existing
> wide version that calls yours.
> 
> Same comment below.

I have an old patch that does something similar that I never uploaded. I'll
resurrect that.

http://codereview.chromium.org/151065/diff/1087/127
File net/base/net_util.h (right):

http://codereview.chromium.org/151065/diff/1087/127#newcode154
Line 154: // raw_bytes is used to generate the path part a url in the html.
On 2009/07/02 21:37:22, brettw wrote:
> Can you add to the beginning of this function what the function actually does?

Done.

http://codereview.chromium.org/151065/diff/1087/127#newcode159
Line 159: const base::Time& modified);
On 2009/07/02 21:37:22, brettw wrote:
> You can go ahead and pass Time by value, since it's just a number. You can
keep
> it this way if you want, though.

partially done in patch set 12. I forgot to update the cc file so that trybots
all failed to link.

http://codereview.chromium.org/151065/diff/1087/132
File net/base/net_util_win.cc (right):

http://codereview.chromium.org/151065/diff/1087/132#newcode63
Line 63: // If the check passes, we converted the result to UTF-8.
On 2009/07/02 21:37:22, brettw wrote:
> passes -> passed
> treat -> treated

Done.

http://codereview.chromium.org/151065/diff/1087/123
File net/url_request/url_request_file_dir_job.cc (right):

http://codereview.chromium.org/151065/diff/1087/123#newcode9
Line 9: #include "base/sys_string_conversions.h"
On 2009/07/02 21:37:22, brettw wrote:
> This should go below to be in order.

Done.

http://codereview.chromium.org/151065/diff/1087/125
File net/url_request/url_request_new_ftp_job.cc (right):

http://codereview.chromium.org/151065/diff/1087/125#newcode22
Line 22: namespace {
On 2009/07/02 21:37:22, brettw wrote:
> Put a blank line after namespace boundaries.

Done.

http://codereview.chromium.org/151065/diff/1087/125#newcode44
Line 44: const std::string encoding) {
On 2009/07/02 21:37:22, brettw wrote:
> This should be a reference.

Done.

http://codereview.chromium.org/151065/diff/1087/125#newcode48
Line 48: // Try the detected encoding before falling back to the native
codepage.
On 2009/07/02 21:37:22, brettw wrote:
> This indenting is messed up.

Done.

http://codereview.chromium.org/151065/diff/1087/125#newcode193
Line 193: encoding_ = DetectEncoding(buf->data(), bytes_read);
On 2009/07/02 21:37:22, brettw wrote:
> Can you put a blank line after this, and also before your comment a few lines
> up? The block of code is getting out of control.

Done.

http://codereview.chromium.org/151065/diff/1087/122
File net/url_request/url_request_new_ftp_job.h (right):

http://codereview.chromium.org/151065/diff/1087/122#newcode63
Line 63: 
On 2009/07/02 21:37:22, brettw wrote:
> Delete this extra line.

Done.

[jungshik at Google, 2009-07-07 06:36:42]

Brett, can you take another look? Thanks

[jungshik at Google, 2009-07-07 19:26:31]

On 2009/07/07 06:36:42, Jungshik Shin wrote:
> Brett, can you take another look? Thanks

Patch set 17 and set 18 are identical except for a few cases added to
FileURLConversion test in net_util_unittest.cc

[brettw, 2009-07-07 19:59:48]

LGTM

http://codereview.chromium.org/151065/diff/2113/3080
File base/string_util.h (right):

http://codereview.chromium.org/151065/diff/2113/3080#newcode263
Line 263: inline bool WideToCodepage(const std::wstring& wide,
I think I would prefer just to have these defined in the header once with no
impl, and move the ifdefs/forwarding to UTF16ToCodepage to the
string_util_icu.cc file.

http://codereview.chromium.org/151065/diff/2113/3082
File base/string_util_icu.cc (right):

http://codereview.chromium.org/151065/diff/2113/3082#newcode273
Line 273: encoded_max_length, uchar_src, uchar_len, &status);
This should be indented 2 more spaces.

http://codereview.chromium.org/151065/diff/2113/3081
File base/string_util_unittest.cc (right):

http://codereview.chromium.org/151065/diff/2113/3081#newcode17
Line 17: string16 BuildString16(const wchar_t* s) {
Can you add a quick comment above this?

[jungshik at Google, 2009-07-07 23:51:09]

Brett,
Thank you for the review. I've addressed your concerns locally (not uploaded
a new patch).
To Darin:

Do you have any comment on net/ changes? If not, I'll land it when the tree
is opened.

Jungshik


2009/7/7 <brettw@chromium.org>

> LGTM
>
>
> http://codereview.chromium.org/151065/diff/2113/3080
> File base/string_util.h (right):
>
> http://codereview.chromium.org/151065/diff/2113/3080#newcode263
> Line 263: inline bool WideToCodepage(const std::wstring& wide,
> I think I would prefer just to have these defined in the header once
> with no impl, and move the ifdefs/forwarding to UTF16ToCodepage to the
> string_util_icu.cc file.
>
> http://codereview.chromium.org/151065/diff/2113/3082
> File base/string_util_icu.cc (right):
>
> http://codereview.chromium.org/151065/diff/2113/3082#newcode273
> Line 273: encoded_max_length, uchar_src, uchar_len, &status);
> This should be indented 2 more spaces.
>
> http://codereview.chromium.org/151065/diff/2113/3081
> File base/string_util_unittest.cc (right):
>
> http://codereview.chromium.org/151065/diff/2113/3081#newcode17
> Line 17: string16 BuildString16(const wchar_t* s) {
> Can you add a quick comment above this?
>
>
> http://codereview.chromium.org/151065
>

[wtc, 2009-07-15 23:53:23]

http://codereview.chromium.org/151065/diff/2113/3077
File net/url_request/url_request_new_ftp_job.cc (right):

http://codereview.chromium.org/151065/diff/2113/3077#newcode29
Line 29: std::string DetectEncoding(const char*input, size_t len) {
Nit: add a space before 'input'.