Bug 53282 – Downloads in WK2 on Windows should write resume data to bundle

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Jon Honeycutt]

In WK1, the WebDownload class is responsible for writing resume data to the download bundle. WK2 should do the same.

<rdar://problem/8753077>

[Jon Honeycutt]

Created attachment 80409 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/network/cf/DownloadBundle.h:30:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebCore/platform/network/win/DownloadBundleWin.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/network/win/DownloadBundleWin.cpp:43:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/network/win/DownloadBundleWin.cpp:48:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/network/win/DownloadBundleWin.cpp:49:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 5 in 14 files


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

[Build Bot]

Attachment 80409 [details] did not build on win:
Build output: http://queues.webkit.org/results/7629274

[Jon Honeycutt]

Created attachment 80477 [details]
Patch v2

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/network/cf/DownloadBundle.h:30:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 14 files


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

[Alice Liu]

Comment on attachment 80477 [details]
Patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=80477&action=review

> Source/WebKit/win/WebDownloadCFNet.cpp:253
> +    resumeData.adoptCF(CFURLDownloadCopyResumeData(m_download.get()));

awesome

> Source/WebKit2/WebProcess/Downloads/cf/DownloadCFNet.cpp:171
> +}

do you need to null-check download?

> Source/WebKit2/WebProcess/Downloads/cf/DownloadCFNet.cpp:179
> +    download->didCreateDestination(download->destination());

do you need to null-check download?

> Source/WebKit2/WebProcess/Downloads/curl/DownloadCurl.cpp:58
> +}

if there's nothing that you know we need to do, you can leave this out. but if we do need to do something, please comment at least briefly about it.

> Source/WebKit2/WebProcess/Downloads/mac/DownloadMac.mm:174
> +

presumably intentionally empty, and doesn't need notImplemented(), i assume.

> Source/WebKit2/WebProcess/Downloads/qt/DownloadQt.cpp:57
> +}

if there's nothing that you know we need to do, you can leave this out. but if we do need to do something, please comment at least briefly about it.

[Jon Honeycutt]

(In reply to comment #6)
> (From update of attachment 80477 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80477&action=review
> 
> > Source/WebKit/win/WebDownloadCFNet.cpp:253
> > +    resumeData.adoptCF(CFURLDownloadCopyResumeData(m_download.get()));
> 
> awesome
> 
> > Source/WebKit2/WebProcess/Downloads/cf/DownloadCFNet.cpp:171
> > +}
> 
> do you need to null-check download?
> 
> > Source/WebKit2/WebProcess/Downloads/cf/DownloadCFNet.cpp:179
> > +    download->didCreateDestination(download->destination());
> 
> do you need to null-check download?

None of the callback functions null check download, so I don't think so.

> 
> > Source/WebKit2/WebProcess/Downloads/curl/DownloadCurl.cpp:58
> > +}
> 
> if there's nothing that you know we need to do, you can leave this out. but if we do need to do something, please comment at least briefly about it.

I'm not sure whether other platforms will use this function, but it needs a definition. I'm going to leave the calls to notImplemented() and let someone remove them when downloads are implemented for this platform.

> 
> > Source/WebKit2/WebProcess/Downloads/mac/DownloadMac.mm:174
> > +
> 
> presumably intentionally empty, and doesn't need notImplemented(), i assume.

That's right.

> 
> > Source/WebKit2/WebProcess/Downloads/qt/DownloadQt.cpp:57
> > +}
> 
> if there's nothing that you know we need to do, you can leave this out. but if we do need to do something, please comment at least briefly about it.

Same response as earlier.

Thanks for the review!

[Jon Honeycutt]

Landed in r77055.