Use the suggested name from an anchor's 'download' attribute.

Use the suggested name from an anchor's 'download' attribute.

This uses the feature added in WebKit: https://bugs.webkit.org/show_bug.cgi?id=64580

BUG=89346
TEST=manually

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

[sadrul, 2011-07-24 21:36:31]

pawel: please review the changes in chrome/browser/download

jam: please review the changes in content/

abart, darin: FYI

http://codereview.chromium.org/7484061/diff/2001/chrome/browser/download/down...
File chrome/browser/download/download_create_info.h (left):

http://codereview.chromium.org/7484061/diff/2001/chrome/browser/download/down...
chrome/browser/download/download_create_info.h:50: FilePath suggested_path;
Removed because this is just not used anywhere.

[sadrul, 2011-07-25 03:54:33]

On 2011/07/24 21:36:31, sadrul wrote:
> pawel: please review the changes in chrome/browser/download
> 
> jam: please review the changes in content/
> 
> abart, darin: FYI
> 
>
http://codereview.chromium.org/7484061/diff/2001/chrome/browser/download/down...
> File chrome/browser/download/download_create_info.h (left):
> 
>
http://codereview.chromium.org/7484061/diff/2001/chrome/browser/download/down...
> chrome/browser/download/download_create_info.h:50: FilePath suggested_path;
> Removed because this is just not used anywhere.

A browser-test will be added (CL: http://codereview.chromium.org/7484063/) once
the necessary webkit CLs land and are rolled in.

[Randy Smith (Not in Mondays), 2011-07-25 11:41:58]

Adding Andy to CL for reviewing suggested_name removal, and Asanka for file name
generation change.  (Asanka: I didn't look in detail, but I'm concerned the file
name generation change conflicts with your current CL.)

[sadrul, 2011-07-25 14:03:11]

On 2011/07/25 11:41:58, rdsmith wrote:
> Adding Andy to CL for reviewing suggested_name removal, and Asanka for file
name
> generation change.  (Asanka: I didn't look in detail, but I'm concerned the
file
> name generation change conflicts with your current CL.)

There may be conflict with Asanka's CL (I suspect you mean
http://codereview.chromium.org/7300005/?), but it should be trivial to resolve.
The same should be the case with Andy's CL too.

[asanka, 2011-07-25 15:20:03]

On 2011/07/25 14:03:11, sadrul wrote:
> On 2011/07/25 11:41:58, rdsmith wrote:
> > Adding Andy to CL for reviewing suggested_name removal, and Asanka for file
> name
> > generation change.  (Asanka: I didn't look in detail, but I'm concerned the
> file
> > name generation change conflicts with your current CL.)
> 
> There may be conflict with Asanka's CL (I suspect you mean
> http://codereview.chromium.org/7300005/?), but it should be trivial to
resolve.
> The same should be the case with Andy's CL too.

The conflict with the refactor CL is minor.

Regarding the filename generation change: Is it expected that the value of the
download attribute will be used even for an untrusted operation (when the
origins of the referrer and the download resource differ and there's no
Content-Disposition)?

[sadrul, 2011-07-25 15:25:23]

On 2011/07/25 15:20:03, asanka wrote:
> On 2011/07/25 14:03:11, sadrul wrote:
> > On 2011/07/25 11:41:58, rdsmith wrote:
> > > Adding Andy to CL for reviewing suggested_name removal, and Asanka for
file
> > name
> > > generation change.  (Asanka: I didn't look in detail, but I'm concerned
the
> > file
> > > name generation change conflicts with your current CL.)
> > 
> > There may be conflict with Asanka's CL (I suspect you mean
> > http://codereview.chromium.org/7300005/?), but it should be trivial to
> resolve.
> > The same should be the case with Andy's CL too.
> 
> The conflict with the refactor CL is minor.
> 
> Regarding the filename generation change: Is it expected that the value of the
> download attribute will be used even for an untrusted operation (when the
> origins of the referrer and the download resource differ and there's no
> Content-Disposition)?

Indeed. The value of the download attribute will be ignored only if there is a
filename suggested in the C-D header.

[asanka, 2011-07-25 17:49:27]

On 2011/07/25 15:25:23, sadrul wrote:
> Indeed. The value of the download attribute will be ignored only if there is a
> filename suggested in the C-D header.

Ah.  I asked because our handling of filename determination seems to differ
significantly from
http://www.whatwg.org/specs/web-apps/current-work/#as-a-download .  But that's
not just because of the download attribute.

Could you add a test for this?

[sadrul, 2011-07-25 17:52:09]

On 2011/07/25 17:49:27, asanka wrote:
> On 2011/07/25 15:25:23, sadrul wrote:
> > Indeed. The value of the download attribute will be ignored only if there is
a
> > filename suggested in the C-D header.
> 
> Ah.  I asked because our handling of filename determination seems to differ
> significantly from
> http://www.whatwg.org/specs/web-apps/current-work/#as-a-download .  But that's
> not just because of the download attribute.
> 
> Could you add a test for this?

Already added: http://codereview.chromium.org/7484063/ (the tests will pass only
when the webkit CL lands and is rolled in)

[jam, 2011-07-25 18:16:48]

content lgtm

[ahendrickson, 2011-07-25 19:06:18]

LGTM.

When resuming downloads is implemented, we may need a suggested name field. 
I'll worry about that later.

[asanka, 2011-07-25 19:42:02]

On 2011/07/25 17:52:09, sadrul wrote:
> On 2011/07/25 17:49:27, asanka wrote:
> > Could you add a test for this?
> 
> Already added: http://codereview.chromium.org/7484063/ (the tests will pass
only
> when the webkit CL lands and is rolled in)

Excellent!  My only concern is that we are storing the suggested path in
DownloadSaveInfo which seems to be currently used to store drop target
information.  Meanwhile, the other parameters that factor into the filename
determination are passed separately until the DownloadItem is created.

Have you considered passing the suggested filename as a separate parameter in
DownloadCreateInfo?

Otherwise LGTM.

[sadrul, 2011-07-25 22:22:30]

On 2011/07/25 19:42:02, asanka wrote:
> On 2011/07/25 17:52:09, sadrul wrote:
> > On 2011/07/25 17:49:27, asanka wrote:
> > > Could you add a test for this?
> > 
> > Already added: http://codereview.chromium.org/7484063/ (the tests will pass
> only
> > when the webkit CL lands and is rolled in)
> 
> Excellent!  My only concern is that we are storing the suggested path in
> DownloadSaveInfo which seems to be currently used to store drop target
> information.  Meanwhile, the other parameters that factor into the filename
> determination are passed separately until the DownloadItem is created.
> 
> Have you considered passing the suggested filename as a separate parameter in
> DownloadCreateInfo?

We need to store the suggested filename somewhere until the time a
DownloadCreateInfo is created because of a click-event on a@download (since
DownloadCreateInfo is created from a callback). The simplest way is to store
this information in DownloadSaveInfo. I think this is not only convenient, but
also a relevant place to store the information (since DownloadSaveInfo "// Holds
the information about how to save a download file."). It shouldn't conflict with
the current usage of DownloadSaveInfo to store the relevant info for a
dragged-file.

> Otherwise LGTM.

[Paweł Hajdan Jr., 2011-07-25 23:08:43]

LGTM

[asanka, 2011-07-26 15:32:46]

On 2011/07/25 22:22:30, sadrul wrote:
> On 2011/07/25 19:42:02, asanka wrote:
> > Have you considered passing the suggested filename as a separate parameter
in
> > DownloadCreateInfo?
> 
> We need to store the suggested filename somewhere until the time a
> DownloadCreateInfo is created because of a click-event on a@download (since
> DownloadCreateInfo is created from a callback). The simplest way is to store
> this information in DownloadSaveInfo. I think this is not only convenient, but
> also a relevant place to store the information (since DownloadSaveInfo "//
Holds
> the information about how to save a download file."). It shouldn't conflict
with
> the current usage of DownloadSaveInfo to store the relevant info for a
> dragged-file.

Ok. Thanks.

LGTM.