Bug 168077 - [Mac][WK2] Use MIME type to add file extension to downloads' suggested filenames when missing
Summary: [Mac][WK2] Use MIME type to add file extension to downloads' suggested filena...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on: 202147
Blocks:
  Show dependency treegraph
 
Reported: 2017-02-09 14:16 PST by Chris Dumez
Modified: 2019-09-24 14:24 PDT (History)
6 users (show)

See Also:


Attachments
WIP Patch (12.47 KB, patch)
2017-02-09 14:18 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (25.03 KB, patch)
2017-02-09 16:41 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (24.79 KB, patch)
2017-02-09 18:33 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2017-02-09 14:16:38 PST
Use MIME type to add file extension to downloads' suggested filenames when missing:
- https://html.spec.whatwg.org/#as-a-download (step 19)
Comment 1 Chris Dumez 2017-02-09 14:17:06 PST
<rdar://problem/30412595>
Comment 2 Chris Dumez 2017-02-09 14:18:21 PST
Created attachment 301083 [details]
WIP Patch
Comment 3 Chris Dumez 2017-02-09 14:27:10 PST
Still need to fix the !NETWORK_SESSION code path so it will likely fail on EWS.
Comment 4 Chris Dumez 2017-02-09 16:41:27 PST
Created attachment 301101 [details]
Patch
Comment 5 Alex Christensen 2017-02-09 17:25:25 PST
Comment on attachment 301101 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=301101&action=review

I thought it was the job of CFNetwork to look at the response headers, content sniffing, etc. and give us a good suggested filename.

> Source/WebKit2/NetworkProcess/Downloads/Download.h:150
> +    WebCore::ResourceResponse m_response;

Could we just store a String instead of the whole Response?
Comment 6 Chris Dumez 2017-02-09 18:29:21 PST
(In reply to comment #5)
> Comment on attachment 301101 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=301101&action=review
> 
> I thought it was the job of CFNetwork to look at the response headers,
> content sniffing, etc. and give us a good suggested filename.

One issue is that in the download attribute case, the suggested filename comes from the attribute, not from CFNetwork. So even if CFNetwork did this, it would not fix this issue.

> 
> > Source/WebKit2/NetworkProcess/Downloads/Download.h:150
> > +    WebCore::ResourceResponse m_response;
> 
> Could we just store a String instead of the whole Response?

Sure, will do.
Comment 7 Chris Dumez 2017-02-09 18:33:19 PST
Created attachment 301118 [details]
Patch
Comment 8 Chris Dumez 2017-02-09 20:02:27 PST
Comment on attachment 301118 [details]
Patch

Clearing flags on attachment: 301118

Committed r212041: <http://trac.webkit.org/changeset/212041>
Comment 9 Chris Dumez 2017-02-09 20:02:31 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Darin Adler 2017-02-12 01:38:20 PST
Comment on attachment 301118 [details]
Patch

On macOS and iOS, does this duplicate logic already present in CFNetwork? I know it has a similar concept.
Comment 11 Chris Dumez 2017-02-12 08:22:05 PST
(In reply to comment #10)
> Comment on attachment 301118 [details]
> Patch
> 
> On macOS and iOS, does this duplicate logic already present in CFNetwork? I
> know it has a similar concept.

That is quite possible. However, the radar I was fixing was about downloads that were triggered by anchors that have a download attribute. In this case, the suggested file name comes from the attribute, not from CFNetwork. This patch also helps for blob downloads, which do not involve CFNetwork.
Comment 12 Darin Adler 2017-02-12 10:47:47 PST
(In reply to comment #11)
> That is quite possible. However, the radar I was fixing was about downloads
> that were triggered by anchors that have a download attribute. In this case,
> the suggested file name comes from the attribute, not from CFNetwork. This
> patch also helps for blob downloads, which do not involve CFNetwork.

Sure, that’s the reason we need two sets of code that do almost the same thing. I think we should figure out how to keep these two sets of code following similar or same rules; to that end, maybe we can even come up with a way to actually share the code. It would be annoying long term if each mechanism had a separate list of MIME types and extensions and different set of rules for when to leave the existing extension alone and when to add a new extension of replace an existing one.