[webkit-reviews] review denied: [Bug 48447] Begin stubbing out the Download class : [Attachment 72061] Patch

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has denied Anders Carlsson
<andersca at apple.com>'s request for review:
Bug 48447: Begin stubbing out the Download class
https://bugs.webkit.org/show_bug.cgi?id=48447

Attachment 72061: Patch
https://bugs.webkit.org/attachment.cgi?id=72061&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=72061&action=review

This seems certain to crash if the delegate ever calls back into the Download.

> WebKit2/ChangeLog:25
> +	   * WebProcess/Downloads/mac/DownloadMac.mm: Added.
> +	   (WebKit::Download::start):
> +	   Create an NSURLDownload.
> +    

You seem to have left DownloadCFNet and DownloadQt out of your ChangeLog!

> WebKit2/WebProcess/Downloads/DownloadManager.cpp:49
> +void DownloadManager::startDownload(uint64_t downloadID, const
ResourceRequest& request)
> +{
> +    OwnPtr<Download> download = Download::create(downloadID, request);
> +    download->start();
> +}

Is it OK that the download gets deleted right after start() returns?

> WebKit2/WebProcess/Downloads/mac/DownloadMac.mm:31
> + at interface WKDownloadAsDelegate : NSObject <NSURLConnectionDelegate> {
> +    WebKit::Download* m_download;

I thought we just used _ for ivars.

> WebKit2/WebProcess/Downloads/mac/DownloadMac.mm:68
> +- (void)invalidate
> +{
> +    m_download = 0;
> +}

Who calls this? It seems like Download should call it in its destructor (if not
earlier).

> WebKit2/WebProcess/Downloads/mac/DownloadMac.mm:72
> +    NSLog(@"download did begin!");

Did you mean to leave this in here?

> WebKit2/win/WebKit2.vcproj:1264
> +				<File
> +				       
RelativePath="..\WebProcess\Downloads\cf\DownloadCFNet.h"
> +					>
> +				</File>

I don't think this file exists.