Introduce new download SPI
Created attachment 411398 [details] Patch
Still needs a bunch of polish, but feel free to give feedback on the general direction.
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
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".
Created attachment 411940 [details] Patch
<rdar://problem/70556859>
Created attachment 413659 [details] Patch
Created attachment 413711 [details] Patch
Created attachment 413914 [details] Patch
Created attachment 413916 [details] Patch
WKNavigationResponsePolicyBecomeDownload needs @constant header doc. Will add before landing.
Comment on attachment 413916 [details] Patch Same with WKDownloadRedirectPolicy. More header doc.
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.
Created attachment 413978 [details] Patch
Created attachment 413992 [details] Patch
rdar://problem/26818508
*** Bug 158801 has been marked as a duplicate of this bug. ***
Created attachment 414415 [details] Patch
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
Created attachment 414491 [details] Patch
Created attachment 414497 [details] Patch
Created attachment 414502 [details] Patch
Created attachment 414532 [details] Patch
Created attachment 414596 [details] Patch
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.
Comment on attachment 414596 [details] Patch (Relatively) Minor changes from last time, still r+
Created attachment 414617 [details] Patch
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.
Created attachment 414708 [details] Patch
Created attachment 415365 [details] Patch
Committed r270422: <https://trac.webkit.org/changeset/270422> All reviewed patches have been landed. Closing bug and clearing flags on attachment 415365 [details].
Re-opened since this is blocked by bug 219554
Reverted in r270454, please see radar for details.
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.
Created attachment 415823 [details] Patch
Created attachment 415824 [details] Patch
Committed r270638: <https://trac.webkit.org/changeset/270638> All reviewed patches have been landed. Closing bug and clearing flags on attachment 415824 [details].