Bug 137507 – WKContext needs to provide an API to resume a download

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

[Jeff Miller]

We currently have an API to start a new download:

WK_EXPORT WKDownloadRef WKContextDownloadURLRequest(WKContextRef context, const WKURLRequestRef request);

but there's no API to resume a partially completed download. Presumably, such an API would take the data returned by WKDownloadGetResumeData() as well as a path which can be used by NSURLDownload.

[Radar WebKit Bug Importer]

<rdar://problem/18576259>

[Jeff Miller]

Created attachment 239494 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 239494 [details]
Patch

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

> Source/WebKit2/Shared/Downloads/mac/DownloadMac.mm:99
> +    m_nsURLDownload = [[NSURLDownload alloc] initWithResumeData:nsData.get() delegate:m_delegate.get() path:path];

Nit: Needs adoptNS(...) here.

[Conrad Shultz]

Comment on attachment 239494 [details]
Patch

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

> Source/WebKit2/WebProcess/WebProcess.messages.in:64
> +    # three messages.

There are other messages files with fewer messages; since you're having to change the comment I wonder whether this would be an opportune time to spin off a Download.messages.in.

[Jeff Miller]

Comment on attachment 239494 [details]
Patch

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

>> Source/WebKit2/Shared/Downloads/mac/DownloadMac.mm:99
>> +    m_nsURLDownload = [[NSURLDownload alloc] initWithResumeData:nsData.get() delegate:m_delegate.get() path:path];
> 
> Nit: Needs adoptNS(...) here.

I'll fix this.

>> Source/WebKit2/WebProcess/WebProcess.messages.in:64
>> +    # three messages.
> 
> There are other messages files with fewer messages; since you're having to change the comment I wonder whether this would be an opportune time to spin off a Download.messages.in.

Fair point. I'll do that with a separate bug.

[Jeff Miller]

Comment on attachment 239494 [details]
Patch

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

>>> Source/WebKit2/WebProcess/WebProcess.messages.in:64
>>> +    # three messages.
>> 
>> There are other messages files with fewer messages; since you're having to change the comment I wonder whether this would be an opportune time to spin off a Download.messages.in.
> 
> Fair point. I'll do that with a separate bug.

I filed bug 137744 to track this.

[Jeff Miller]

Created attachment 239891 [details]
Patch

[Anders Carlsson]

Comment on attachment 239494 [details]
Patch

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

> Source/WebKit2/Shared/Downloads/mac/DownloadMac.mm:98
> +    RetainPtr<NSData> nsData = adoptNS([[NSData alloc] initWithBytes:resumeData.data() length:resumeData.size()]);

Can use auto nsData =

> Source/WebKit2/UIProcess/API/C/WKContext.cpp:285
> +WKDownloadRef WKContextResumeDownload(WKContextRef contextRef, WKDataRef resumeData, WKStringRef path)

I'm not in love with this name, since it reads like a getter. I don't have a better idea though.

[Jeff Miller]

Comment on attachment 239494 [details]
Patch

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

>> Source/WebKit2/Shared/Downloads/mac/DownloadMac.mm:98
>> +    RetainPtr<NSData> nsData = adoptNS([[NSData alloc] initWithBytes:resumeData.data() length:resumeData.size()]);
> 
> Can use auto nsData =

Fixed.

>> Source/WebKit2/UIProcess/API/C/WKContext.cpp:285
>> +WKDownloadRef WKContextResumeDownload(WKContextRef contextRef, WKDataRef resumeData, WKStringRef path)
> 
> I'm not in love with this name, since it reads like a getter. I don't have a better idea though.

I don't, either.

[Jeff Miller]

Created attachment 240135 [details]
Patch

[Darin Adler]

Comment on attachment 240135 [details]
Patch

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

> Source/WebKit2/NetworkProcess/NetworkProcess.h:36
> +#include "SandboxExtension.h"

Not needed, because DownloadManager.h now includes this header too.

> Source/WebKit2/Shared/Downloads/mac/DownloadMac.mm:102
> +    auto nsData = adoptNS([[NSData alloc] initWithBytes:resumeData.data() length:resumeData.size()]);

Is this always how we turn an IPC::DataReference into an NSData? I would have expected a more streamlined idiom.

> Source/WebKit2/Shared/Downloads/mac/DownloadMac.mm:105
> +    m_request = ResourceRequest([m_nsURLDownload request]);

Is the explicit constructor needed here?

> Source/WebKit2/UIProcess/API/C/WKContext.h:113
>  WK_EXPORT WKDownloadRef WKContextDownloadURLRequest(WKContextRef context, const WKURLRequestRef request);

Not new to this patch: This const is a mistake.

> Source/WebKit2/UIProcess/WebContext.cpp:908
> +

Should leave out this blank line, I think.

[Jeff Miller]

Comment on attachment 240135 [details]
Patch

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

>> Source/WebKit2/NetworkProcess/NetworkProcess.h:36
>> +#include "SandboxExtension.h"
> 
> Not needed, because DownloadManager.h now includes this header too.

Removed.

>> Source/WebKit2/Shared/Downloads/mac/DownloadMac.mm:102
>> +    auto nsData = adoptNS([[NSData alloc] initWithBytes:resumeData.data() length:resumeData.size()]);
> 
> Is this always how we turn an IPC::DataReference into an NSData? I would have expected a more streamlined idiom.

As far as I can tell, yes. I see this idiom used in other places.

>> Source/WebKit2/Shared/Downloads/mac/DownloadMac.mm:105
>> +    m_request = ResourceRequest([m_nsURLDownload request]);
> 
> Is the explicit constructor needed here?

It isn't. I'll remove the explicit constructor.

>> Source/WebKit2/UIProcess/API/C/WKContext.h:113
>>  WK_EXPORT WKDownloadRef WKContextDownloadURLRequest(WKContextRef context, const WKURLRequestRef request);
> 
> Not new to this patch: This const is a mistake.

I'll fix this in a separate patch.

>> Source/WebKit2/UIProcess/WebContext.cpp:908
>> +
> 
> Should leave out this blank line, I think.

Done.

[Jeff Miller]

Committed r174987: <http://trac.webkit.org/changeset/174987>