[webkit-reviews] review granted: [Bug 179539] Call willPerformHTTPRedirection on WebCoreNSURLSession's delegate : [Attachment 326613] Patch

[bugzilla-daemon at webkit.org]

Jer Noble <jer.noble at apple.com> has granted Alex Christensen
<achristensen at apple.com>'s request for review:
Bug 179539: Call willPerformHTTPRedirection on WebCoreNSURLSession's delegate
https://bugs.webkit.org/show_bug.cgi?id=179539

Attachment 326613: Patch

https://bugs.webkit.org/attachment.cgi?id=326613&action=review




--- Comment #2 from Jer Noble <jer.noble at apple.com> ---
Comment on attachment 326613
  --> https://bugs.webkit.org/attachment.cgi?id=326613
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>.