[webkit-reviews] review granted: [Bug 129322] [iOS] Download support by CFURLDownloadRef under USE(CFNETWORK). : [Attachment 227570] [iOS] Download support by CFURLDownloadRef under USE(CFNETWORK).

[bugzilla-daemon at webkit.org]

Anders Carlsson <andersca at apple.com> has granted Andy Estes
<aestes at apple.com>'s request for review:
Bug 129322: [iOS] Download support by CFURLDownloadRef under USE(CFNETWORK).
https://bugs.webkit.org/show_bug.cgi?id=129322

Attachment 227570: [iOS] Download support by CFURLDownloadRef under
USE(CFNETWORK).
https://bugs.webkit.org/attachment.cgi?id=227570&action=review

------- Additional Comments from Anders Carlsson <andersca at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=227570&action=review


Looks great overall. Please move the DownloadClient to its own file (similar to
UIClient and HistoryClient). The reason why i decided to make the C SPI clients
local classes was to encapsulate that legacy C SPI in as few files as possible.
We don't need to do that with the modern API.

> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:466
> +    return adoptCF(d->m_connection.leakRef());

I think this can just be std::move(d->m_connection);

> Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:333
> +    RetainPtr<CFURLConnectionRef> connection =
handle->releaseConnectionForDownload();

auto connection =

> Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:338
>								       
request:request.cfURLRequest(UpdateHTTPBody)
>								      
response:response.cfURLResponse()
>								      
delegate:[webView downloadDelegate]
>									 
proxy:nil];

I'd get rid of this weird lineup indentation and put everything on a single
row.

> Source/WebKit2/Shared/Downloads/ios/DownloadIOS.mm:45
> +// making them asynchorous calls.

Spelling error, asynchorous.

> Source/WebKit2/Shared/Downloads/ios/DownloadIOS.mm:67
> +    {

Lambdas aren't functions (well they are), so there shouldn't be a newline
before the { here. (Same for all the other callbacks).

> Source/WebKit2/Shared/Downloads/ios/DownloadIOS.mm:75
> +	   return request;

I think this needs to return a retained request.

> Source/WebKit2/Shared/Downloads/ios/DownloadIOS.mm:128
> +	       RetainPtr<CFDataRef> resumeData =
adoptCF(CFURLDownloadCopyResumeData(downloadRef));

auto resumeData =