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

Wed Feb 26 09:44:06 PST 2014

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

Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #225185|review?                     |review-
               Flag|                            |

--- Comment #4 from Darin Adler <darin at apple.com>  2014-02-26 09:41:12 PST ---
(From update of attachment 225185)
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.

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