[webkit-reviews] review denied: [Bug 129322] [iOS] Download support by CFURLDownloadRef under USE(CFNETWORK). : [Attachment 225185] Fixed a build break for Mac.

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Yongjun Zhang
<yongjun_zhang 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 225185: Fixed a build break for Mac.
https://bugs.webkit.org/attachment.cgi?id=225185&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.