Created attachment 326613 [details] Patch

Comment on attachment 326613 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=326613&action=review r=me, with nits. > Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:617 > + [self.session addDelegateOperation:[strongSelf = RetainPtr<WebCoreNSURLSessionDataTask>(self), response = RetainPtr<NSURLResponse>(response.nsURLResponse()), request = WTFMove(request), completionHandler = WTFMove(completionHandler)] () mutable { Can't these "RetainPtr<...>(...)" calls just be "retainPtr(...)" calls? > Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:619 > + RetainPtr<NSURLRequest> nsRequest = request.nsURLRequest(DoNotUpdateHTTPBody); auto? Or maybe no need for this variable at all. > Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:621 > + [dataDelegate URLSession:(NSURLSession *)strongSelf.get().session task:(NSURLSessionTask *)strongSelf.get() willPerformHTTPRedirection:(NSHTTPURLResponse *)response.get() newRequest:nsRequest.get() completionHandler:BlockPtr<void(NSURLRequest *)>::fromCallable([completionHandler = WTFMove(completionHandler)](NSURLRequest *newRequest) mutable { This is ... kinda ugly. There should be some kind of templated "blockPtr()" which derives the BlockPtr template type from the signature of the callable, so that this could be written "completionHandler:blockPtr([...] { ... })"; Also, you're creating a block, wrapping it in a BlockPtr, just to call the completionHandler with the same signature as the BlockPtr and the block. That's screaming for some simplification. Also, why does response.get() have to be casted to a (NSHTTPURLResponse *)? response is a RetainPtr<NSURLResponse>.

(In reply to Jer Noble from comment #2) > This is ... kinda ugly. There should be some kind of templated "blockPtr()" > which derives the BlockPtr template type from the signature of the callable, > so that this could be written "completionHandler:blockPtr([...] { ... })"; > Also, you're creating a block, wrapping it in a BlockPtr, just to call the > completionHandler with the same signature as the BlockPtr and the block. > That's screaming for some simplification. We do the same thing in our ObjC API. I've made it two separate statements to be more clear. > Also, why does response.get() have to be casted to a (NSHTTPURLResponse *)? > response is a RetainPtr<NSURLResponse>. This API requires an NSHTTPURLResponse. Not all NSURLResponses are NSHTTPURLResponses. Ours should be, but I'll add an early return and an assert if it isn't.

<rdar://problem/35562336>

(In reply to Alex Christensen from comment #3) > (In reply to Jer Noble from comment #2) > > Also, why does response.get() have to be casted to a (NSHTTPURLResponse *)? > > response is a RetainPtr<NSURLResponse>. > This API requires an NSHTTPURLResponse. Not all NSURLResponses are > NSHTTPURLResponses. Ours should be, but I'll add an early return and an > assert if it isn't. Duh. Of course.