Bug 237234

Summary: Add SPI _WKDataTask
Product: WebKit Reporter: Alex Christensen <achristensen>
Component: Page LoadingAssignee: Alex Christensen <achristensen>
Status: RESOLVED FIXED    
Severity: Normal CC: beidson, benjamin, cdumez, cmarcelo, ews-watchlist, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Alex Christensen 2022-02-25 21:35:43 PST 
Add SPI _WKDataTask
Comment 1 Alex Christensen 2022-02-25 21:38:56 PST 
Created attachment 453286 [details]
Patch
Comment 2 Alex Christensen 2022-02-25 21:51:54 PST 
Created attachment 453287 [details]
Patch
Comment 3 Alex Christensen 2022-02-26 07:02:44 PST 
Created attachment 453293 [details]
Patch
Comment 4 Alex Christensen 2022-02-26 07:03:41 PST 
Created attachment 453294 [details]
Patch
Comment 5 Tim Horton 2022-02-28 23:23:59 PST 
Comment on attachment 453294 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/_WKDataTask.h:41
> +// Calling this will often delegate's dataTask:didCompleteWithError: with { NSURLErrorDomain, NSURLErrorCancelled }

often?
Comment 6 Tim Horton 2022-02-28 23:44:22 PST 
Comment on attachment 453294 [details]
Patch

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

> Source/WebKit/NetworkProcess/cocoa/WKURLSessionTaskDelegate.mm:76
> +- (void)URLSession:(NSURLSession *)session task:(NSURLSessionTask *)task willPerformHTTPRedirection:(NSHTTPURLResponse *)response newRequest:(NSURLRequest *)request completionHandler:(void (^)(NSURLRequest * _Nullable))completionHandler

No nullability in implementation files (because it's viral).

> Source/WebKit/UIProcess/API/APIDataTask.cpp:56
> +    , m_client(DataTaskClient::create()) { }

Odd place for those curly braces

> Source/WebKit/UIProcess/API/APIDataTaskClient.h:50
> +    virtual void willPerformHTTPRedirection(DataTask&, WebCore::ResourceResponse&&, WebCore::ResourceRequest&&, CompletionHandler<void(bool)>&& completionHandler) const { completionHandler(true); }

Is `HTTP` proper WebKit style or `Http`? (I like the all-caps version but I think WebKit style does not)

> Source/WebKit/UIProcess/API/Cocoa/_WKDataTaskDelegate.h:51
> +- (void)dataTask:(_WKDataTask *)dataTask didCompleteWithError:(nullable NSError *)error;

Why both nullable and _Nullable?
Comment 7 Tim Horton 2022-02-28 23:44:35 PST 
Probably should get a Brady review too
Comment 8 Alex Christensen 2022-03-01 09:14:50 PST 
(In reply to Tim Horton from comment #6)
> > Source/WebKit/UIProcess/API/Cocoa/_WKDataTaskDelegate.h:51
> > +- (void)dataTask:(_WKDataTask *)dataTask didCompleteWithError:(nullable NSError *)error;
> 
> Why both nullable and _Nullable?
nullable for selector parameters, _Nullable for block parameters
Comment 9 Alex Christensen 2022-03-01 09:15:16 PST 
I think I'm also going to make the delegate strong until cancelled or completed, kind of like NSURLSessionTask, and I'll make cancel give you no more callbacks.
Comment 10 Alex Christensen 2022-03-01 12:58:32 PST 
Created attachment 453528 [details]
Patch
Comment 11 Radar WebKit Bug Importer 2022-03-01 15:30:38 PST 
<rdar://problem/89645347>
Comment 12 Alex Christensen 2022-03-01 22:42:20 PST 
Created attachment 453571 [details]
Patch
Comment 13 EWS 2022-03-02 00:10:21 PST 
Committed r290722 (247969@main): <https://commits.webkit.org/247969@main>

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