Summary: | [Mac][WK2] Use MIME type to add file extension to downloads' suggested filenames when missing | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||
Component: | DOM | Assignee: | Chris Dumez <cdumez> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | achristensen, bfulgham, cdumez, commit-queue, darin, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | 202147 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
Chris Dumez
2017-02-09 14:16:38 PST
Created attachment 301083 [details]
WIP Patch
Still need to fix the !NETWORK_SESSION code path so it will likely fail on EWS. Created attachment 301101 [details]
Patch
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? (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. Created attachment 301118 [details]
Patch
Comment on attachment 301118 [details] Patch Clearing flags on attachment: 301118 Committed r212041: <http://trac.webkit.org/changeset/212041> All reviewed patches have been landed. Closing bug. 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.
(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. (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. |