Bug 129322 – [iOS] Download support by CFURLDownloadRef under USE(CFNETWORK).

• Component: WebKit2
• Status: RESOLVED FIXED

[Yongjun Zhang]

It would be nice to enable download support by using CFURLDownload API under USE(CFNETWORK).

[Yongjun Zhang]

Created attachment 225173 [details]
Patch.

[Yongjun Zhang]

Created attachment 225185 [details]
Fixed a build break for Mac.

[Yongjun Zhang]

<rdar://problem/16125311>

[Darin Adler]

Comment on attachment 225185 [details]
Fixed a build break for Mac.

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

review- because there are at least three things here that I think need to be fixed. (The leak because of lack of adoptCF, the BOOL * vs. bool * casting, and the way the releaseConnectionForDownload relates to the CFRelease call after it.)

> Source/WebKit2/Shared/Downloads/ios/DownloadIOS.mm:31
> +#if USE(CFNETWORK)
> +#import <CFNetwork/CFURLDownload.h>
> +#endif

The correct WebKit project coding style is to put conditional imports in a separate paragraph after all the unconditional ones. But I don’t understand why this needs to be conditional? Is there someone who needs to compile DownloadIOS with USE(CFNETWORK) false? And if they do, is it really critical to avoid this include in that case? I suggest just a plain old import here with no conditional.

> Source/WebKit2/Shared/Downloads/ios/DownloadIOS.mm:39
> +using namespace WTF;

This is not correct. WTF headers are supposed to export their symbols with a "using" in the main namespace, and you should never need WTF:: prefixes. If you do need them, the fix should be in WTF, not adding one of these "using namespace".

> Source/WebKit2/Shared/Downloads/ios/DownloadIOS.mm:44
> +// FIXME: It would be nice if these callbacks wouldn't have to be invoked on the main thread.

Would be nice if this comment explained why we need to do it rather than just saying it would be nice if we didn’t.

> Source/WebKit2/Shared/Downloads/ios/DownloadIOS.mm:47
> +    if (RunLoop::isMain()) {

If we’re going to use RunLoop here, can we use the RunLoop version of "run on main thread" instead?

> Source/WebKit2/Shared/Downloads/ios/DownloadIOS.mm:56
> +static void setUpDownloadClient(CFURLDownloadClient &client, Download *dl)

Incorrect formatting here. It should be CFURLDownloadClient& with the space after the "&". Also, I suggest a name like download instead of "dl" for the download argument. Since the download argument can never be null, I suggest a reference rather than a pointer.

> Source/WebKit2/Shared/Downloads/ios/DownloadIOS.mm:69
> +    client.didStart = [](CFURLDownloadRef downloadRef, const void *clientInfo)
> +    {
> +        dispatchOnMainThread(^{
> +            Download *download = (Download *)clientInfo;
> +            download->didStart();
> +        });
> +    };

Why use clientInfo here to pass in the Download, when a lambda and a block can simply capture the pointer value instead. Avoiding the typecast would be really nice. Also, why not use lambdas for both rather than one lambda and one block? Same comments about the same idiom elsewhere in the patch. Also, I suggest omitting the argument names for unused arguments in the lambda.

> Source/WebKit2/Shared/Downloads/ios/DownloadIOS.mm:78
> +        notImplemented();

Is it really important to call the notImplemented function here? How will this help us in the future?

> Source/WebKit2/Shared/Downloads/ios/DownloadIOS.mm:110
> +        __block BOOL returnValue;
> +        dispatchOnMainThread(^{
> +            Download *download = (Download *)clientInfo;
> +            returnValue = download->shouldDecodeSourceDataOfMIMEType(encodingType);
> +        });
> +
> +        return returnValue;;

It seems a little strange to use a block here instead of a lambda.

> Source/WebKit2/Shared/Downloads/ios/DownloadIOS.mm:149
> +            RetainPtr<NSData> resumeData = (NSData *)CFURLDownloadCopyResumeData(downloadRef);
> +            IPC::DataReference dataReference(reinterpret_cast<const uint8_t*>([resumeData bytes]), [resumeData length]);

This leaks the resume data, because it omits the adoptCF call. Also, why convert the CFData to an NSData? CFDataGetBytePtr and CFDataGetLength would work fine without converting the type and even avoid the reinterpret cast, I believe. WebPage::getSelectionAsWebArchiveData and WebPage::getWebArchiveOfFrame have examples that do this correctly, and WebPage::drawPagesToPDF has an example I like even better that skips the local variable.

> Source/WebKit2/Shared/Downloads/ios/DownloadIOS.mm:170
> +    CFURLConnectionRef connection = handle->connection();
> +    m_download = CFURLDownloadCreateAndStartWithLoadingConnection(NULL, connection, m_request.cfURLRequest(UpdateHTTPBody), response.cfURLResponse(), &client);
> +    handle->releaseConnectionForDownload();
> +    CFRelease(connection);

This is a really awkward construction. It seems that ResourceHandle::releaseConnectionForDownload should return a RetainPtr of the connection. An explicit CFRelease like this seems both unclear and dangerous.

Is m_download a RetainPtr? It worries me to see a Create here and not see the CFRelease that balances it. I suggest use of RetainPtr.

> Source/WebKit2/UIProcess/API/Cocoa/WKProcessGroup.mm:179
> +static void didStart(WKContextRef context, WKDownloadRef download, const void *clientInfo)

This should be const void*, not const void *, to match WebKit coding style.

> Source/WebKit2/UIProcess/API/Cocoa/WKProcessGroup.mm:181
> +    WKProcessGroup *processGroup = (WKProcessGroup *)clientInfo;

Please use a static_cast here rather than a C-style cast.

> Source/WebKit2/UIProcess/API/Cocoa/WKProcessGroup.mm:186
> +    if ([downloadDelegate respondsToSelector:@selector(downloadDidStart:)]) {
> +        [downloadDelegate downloadDidStart:wrapper(*toImpl(download))];
> +    }

Brace style in WebKIt omits them on single line if statements, and while it’s OK to change that style rule some day I’m not sure we should change it today.

> Source/WebKit2/UIProcess/API/Cocoa/WKProcessGroup.mm:191
> +    WKProcessGroup *processGroup = (WKProcessGroup *)clientInfo;

Please use a static_cast here rather than a C-style cast.

> Source/WebKit2/UIProcess/API/Cocoa/WKProcessGroup.mm:195
> +        NSString *destination = [downloadDelegate download:wrapper(*toImpl(download)) decideDestinationWithSuggestedFilename:wrapper(*toImpl(filename)) allowOverwrite:(BOOL *)allowOverwrite];

The typecast from (bool *) to (BOOL *) is not safe here. Please don’t do that. Using a temporary BOOL and then assigning to the bool would be the safe idiom.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebDownload.h:30
> +#import <Foundation/Foundation.h>

Is this import really needed? I am pretty sure that it’s not.

Comment on attachment 225185 [details]
Fixed a build break for Mac.

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

>> Source/WebKit2/UIProcess/API/Cocoa/WKWebDownload.h:30
>> +#import <Foundation/Foundation.h>
> 
> Is this import really needed? I am pretty sure that it’s not.

Since this is a private header, it should be self-contained, so I think it’s correct to import Foundation.h here for NSObject.

[Yongjun Zhang]

(In reply to comment #4)
> (From update of attachment 225185 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225185&action=review
> 

Thanks for the comments! I will address these in another patch.

> review- because there are at least three things here that I think need to be fixed. (The leak because of lack of adoptCF, the BOOL * vs. bool * casting,

Will fix.
  
>and the way the releaseConnectionForDownload relates to the CFRelease call after it.)
> 
> > Source/WebKit2/Shared/Downloads/ios/DownloadIOS.mm:31
> > +#if USE(CFNETWORK)
> > +#import <CFNetwork/CFURLDownload.h>
> > +#endif
> 
> The correct WebKit project coding style is to put conditional imports in a separate paragraph after all the unconditional ones. But I don’t understand why this needs to be conditional? Is there someone who needs to compile DownloadIOS with USE(CFNETWORK) false? And if they do, is it really critical to avoid this include in that case? I suggest just a plain old import here with no conditional.

No, I don't think we will build iOS with USE(CFNETWORK) false, will remove the guards.

> 
> > Source/WebKit2/Shared/Downloads/ios/DownloadIOS.mm:39
> > +using namespace WTF;
> 
> This is not correct. WTF headers are supposed to export their symbols with a "using" in the main namespace, and you should never need WTF:: prefixes. If you do need them, the fix should be in WTF, not adding one of these "using namespace".

I will remove this.

> 
> > Source/WebKit2/Shared/Downloads/ios/DownloadIOS.mm:44
> > +// FIXME: It would be nice if these callbacks wouldn't have to be invoked on the main thread.
> 
> Would be nice if this comment explained why we need to do it rather than just saying it would be nice if we didn’t.
> 
> > Source/WebKit2/Shared/Downloads/ios/DownloadIOS.mm:47
> > +    if (RunLoop::isMain()) {
> 
> If we’re going to use RunLoop here, can we use the RunLoop version of "run on main thread" instead?
> 
> > Source/WebKit2/Shared/Downloads/ios/DownloadIOS.mm:56
> > +static void setUpDownloadClient(CFURLDownloadClient &client, Download *dl)
> 
> Incorrect formatting here. It should be CFURLDownloadClient& with the space after the "&". Also, I suggest a name like download instead of "dl" for the download argument. Since the download argument can never be null, I suggest a reference rather than a pointer.
> 
> > Source/WebKit2/Shared/Downloads/ios/DownloadIOS.mm:69
> > +    client.didStart = [](CFURLDownloadRef downloadRef, const void *clientInfo)
> > +    {
> > +        dispatchOnMainThread(^{
> > +            Download *download = (Download *)clientInfo;
> > +            download->didStart();
> > +        });
> > +    };
> 
> Why use clientInfo here to pass in the Download, when a lambda and a block can simply capture the pointer value instead. Avoiding the typecast would be really nice. 

I tried to capture 'download' in the lambda but the compiler complained it couldn't assign a lambda to the callback.  It seems like lambda with no capture can be converted to a function pointer; which doesn't seem to be the case for lambda with capture.

> Also, why not use lambdas for both rather than one lambda and one block? 

I will change the block to lambda as well.

> Same comments about the same idiom elsewhere in the patch. Also, I suggest omitting the argument names for unused arguments in the lambda.

Will do.

> 
> > Source/WebKit2/Shared/Downloads/ios/DownloadIOS.mm:78
> > +        notImplemented();
> 
> Is it really important to call the notImplemented function here? How will this help us in the future?

I was following the pattern for other unimplemented methods in this class.

> 
> > Source/WebKit2/Shared/Downloads/ios/DownloadIOS.mm:110
> > +        __block BOOL returnValue;
> > +        dispatchOnMainThread(^{
> > +            Download *download = (Download *)clientInfo;
> > +            returnValue = download->shouldDecodeSourceDataOfMIMEType(encodingType);
> > +        });
> > +
> > +        return returnValue;;
> 
> It seems a little strange to use a block here instead of a lambda.
> 
> > Source/WebKit2/Shared/Downloads/ios/DownloadIOS.mm:149
> > +            RetainPtr<NSData> resumeData = (NSData *)CFURLDownloadCopyResumeData(downloadRef);
> > +            IPC::DataReference dataReference(reinterpret_cast<const uint8_t*>([resumeData bytes]), [resumeData length]);
> 
> This leaks the resume data, because it omits the adoptCF call. Also, why convert the CFData to an NSData? CFDataGetBytePtr and CFDataGetLength would work fine without converting the type and even avoid the reinterpret cast, I believe. WebPage::getSelectionAsWebArchiveData and WebPage::getWebArchiveOfFrame have examples that do this correctly, and WebPage::drawPagesToPDF has an example I like even better that skips the local variable.

this is an oversight when I copied the impl from DownloadMac.mm. Will fix.

> 
> > Source/WebKit2/Shared/Downloads/ios/DownloadIOS.mm:170
> > +    CFURLConnectionRef connection = handle->connection();
> > +    m_download = CFURLDownloadCreateAndStartWithLoadingConnection(NULL, connection, m_request.cfURLRequest(UpdateHTTPBody), response.cfURLResponse(), &client);
> > +    handle->releaseConnectionForDownload();
> > +    CFRelease(connection);
> 
> This is a really awkward construction. It seems that ResourceHandle::releaseConnectionForDownload should return a RetainPtr of the connection. An explicit CFRelease like this seems both unclear and dangerous.

I agree. 

This pattern is copied from WebKit1 WebFrameLoaderClient:  https://trac.webkit.org/changeset/91198/trunk/Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm

If ResourceHandle::releaseConnectionForDownload returns RetainPtr then we won't need to call CFRelease.  It currently returns CFURLConnectionRef (not RetainPtr).  This could be misleading since all releaseConnectionForDownload does is clear the connection data member (a RetainPtr) by calling leadRef() in ResourceHandleCFNet.cpp:

    return d->m_connection.leakRef();

I will file another bug to track this.

> 
> Is m_download a RetainPtr? It worries me to see a Create here and not see the CFRelease that balances it. I suggest use of RetainPtr.

Yes it is RetainPtr in Download.h:

RetainPtr<CFURLDownloadRef> m_download;

I also realize I should call adoptCF for CFURLDownloadCreateAndStartWithLoadingConnection too.

> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKProcessGroup.mm:179
> > +static void didStart(WKContextRef context, WKDownloadRef download, const void *clientInfo)
> 
> This should be const void*, not const void *, to match WebKit coding style.
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKProcessGroup.mm:181
> > +    WKProcessGroup *processGroup = (WKProcessGroup *)clientInfo;
> 
> Please use a static_cast here rather than a C-style cast.
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKProcessGroup.mm:186
> > +    if ([downloadDelegate respondsToSelector:@selector(downloadDidStart:)]) {
> > +        [downloadDelegate downloadDidStart:wrapper(*toImpl(download))];
> > +    }
> 
> Brace style in WebKIt omits them on single line if statements, and while it’s OK to change that style rule some day I’m not sure we should change it today.
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKProcessGroup.mm:191
> > +    WKProcessGroup *processGroup = (WKProcessGroup *)clientInfo;
> 
> Please use a static_cast here rather than a C-style cast.
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKProcessGroup.mm:195
> > +        NSString *destination = [downloadDelegate download:wrapper(*toImpl(download)) decideDestinationWithSuggestedFilename:wrapper(*toImpl(filename)) allowOverwrite:(BOOL *)allowOverwrite];
> 
> The typecast from (bool *) to (BOOL *) is not safe here. Please don’t do that. Using a temporary BOOL and then assigning to the bool would be the safe idiom.

Will fix.

> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKWebDownload.h:30
> > +#import <Foundation/Foundation.h>
> 
> Is this import really needed? I am pretty sure that it’s not.

[Yongjun Zhang]

Created attachment 225499 [details]
Revised patch to address review comment.

Main changes:

1) Fixed leak for resumeData in didFail; and adoptCF for CFURLDownloadRef created on Download::startWithHandle.
2) Use a temporary variable BOOL instead of casting (bool*) to (BOOL*) in decideDestinationWithSuggestedFilename.
3) Changed to use lambda for blocks passing into dispatchOnMainThread.
4) Some style fixes pointed out in reviewing comments.

[Anders Carlsson]

Comment on attachment 225499 [details]
Revised patch to address review comment.

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

> Source/WebKit2/UIProcess/API/Cocoa/WKProcessGroup.h:45
> +@protocol WKDownloadDelegate <NSObject>
> +@optional
> +
> +- (void)downloadDidStart:(WKWebDownload *)download;
> +- (NSString *)download:(WKWebDownload *)download decideDestinationWithSuggestedFilename:(NSString *)filename allowOverwrite:(BOOL *)allowOverwrite;
> +- (void)downloadDidFinish:(WKWebDownload *)download;
> +- (void)download:(WKWebDownload *)download didReceiveResponse:(NSURLResponse *)response;
> +- (void)download:(WKWebDownload *)download didReceiveData:(uint64_t)length;
> +
> +@end

This should not be here. It should probably be in WKProcessPoolPrivate, and named accordingly.

> Source/WebKit2/UIProcess/API/Cocoa/WKProcessGroup.h:67
> +@property (assign) id <WKDownloadDelegate> downloadDelegate;

This should be on WKProcessPoolPrivate (and named accordingly). It should also be weak.

> Source/WebKit2/UIProcess/API/Cocoa/WKProcessGroup.mm:244
> +static void setUpDownloadClient(WKProcessGroup *processGroup, WKContextRef contextRef)
> +{
> +    WKContextDownloadClientV0 downloadClient;
> +    memset(&downloadClient, 0, sizeof(downloadClient));
> +
> +    downloadClient.base.version = 0;
> +    downloadClient.base.clientInfo = processGroup;
> +    downloadClient.didStart = didStart;
> +    downloadClient.decideDestinationWithSuggestedFilename = decideDestinationWithSuggestedFilename;
> +    downloadClient.didFinish = didFinish;
> +    downloadClient.didReceiveResponse = didReceiveResponse;
> +    downloadClient.didReceiveData = didReceiveData;
> +
> +    WKContextSetDownloadClient(contextRef, &downloadClient.base);
> +}

This is also wrong. You should create an APIDownloadClient subclass, see what we've done for APILoaderClient and APIPolicyClient for example.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebDownload.h:33
> +@interface WKWebDownload : NSObject

Should be SPI and marked accordingly.

[Yongjun Zhang]

For API::DownloadClient, I filed: https://bugs.webkit.org/show_bug.cgi?id=130484

[Andy Estes]

(In reply to comment #4)
> (From update of attachment 225185 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225185&action=review
> 
> > Source/WebKit2/Shared/Downloads/ios/DownloadIOS.mm:47
> > +    if (RunLoop::isMain()) {
> 
> If we’re going to use RunLoop here, can we use the RunLoop version of "run on main thread" instead?

As far as I can tell, RunLoop only supports asynchronous dispatch to the main thread.

[Andy Estes]

(In reply to comment #4)
> (From update of attachment 225185 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225185&action=review
> 
> > Source/WebKit2/Shared/Downloads/ios/DownloadIOS.mm:110
> > +        __block BOOL returnValue;
> > +        dispatchOnMainThread(^{
> > +            Download *download = (Download *)clientInfo;
> > +            returnValue = download->shouldDecodeSourceDataOfMIMEType(encodingType);
> > +        });
> > +
> > +        return returnValue;;
> 
> It seems a little strange to use a block here instead of a lambda.

Since we're ultimately invoking a block in dispatch_sync(), I think it makes sense to use a block instead of always converting from a lambda.

[Andy Estes]

(In reply to comment #6)
> (In reply to comment #4)
> > (From update of attachment 225185 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=225185&action=review
> > 
> > > Source/WebKit2/Shared/Downloads/ios/DownloadIOS.mm:170
> > > +    CFURLConnectionRef connection = handle->connection();
> > > +    m_download = CFURLDownloadCreateAndStartWithLoadingConnection(NULL, connection, m_request.cfURLRequest(UpdateHTTPBody), response.cfURLResponse(), &client);
> > > +    handle->releaseConnectionForDownload();
> > > +    CFRelease(connection);
> > 
> > This is a really awkward construction. It seems that ResourceHandle::releaseConnectionForDownload should return a RetainPtr of the connection. An explicit CFRelease like this seems both unclear and dangerous.
> 
> I agree. 
> 
> This pattern is copied from WebKit1 WebFrameLoaderClient:  https://trac.webkit.org/changeset/91198/trunk/Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm
> 
> If ResourceHandle::releaseConnectionForDownload returns RetainPtr then we won't need to call CFRelease.  It currently returns CFURLConnectionRef (not RetainPtr).  This could be misleading since all releaseConnectionForDownload does is clear the connection data member (a RetainPtr) by calling leadRef() in ResourceHandleCFNet.cpp:
> 
>     return d->m_connection.leakRef();
> 
> I will file another bug to track this.

I'm going to fix this as part of this bug.

[Andy Estes]

Created attachment 227570 [details]
[iOS] Download support by CFURLDownloadRef under USE(CFNETWORK).

[WebKit Commit Bot]

Attachment 227570 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKProcessPoolPrivate.h:45:  Missing spaces around =  [whitespace/operators] [4]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andy Estes]

Comment on attachment 227570 [details]
[iOS] Download support by CFURLDownloadRef under USE(CFNETWORK).

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

> Source/WebKit2/ChangeLog:42
> [phone]  Yongjun Zhang  <[e-mail]>
> +
> +        [iOS] Download support by CFURLDownloadRef under USE(CFNETWORK)
> +        https://bugs.webkit.org/show_bug.cgi?id=129322
> +
> +        Enable download support for WK2 by using CFURLDownload API.
> +
> +        Reviewed by NOBODY (OOPS!).

Oops, I meant to merge this into my ChangeLog entry.

[Anders Carlsson]

Comment on attachment 227570 [details]
[iOS] Download support by CFURLDownloadRef under USE(CFNETWORK).

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 =

[Andy Estes]

Committed r166186: <http://trac.webkit.org/changeset/166186>