Bug 168077 – [Mac][WK2] Use MIME type to add file extension to downloads' suggested filenames when missing

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Chris Dumez]

Use MIME type to add file extension to downloads' suggested filenames when missing:
- https://html.spec.whatwg.org/#as-a-download (step 19)

[Chris Dumez]

<rdar://problem/30412595>

[Chris Dumez]

Created attachment 301083 [details]
WIP Patch

[Chris Dumez]

Still need to fix the !NETWORK_SESSION code path so it will likely fail on EWS.

[Chris Dumez]

Created attachment 301101 [details]
Patch

[Alex Christensen]

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?

[Chris Dumez]

(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.

[Chris Dumez]

Created attachment 301118 [details]
Patch

[Chris Dumez]

Comment on attachment 301118 [details]
Patch

Clearing flags on attachment: 301118

Committed r212041: <http://trac.webkit.org/changeset/212041>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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.

[Chris Dumez]

(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.

[Darin Adler]

(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.