Bug 53282

Summary: Downloads in WK2 on Windows should write resume data to bundle
Product: WebKit Reporter: Jon Honeycutt <jhoneycutt>
Component: WebKit2Assignee: Jon Honeycutt <jhoneycutt>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, webkit.review.bot
Priority: P2 Keywords: InRadar, PlatformOnly
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows 7   
Attachments:
Description Flags
Patch
none
Patch v2 alice.barraclough: review+

Description Jon Honeycutt 2011-01-27 20:08:24 PST
In WK1, the WebDownload class is responsible for writing resume data to the download bundle. WK2 should do the same.

<rdar://problem/8753077>
Comment 1 Jon Honeycutt 2011-01-27 20:19:38 PST
Created attachment 80409 [details]
Patch
Comment 2 WebKit Review Bot 2011-01-27 20:22:07 PST
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.
Comment 3 Build Bot 2011-01-27 20:56:51 PST
Attachment 80409 [details] did not build on win:
Build output: http://queues.webkit.org/results/7629274
Comment 4 Jon Honeycutt 2011-01-28 12:09:17 PST
Created attachment 80477 [details]
Patch v2
Comment 5 WebKit Review Bot 2011-01-28 12:12:24 PST
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.
Comment 6 Alice Liu 2011-01-28 12:46:55 PST
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.
Comment 7 Jon Honeycutt 2011-01-29 00:29:46 PST
(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!
Comment 8 Jon Honeycutt 2011-01-29 00:36:04 PST
Landed in r77055.