[Webkit-unassigned] [Bug 129322] [iOS] Download support by CFURLDownloadRef under USE(CFNETWORK).
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Feb 27 13:05:30 PST 2014
https://bugs.webkit.org/show_bug.cgi?id=129322
--- Comment #6 from Yongjun Zhang <yongjun_zhang at apple.com> 2014-02-27 13:02:36 PST ---
(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.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list