Summary: | WKContext needs to provide an API to resume a download | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jeff Miller <jeffm> | ||||||||
Component: | WebKit2 | Assignee: | Jeff Miller <jeffm> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | andersca, jeffm, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Mac | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Jeff Miller
2014-10-07 16:41:24 PDT
Created attachment 239494 [details]
Patch
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. 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. 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. 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. Created attachment 239891 [details]
Patch
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. 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. Created attachment 240135 [details]
Patch
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. 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. Committed r174987: <http://trac.webkit.org/changeset/174987> |