[webkit-reviews] review denied: [Bug 129322] [iOS] Download support by CFURLDownloadRef under USE(CFNETWORK). : [Attachment 225499] Revised patch to address review comment.

[bugzilla-daemon at webkit.org]

Anders Carlsson <andersca 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 225499: Revised patch to address review comment.
https://bugs.webkit.org/attachment.cgi?id=225499&action=review

------- Additional Comments from Anders Carlsson <andersca at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=225499&action=review


> Source/WebKit2/UIProcess/API/Cocoa/WKProcessGroup.h:45
> + at protocol WKDownloadDelegate <NSObject>
> + at 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;
> +
> + at end

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

> Source/WebKit2/UIProcess/API/Cocoa/WKProcessGroup.h:67
> + at 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
> + at interface WKWebDownload : NSObject

Should be SPI and marked accordingly.