[Webkit-unassigned] [Bug 129322] [iOS] Download support by CFURLDownloadRef under USE(CFNETWORK).

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=129322


Anders Carlsson <andersca at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #227570|review?                     |review+
               Flag|                            |




--- Comment #16 from Anders Carlsson <andersca at apple.com>  2014-03-24 11:18:37 PST ---
(From update of attachment 227570)
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 =

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.