download filename fix

This CL makes Chrome on par with Firefox in terms of 'GetSuggestedFilename' for file download via context-menu.

For a download initiated with a click on a link in a web page, a webkit-side change is necessary, which will be done later.

Add a field (referrer_charset) to URLRequestContext and DownloadCreateInfo. It's set to the character encoding of a document where the download request originates from when it's known (download initiated via "save as" in the context menu). 

If it's not known (a download initiated by clicking on a download link or typing a url directly to the omnibox), it's initialized to the default character encoding in the user's preference. I guess this is marginally better than leaving it empty (in that case, step 2b below will be skipped and step 2c will be taken) because a user has a better control over how raw 8bit characters in C-D are interpreted (especially on Windows where a reboot is required to change the OS default codepage). 

This is later passed to GetSuggestedFilename and used as one of fallback encodings (1. UTF-8, 2. origin_charset, 3. default OS codepage). With this change, we support the following:

1. RFC 2047
2. Raw-8bit-characters : a. UTF-8, b. origin_charset, c. default os codepage. 
3. %-escaped UTF-8. 

In this CL, for #3, I didn't add a fallback similar to one used for #2. If necessary, it can be added easily. New entries are added to 3 existing tests. What's previously not covered (raw 8bit Content-Disposition header) is now covered in all 3 tests. 

BUG=1148
TEST=net unit test: NetUtilTest.GetFileNameFromCD                    
                    NetUtilTest.GetSuggestedFilename         
     unittest : DownloadManagerTest.TestDownloadFilename


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

[jungshik at Google, 2009-04-21 22:41:46]

I'm not sure why ContextMenu..* test fails (it fails even with the patch removed
in my tree, but buildbots are ok...). Anyway, while I'm looking into it, I'm
asking for review.

[jungshik at Google, 2009-04-23 00:16:16]

Adding abarth per Paul.

[abarth-chromium, 2009-04-23 01:06:37]

I'm not a charset expert, but this looks entirely reasonable to me.  (Minor
style nits below.)

http://codereview.chromium.org/83002/diff/4202/4219
File chrome/browser/download/download_manager_unittest.cc (right):

http://codereview.chromium.org/83002/diff/4202/4219#newcode70
Line 70: {"", "https://blah.google.com/misc/2.avi", "video/x-msvideo", 
L"2.avi"},
Adding this space seems spurious.

http://codereview.chromium.org/83002/diff/4202/4204
File webkit/glue/context_menu_client_impl.cc (right):

http://codereview.chromium.org/83002/diff/4202/4204#newcode176
Line 176:
webkit_glue::StringToStdWString(selected_frame->loader()->encoding()));
Need two more spaces of indent here.

[Paul Godavari, 2009-04-24 01:47:18]

LGTM.

http://codereview.chromium.org/83002/diff/4202/4207
File chrome/browser/download/download_manager.h (right):

http://codereview.chromium.org/83002/diff/4202/4207#newcode359
Line 359: const std::string& referrer_encoding,
Just curious why use 'referrer_encoding' here, when you use 'origin_charset' or
'frame_encoding' everywhere else?

http://codereview.chromium.org/83002/diff/4202/4220
File chrome/browser/download/save_package.cc (right):

http://codereview.chromium.org/83002/diff/4202/4220#newcode300
Line 300: // makes sense and pass it to GetSuggestedFilename.
Can you file a bug so we can track it?

[jungshik at Google, 2009-04-29 00:59:26]

Thank you, Paul and Adam for review. 
I've been waiting for Darin to take a look, but I realized that he's on vacation
this week. I'm asking Wan-Teh to take a look. 

As for encoding vs charset, there's no particular reason. Perhaps, I'll just use
'charset' everywhere.

[wtc, 2009-04-29 19:18:07]

Jungshik, do you want me to look at the whole CL or just the
files in the net module (or some other subset of the CL)?

[jungshik at Google, 2009-04-29 23:09:39]

On 2009/04/29 19:18:07, wtc wrote:
> Jungshik, do you want me to look at the whole CL or just the
> files in the net module (or some other subset of the CL)?

Replying via email doesn't show up here. Just in case, Wan-Teh missed it. I'm
replying again here.

I don't mind if you look at the whole CL, but just looking at files in the net
module would suffice because Paul and Adam already took a look at other parts.

[wtc, 2009-04-29 23:50:22]

LGTM.  I only reviewed your changes to the net module.

Please review my comments below and fix the nits as
appropriate.

Some of the things you said in the CL's description probably
should be moved to the source files so that we don't need
to do "svn log" to get this useful info.  Just a thought...

http://codereview.chromium.org/83002/diff/4202/4214
File net/base/net_util.h (right):

http://codereview.chromium.org/83002/diff/4202/4214#newcode74
Line 74: // Return the filename extracted from Content-Disposition header.  Only
two
Please review this block comment and update it as
appropriate.  For example, your CL's description says:

  With this change, we support the following:

  1. RFC 2047
  2. Raw-8bit-characters : a. UTF-8, b. origin_charset,
     c. default os codepage. 
  3. %-escaped UTF-8.

but this block comment says:

  Only two formats are supported: a. %-escaped UTF-8
  b. RFC 2047.

In addition, the comment should document the new
origin_charset parameter and the fact that it can be
an empty string (meaning the origin charset is unknown).

http://codereview.chromium.org/83002/diff/4202/4214#newcode134
Line 134: // Gets the filename from the raw Content-Disposition header (as read
from the
Nit: we should also document the origin_charset and
default_name parameters.

http://codereview.chromium.org/83002/diff/4202/4215
File net/base/net_util_unittest.cc (right):

http://codereview.chromium.org/83002/diff/4202/4215#newcode42
Line 42: const char* charset;
Should we name this member origin_charset?

http://codereview.chromium.org/83002/diff/4202/4215#newcode358
Line 358: #if 0
Did you mean to add this commented-out test case?

http://codereview.chromium.org/83002/diff/4202/4215#newcode821
Line 821: test_cases[i].default_filename);
Nit: this line should fit on the previous line...

http://codereview.chromium.org/83002/diff/4202/4212
File net/url_request/url_request_context.h (right):

http://codereview.chromium.org/83002/diff/4202/4212#newcode95
Line 95: // The charset of the referrer where this request comes from. It's not
Should we name this member referer_charset_, since it's the
charset of the referrer?