Bug 217747 – Introduce new download API

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Introduce new download SPI

[Alex Christensen]

Created attachment 411398 [details]
Patch

[Alex Christensen]

Still needs a bunch of polish, but feel free to give feedback on the general direction.

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Geoffrey Garen]

Comment on attachment 411398 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/_WKDownload2.h:40
> +- (void)cancelByProducingResumeData:(void (^)(NSData * _Nullable resumeData))completionHandler;

I would just call this cancel. The parameter name already describes what is passed to the completion handler.

> Source/WebKit/UIProcess/API/Cocoa/_WKDownloadDelegate2.h:47
> +// FIXME: Blobs don't have a response, but can still have a suggested filename.
> +// Separate callback, or nullable response and separate string?
> +- (void)download:(_WKDownload2 *)download decideDestinationWithResponse:(NSURLResponse *)response completionHandler:(void (^)(NSURL *destination))completionHandler;

Seems like the suggested filename should be its own parameter, and response should be nullable. (Seems OK, since most people will only look at the suggested filename.)

> Source/WebKit/UIProcess/API/Cocoa/_WKDownloadDelegate2.h:55
> +- (void)downloadProcessDidTerminate:(_WKDownload2 *)download;

Does this have to be separate from the didFail case? A termination seems like a failure to me. And I'm not sure what we're expecting people to do with the termination.

Side note: Navigation delegate calls it "...WebContentProcessDidTerminate".

[Alex Christensen]

Created attachment 411940 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/70556859>

[Alex Christensen]

Created attachment 413659 [details]
Patch

[Alex Christensen]

Created attachment 413711 [details]
Patch

[Alex Christensen]

Created attachment 413914 [details]
Patch

[Alex Christensen]

Created attachment 413916 [details]
Patch

[Alex Christensen]

WKNavigationResponsePolicyBecomeDownload needs @constant header doc.  Will add before landing.

[Alex Christensen]

Comment on attachment 413916 [details]
Patch

Same with WKDownloadRedirectPolicy.  More header doc.

[Brady Eidson]

Comment on attachment 413916 [details]
Patch

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

I'm still looking at implementation stuff, but have some header comments

> Source/WebKit/UIProcess/API/Cocoa/WKDownload.h:48
> + @discussion The completionHandler is passed data to be used to resume the download if possible.

I'd make it more clear that the data is used to *attempt* to resume the download, and maybe even call out that due to server side considerations, some downloads may not be resumable.

I would also say something like "to attempt to resume the download, call WKWebView resumeDownloadWithData:"

> Source/WebKit/UIProcess/API/Cocoa/WKDownloadDelegate.h:2
> + * Copyright (C) 2014 Apple Inc. All rights reserved.

While I know 2014 was a happier time, sadly, I am quite aware that it's 2020.

> Source/WebKit/UIProcess/API/Cocoa/WKDownloadDelegate.h:42
> + @param download The web view that received the redirect.

/s/web view/download/. (Same comment applies to all of these)

> Source/WebKit/UIProcess/API/Cocoa/WKDownloadDelegate.h:72
> + suggested download filename.  If the destination file URL is non-null, it must be a file
> + that does not exist yet in a directory that does exist.

"yet" is unnecessary.

The directory must exist and be writeable to the application.

> Source/WebKit/UIProcess/API/Cocoa/WKDownloadDelegate.h:95
> + @param resumeData The data to be used to resume this download if possible.

Call out the API where the resume data can be used (just like the cancel completion handler)

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.h:458
> +/* @abstract Begins a download.
> + @param request The request specifying the URL to download.
> + @param completionHandler A block called when the download has started.
> + */
> +- (void)downloadRequest:(NSURLRequest *)request completionHandler:(void(^)(WKDownload *))completionHandler WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));

The purpose of this method is to begin downloading a resource *in the context of the currently displayed webpage as if the policy delegate made a load into a download instead"

I would explicitly call this out.

[Alex Christensen]

Created attachment 413978 [details]
Patch

[Alex Christensen]

Created attachment 413992 [details]
Patch

[Alex Christensen]

rdar://problem/26818508

[Alex Christensen]

*** Bug 158801 has been marked as a duplicate of this bug. ***

[Alex Christensen]

Created attachment 414415 [details]
Patch

[Brady Eidson]

Comment on attachment 414415 [details]
Patch

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

Besides fixing EWSs, etc... seems fine.

Except for calling downloads "web views"

> Source/WebKit/UIProcess/API/Cocoa/WKDownloadDelegate.h:57
> + @param download The web view that received the authentication challenge.

Download

> Source/WebKit/UIProcess/API/Cocoa/WKDownloadDelegate.h:82
> + @param download The web view that has written data.

Download

> Source/WebKit/UIProcess/API/Cocoa/WKDownloadDelegate.h:93
> + @param download The web view that finished.

Download

> Source/WebKit/UIProcess/API/Cocoa/WKDownloadDelegate.h:98
> + @param download The web view that has failed.

Download

[Alex Christensen]

Created attachment 414491 [details]
Patch

[Alex Christensen]

Created attachment 414497 [details]
Patch

[Alex Christensen]

Created attachment 414502 [details]
Patch

[Alex Christensen]

Created attachment 414532 [details]
Patch

[Alex Christensen]

Created attachment 414596 [details]
Patch

[Alex Christensen]

Comment on attachment 414596 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKDownloadDelegate.h:70
> + @param response The server response, if this download was the result of an HTTP request.

I need to add "or a synthesized response for blob downloads"
Also, response should not be nullable.  I thought it would be with blobs but it's not.

[Brady Eidson]

Comment on attachment 414596 [details]
Patch

(Relatively) Minor changes from last time, still r+

[Alex Christensen]

Created attachment 414617 [details]
Patch

[Alex Christensen]

Comment on attachment 414617 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKNavigation.mm:48
> +- (BOOL)_wasUserInitiated

This is not actually needed.  Will remove.

[Alex Christensen]

Created attachment 414708 [details]
Patch

[Alex Christensen]

Created attachment 415365 [details]
Patch

[EWS]

Committed r270422: <https://trac.webkit.org/changeset/270422>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 415365 [details].

[WebKit Commit Bot]

Re-opened since this is blocked by bug 219554

[Alexey Proskuryakov]

Reverted in r270454, please see radar for details.

[Alex Christensen]

Comment on attachment 415365 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKDownloadDelegate.h:68
> +/* @abstract Invoked when the download needs to respond to an authentication challenge.

This isn't correct.  Also, I'm going to make decideDestinationWithResponse mandatory because downloads will always fail if it is not implemented.

[Alex Christensen]

Created attachment 415823 [details]
Patch

[Alex Christensen]

Created attachment 415824 [details]
Patch

[EWS]

Committed r270638: <https://trac.webkit.org/changeset/270638>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 415824 [details].