Bug 48447 – Begin stubbing out the Download class

• Component: New Bugs
• Status: RESOLVED FIXED

[Anders Carlsson]

Begin stubbing out the Download class

[Anders Carlsson]

Created attachment 72061 [details]
Patch

[WebKit Review Bot]

Attachment 72061 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit2/WebProcess/Downloads/qt/DownloadQt.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit2/WebProcess/Downloads/DownloadManager.h:32:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebKit2/WebProcess/Downloads/cf/DownloadCFNet.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit2/WebProcess/Downloads/Download.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 4 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Roben (:aroben)]

Comment on attachment 72061 [details]
Patch

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
> +@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.

[Anders Carlsson]

(In reply to comment #3)
> (From update of attachment 72061 [details])
> 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.
> 

Yup, I was going to fix that when I add the first callback handler.

> > 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!

Yup, I didn't see a reason for keeping them when I'm just adding a stubbed out function in each file.

> 
> > 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?

For now it is :)

> 
> > WebKit2/WebProcess/Downloads/mac/DownloadMac.mm:31
> > +@interface WKDownloadAsDelegate : NSObject <NSURLConnectionDelegate> {
> > +    WebKit::Download* m_download;
> 
> I thought we just used _ for ivars.

Copied from ResourceHandleMac, but I can change it.

> 
> > 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).
> 

Yeah it should be called from the destructor eventually (again, this is just stubbing out the code).

> > WebKit2/WebProcess/Downloads/mac/DownloadMac.mm:72
> > +    NSLog(@"download did begin!");
> 
> Did you mean to leave this in here?

No

> 
> > WebKit2/win/WebKit2.vcproj:1264
> > +    				<File
> > +    					RelativePath="..\WebProcess\Downloads\cf\DownloadCFNet.h"
> > +    					>
> > +    				</File>
> 
> I don't think this file exists.

Good catch. I'll upload a new patch shortly!

[Anders Carlsson]

Created attachment 72070 [details]
Patch

[WebKit Review Bot]

Attachment 72070 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit2/WebProcess/Downloads/qt/DownloadQt.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit2/WebProcess/Downloads/DownloadManager.h:32:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebKit2/WebProcess/Downloads/cf/DownloadCFNet.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit2/WebProcess/Downloads/Download.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 4 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Anders Carlsson]

Committed r70687: <http://trac.webkit.org/changeset/70687>

[WebKit Review Bot]

http://trac.webkit.org/changeset/70687 might have broken GTK Linux 64-bit Debug