Fix bug 79905: Drag and drop of "DownloadURL" type ignores specified filename for data URLs.

Fix bug 79905: Drag and drop of "DownloadURL" type ignores specified filename for data URLs.

BUG=79905
TEST=Manual test and unittest added for net::GetSuggestedFilename
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=85831

[Randy Smith (Not in Mondays), 2011-05-10 22:27:26]

Added Asanka@ to the reviewer list; he's been working in this code.

[asanka, 2011-05-10 22:43:42]

It seems to me that the correct approach is to fix the logic in
net_util::GetSuggestedFilename() ( in /net/base/net_util.cc ).  There you'll see
that we have a short-circuit if the URL scheme is "about" or "data" and we don't
check the C-D.  You'll want move that check below the call to
GetFilenameFromCD() and also only exit if GetFilenameFromCD() returned an empty
filename.

It would also be great to add a test case or two to either
download_util_unittest (DownloadUtilTest.GenerateFileName) or net_util_unittest
(NetUtilTest.GetSuggestedFilename) for this case so we don't break this later.

[jianli, 2011-05-10 23:21:28]

I've thought about this approach and am hesitant to move the check to the
lower level function since I do not want to "pollute" it. Normally we should
not have CD provided for data URL. Currently only for drag-out-of-browser
scenario, we have both requested file name and data URL provided in the
DownloadURL value and we construct an artificial CD from this file name in
order for GetSuggestedFilename to using it.

On Tue, May 10, 2011 at 3:43 PM, <asanka@chromium.org> wrote:

> It seems to me that the correct approach is to fix the logic in
> net_util::GetSuggestedFilename() ( in /net/base/net_util.cc ).  There
> you'll see
> that we have a short-circuit if the URL scheme is "about" or "data" and we
> don't
> check the C-D.  You'll want move that check below the call to
> GetFilenameFromCD() and also only exit if GetFilenameFromCD() returned an
> empty
> filename.
>
> It would also be great to add a test case or two to either
> download_util_unittest (DownloadUtilTest.GenerateFileName) or
> net_util_unittest
> (NetUtilTest.GetSuggestedFilename) for this case so we don't break this
> later.
>
>
>
> http://codereview.chromium.org/7005011/
>

[asanka, 2011-05-12 16:40:46]

On 2011/05/10 23:21:28, jianli wrote:
> I've thought about this approach and am hesitant to move the check to the
> lower level function since I do not want to "pollute" it. Normally we should
> not have CD provided for data URL. Currently only for drag-out-of-browser
> scenario, we have both requested file name and data URL provided in the
> DownloadURL value and we construct an artificial CD from this file name in
> order for GetSuggestedFilename to using it.

Do you know what changed and broke this logic?  Looks like passing in a
constructed C-D along with a data URL was expected to work at some point.

I'd like to see this avoid constructing a Content-Disposition string here at
all.  There are a few cases where this won't do what you want.

[Randy Smith (Not in Mondays), 2011-05-12 17:40:55]

Scanning the CL, I don't think I have much to contribute beyond what Asanka
will, so I'm removing myself from reviewing it.

[jianli, 2011-05-12 17:54:03]

On Thu, May 12, 2011 at 9:40 AM, <asanka@chromium.org> wrote:

> On 2011/05/10 23:21:28, jianli wrote:
>
>> I've thought about this approach and am hesitant to move the check to the
>> lower level function since I do not want to "pollute" it. Normally we
>> should
>> not have CD provided for data URL. Currently only for drag-out-of-browser
>> scenario, we have both requested file name and data URL provided in the
>> DownloadURL value and we construct an artificial CD from this file name in
>> order for GetSuggestedFilename to using it.
>>
>
> Do you know what changed and broke this logic?  Looks like passing in a
> constructed C-D along with a data URL was expected to work at some point.
>

This is not a regression. The user finds out that using data URL is very
useful and it makes sense.

I think normally we should not expect C-D to come with data URL. Only for
drag-out-of-browser case, we construct C-D in order to call the helper
function GenerateFileName. See my comment below for suggestion to fix this.

>
> I'd like to see this avoid constructing a Content-Disposition string here
> at
> all.  There are a few cases where this won't do what you want.


How about that we introduce another helper function which will take
suggested_file_name instead of content_disposition, like the following:

void GenerateFileNameFromSuggestedFileName(const GURL& url,
                      const std::string& suggested_file_name,
                      const std::string& referrer_charset,
                      const std::string& mime_type,
                      FilePath* generated_name);


>
>
> http://codereview.chromium.org/7005011/
>

[asanka, 2011-05-12 19:16:46]

On 2011/05/12 17:54:03, jianli wrote:
> This is not a regression. The user finds out that using data URL is very
> useful and it makes sense.

Ok.

> How about that we introduce another helper function which will take
> suggested_file_name instead of content_disposition, like the following:
> 
> void GenerateFileNameFromSuggestedFileName(const GURL& url,
>                       const std::string& suggested_file_name,
>                       const std::string& referrer_charset,
>                       const std::string& mime_type,
>                       FilePath* generated_name);

Sounds much better.

[jianli, 2011-05-12 23:45:25]

I've made the fix. Please review again. Thanks.

On Thu, May 12, 2011 at 12:16 PM, <asanka@chromium.org> wrote:

> On 2011/05/12 17:54:03, jianli wrote:
>
>> This is not a regression. The user finds out that using data URL is very
>> useful and it makes sense.
>>
>
> Ok.
>
>
>  How about that we introduce another helper function which will take
>> suggested_file_name instead of content_disposition, like the following:
>>
>
>  void GenerateFileNameFromSuggestedFileName(const GURL& url,
>>                       const std::string& suggested_file_name,
>>                       const std::string& referrer_charset,
>>                       const std::string& mime_type,
>>                       FilePath* generated_name);
>>
>
> Sounds much better.
>
>
>
> http://codereview.chromium.org/7005011/
>

[asanka, 2011-05-13 13:32:24]

LGTM with the changes I mention below.

You'll need OWNERS approval though.

http://codereview.chromium.org/7005011/diff/11005/chrome/browser/download/dow...
File chrome/browser/download/download_util.cc (right):

http://codereview.chromium.org/7005011/diff/11005/chrome/browser/download/dow...
chrome/browser/download/download_util.cc:170: void
GenerateFileNameInternal(const GURL& url,
Can you also change GenerateFileNameFromInfo() to call
GenerateFileNameInternal() directly?

http://codereview.chromium.org/7005011/diff/11005/chrome/browser/ui/cocoa/tab...
File chrome/browser/ui/cocoa/tab_contents/web_drag_source.mm (left):

http://codereview.chromium.org/7005011/diff/11005/chrome/browser/ui/cocoa/tab...
chrome/browser/ui/cocoa/tab_contents/web_drag_source.mm:406: "attachment;
filename=" + fileName.value();
Thanks for getting rid of this!

http://codereview.chromium.org/7005011/diff/11005/net/base/net_util_unittest.cc
File net/base/net_util_unittest.cc (right):

http://codereview.chromium.org/7005011/diff/11005/net/base/net_util_unittest....
net/base/net_util_unittest.cc:1262: L"suggested"},
Could you also add a test to check that C-D has higher precedence over the
suggested name?

[jianli, 2011-05-17 00:10:44]

rdsmith, can you take a look to approve it? Thanks.

http://codereview.chromium.org/7005011/diff/11005/chrome/browser/download/dow...
File chrome/browser/download/download_util.cc (right):

http://codereview.chromium.org/7005011/diff/11005/chrome/browser/download/dow...
chrome/browser/download/download_util.cc:170: void
GenerateFileNameInternal(const GURL& url,
On 2011/05/13 13:32:24, asanka wrote:
> Can you also change GenerateFileNameFromInfo() to call
> GenerateFileNameInternal() directly?

Done.

http://codereview.chromium.org/7005011/diff/11005/net/base/net_util_unittest.cc
File net/base/net_util_unittest.cc (right):

http://codereview.chromium.org/7005011/diff/11005/net/base/net_util_unittest....
net/base/net_util_unittest.cc:1262: L"suggested"},
On 2011/05/13 13:32:24, asanka wrote:
> Could you also add a test to check that C-D has higher precedence over the
> suggested name?

Done.

[Randy Smith (Not in Mondays), 2011-05-17 18:32:08]

Rubber-stamp LGTM for /downloads/ directory based on Asanka's review.

[jianli, 2011-05-17 18:34:56]

Thanks. Since you're also listed as owner for net/ directory, can you also
rubber-stamp it?


On Tue, May 17, 2011 at 11:32 AM, <rdsmith@chromium.org> wrote:

> Rubber-stamp LGTM for /downloads/ directory based on Asanka's review.
>
>
>
> http://codereview.chromium.org/7005011/
>

[Randy Smith (Not in Mondays), 2011-05-17 18:37:49]

On 2011/05/17 18:34:56, jianli wrote:
> Thanks. Since you're also listed as owner for net/ directory, can you also
> rubber-stamp it?

My quick scan suggests that that was part of Asanka's review, and I'm willing to
rubber stamp LGTM on that basis (but if it wasn't, please let me know that and
I'll do a real review).

> 
> 
> On Tue, May 17, 2011 at 11:32 AM, <mailto:rdsmith@chromium.org> wrote:
> 
> > Rubber-stamp LGTM for /downloads/ directory based on Asanka's review.
> >
> >
> >
> > http://codereview.chromium.org/7005011/
> >

[jianli, 2011-05-17 18:39:54]

Yes, Asanka has reviewed it. Thanks.

On Tue, May 17, 2011 at 11:37 AM, <rdsmith@chromium.org> wrote:

> On 2011/05/17 18:34:56, jianli wrote:
>
>> Thanks. Since you're also listed as owner for net/ directory, can you also
>> rubber-stamp it?
>>
>
> My quick scan suggests that that was part of Asanka's review, and I'm
> willing to
> rubber stamp LGTM on that basis (but if it wasn't, please let me know that
> and
> I'll do a real review).
>
>
>
>
>  On Tue, May 17, 2011 at 11:32 AM, <mailto:rdsmith@chromium.org> wrote:
>>
>
>  > Rubber-stamp LGTM for /downloads/ directory based on Asanka's review.
>> >
>> >
>> >
>> > http://codereview.chromium.org/7005011/
>> >
>>
>
>
>
> http://codereview.chromium.org/7005011/
>