Download filename encoding fix [try2]:

Download filename encoding fix [try2]:

Fall back on other charsets when using the URL to suggest a filename. The URL should be escaped UTF-8, but may use another encoding instead.

Check for empty string in DecodeWord (avoids crash).

BUG=44301
TEST=see bug. Also, net_unittests.

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

[Evan Stade, 2010-06-09 01:20:15]

(previous patch failed net_unittests)

[brettw, 2010-06-09 01:38:52]

LGTM

[jungshik at google, 2010-06-09 23:29:23]

Why don't you add a couple of tests (to that long array :-) in
net_util_unittest.cc)? Or, did you add it already separately?

http://codereview.chromium.org/2733005/diff/4001/5002
File net/base/net_util.cc (right):

http://codereview.chromium.org/2733005/diff/4001/5002#newcode1111
net/base/net_util.cc:1111: // The URL should be escaped UTF-8, but may not be.
nit: The path part of the URL (coming out of GURL) should be escaped UTF-8, but
may not be. 

TODO:

http://codereview.chromium.org/2733005/diff/4001/5002#newcode1116
net/base/net_util.cc:1116: &decoded_filename);
TODO: referrer_charaset wouldn't help if a non-UTF-8 path is referred to in a
UTF-8 page (e.g. Google search result page). A better heuristic is needed (e.g.
encoding detection).

[Evan Stade, 2010-06-10 00:03:25]

test added.

FWIW, the particular example of a google search results page is actually not a
problem --- I don't think the referrer charset is taken from the page the link
is embedded in, because with this patch, when I click on the link in the bug
report, it works as expected, even though code.google.com uses utf-8.

http://codereview.chromium.org/2733005/diff/4001/5002
File net/base/net_util.cc (right):

http://codereview.chromium.org/2733005/diff/4001/5002#newcode1111
net/base/net_util.cc:1111: // The URL should be escaped UTF-8, but may not be.
On 2010/06/09 23:29:23, Jungshik Shin wrote:
> nit: The path part of the URL (coming out of GURL) should be escaped UTF-8,
but
> may not be. 
> 
> TODO: 
> 
> 
> 

Done.

http://codereview.chromium.org/2733005/diff/4001/5002#newcode1116
net/base/net_util.cc:1116: &decoded_filename);
On 2010/06/09 23:29:23, Jungshik Shin wrote:
> TODO: referrer_charaset wouldn't help if a non-UTF-8 path is referred to in a
> UTF-8 page (e.g. Google search result page). A better heuristic is needed
(e.g.
> encoding detection). 

Done.

[jungshik at google, 2010-06-10 23:50:25]

LGTM 

On 2010/06/10 00:03:25, Evan Stade wrote:
> test added.
> 
> FWIW, the particular example of a google search results page is actually not a
> problem --- I don't think the referrer charset is taken from the page the link
> is embedded in, because with this patch, when I click on the link in the bug
> report, it works as expected, even though http://code.google.com uses utf-8.

That's because there's a bug (to resolve) about 'referrer charset' not being
passed donw to net :-).  And, the next fallback is to use the OS default
encoding (windows-1252 in your environment if you're on English Windows) which
happens to have the same code points for two non-ASCII characters in question as
ISO-8859-2/windows-1250 has (0xE1 and 0xFD for U+00E1 and U+00FD).

Even if |referer_charset| is passed down, the string in question is invalid as
UTF-8 (the conversion will fail) and the next fallback will be taken. So, the
end result would be the same. 


Anyway, your change is surely an improvement. 


> 
> http://codereview.chromium.org/2733005/diff/4001/5002
> File net/base/net_util.cc (right):
> 
> http://codereview.chromium.org/2733005/diff/4001/5002#newcode1111
> net/base/net_util.cc:1111: // The URL should be escaped UTF-8, but may not be.
> On 2010/06/09 23:29:23, Jungshik Shin wrote:
> > nit: The path part of the URL (coming out of GURL) should be escaped UTF-8,
> but
> > may not be. 
> > 
> > TODO: 
> > 
> > 
> > 
> 
> Done.
> 
> http://codereview.chromium.org/2733005/diff/4001/5002#newcode1116
> net/base/net_util.cc:1116: &decoded_filename);
> On 2010/06/09 23:29:23, Jungshik Shin wrote:
> > TODO: referrer_charaset wouldn't help if a non-UTF-8 path is referred to in
a
> > UTF-8 page (e.g. Google search result page). A better heuristic is needed
> (e.g.
> > encoding detection). 
> 
> Done.

[Evan Stade, 2010-06-10 23:54:24]

> That's because there's a bug (to resolve) about 'referrer charset' not being
> passed donw to net :-).  And, the next fallback is to use the OS default
> encoding (windows-1252 in your environment if you're on English Windows) which
> happens to have the same code points for two non-ASCII characters in question
as
> ISO-8859-2/windows-1250 has (0xE1 and 0xFD for U+00E1 and U+00FD).

I'm testing this on linux, and the referrer charset *is* being passed down, and
it is iso-8859-2.