Bug 177625 - [WK2][NetworkSession] Add support for resuming downloads
Summary: [WK2][NetworkSession] Add support for resuming downloads
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on: 177667 183134
Blocks:
  Show dependency treegraph
 
Reported: 2017-09-28 15:02 PDT by Chris Dumez
Modified: 2018-02-26 10:25 PST (History)
7 users (show)

See Also:


Attachments
WIP Patch (13.49 KB, patch)
2017-09-28 15:03 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (13.82 KB, patch)
2017-09-28 15:38 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (14.15 KB, patch)
2017-09-28 15:53 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (14.27 KB, patch)
2017-09-28 16:05 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (14.21 KB, patch)
2017-09-28 16:19 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (13.93 KB, patch)
2017-09-28 16:33 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (14.62 KB, patch)
2017-09-29 15:54 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (13.87 KB, patch)
2017-09-29 16:04 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (13.92 KB, patch)
2017-10-03 15:55 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (16.23 KB, patch)
2017-10-04 14:07 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (16.53 KB, patch)
2017-10-05 09:09 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2017-09-28 15:02:41 PDT
Add support for resuming downloads for the WK2 NETWORK_SESSION code path.
Comment 1 Chris Dumez 2017-09-28 15:02:57 PDT
<rdar://problem/34345975>
Comment 2 Chris Dumez 2017-09-28 15:03:55 PDT
Created attachment 322122 [details]
WIP Patch
Comment 3 Build Bot 2017-09-28 15:06:05 PDT
Attachment 322122 [details] did not pass style-queue:


ERROR: Source/WebKit/NetworkProcess/Downloads/cocoa/DownloadCocoa.mm:44:  No space between ^ and block definition.  [whitespace/brackets] [4]
ERROR: Source/WebKit/NetworkProcess/Downloads/cocoa/DownloadCocoa.mm:45:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Chris Dumez 2017-09-28 15:38:28 PDT
Created attachment 322129 [details]
WIP Patch
Comment 5 Build Bot 2017-09-28 15:41:16 PDT
Attachment 322129 [details] did not pass style-queue:


ERROR: Source/WebKit/NetworkProcess/Downloads/cocoa/DownloadCocoa.mm:44:  No space between ^ and block definition.  [whitespace/brackets] [4]
ERROR: Source/WebKit/NetworkProcess/Downloads/cocoa/DownloadCocoa.mm:45:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Chris Dumez 2017-09-28 15:53:56 PDT
Created attachment 322132 [details]
WIP Patch
Comment 7 Chris Dumez 2017-09-28 16:05:33 PDT
Created attachment 322134 [details]
WIP Patch
Comment 8 Chris Dumez 2017-09-28 16:19:57 PDT
Created attachment 322138 [details]
WIP Patch
Comment 9 Chris Dumez 2017-09-28 16:33:29 PDT
Created attachment 322141 [details]
WIP Patch
Comment 10 Alex Christensen 2017-09-28 17:03:11 PDT
See rdar://problem/23041906
Comment 11 Chris Dumez 2017-09-29 15:54:00 PDT
Created attachment 322248 [details]
WIP Patch
Comment 12 Chris Dumez 2017-09-29 16:04:37 PDT
Created attachment 322250 [details]
WIP Patch
Comment 13 Chris Dumez 2017-10-03 15:55:56 PDT
Created attachment 322600 [details]
WIP Patch
Comment 14 Chris Dumez 2017-10-04 14:07:07 PDT
Created attachment 322726 [details]
Patch
Comment 15 Daniel Bates 2017-10-04 23:44:16 PDT
Comment on attachment 322726 [details]
Patch

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

> Source/WebKit/ChangeLog:13
> +        when cancelling an download over non-HTTP.

an => a

> Source/WebKit/ChangeLog:23
> +        (WebKit::Download::resume):

Please mention the workaround here.

> Source/WebKit/NetworkProcess/Downloads/cocoa/DownloadCocoa.mm:46
> +        WTFLogAlways("Could not find network session with given sessionID");

sessionID => session ID

Would it be helpful to include the value of m_sessionID in the logged message?

> Source/WebKit/NetworkProcess/Downloads/cocoa/DownloadCocoa.mm:53
> +    NSMutableDictionary *dictionary = [NSPropertyListSerialization propertyListWithData:nsData.get() options:NSPropertyListImmutable format:0 error:NULL];

NULL => nullptr 

(Since this is Objective-C++)

> Source/WebKit/NetworkProcess/Downloads/cocoa/DownloadCocoa.mm:54
> +    [dictionary setObject:(NSString *)path forKey:@"NSURLSessionResumeInfoLocalPath"];

static_cast<NSString*>(...)

(Since this is Objective-C++)

> Source/WebKit/NetworkProcess/Downloads/cocoa/DownloadCocoa.mm:55
> +    NSData *updatedData = [NSPropertyListSerialization dataWithPropertyList:dictionary format:NSPropertyListXMLFormat_v1_0 options:0 error:NULL];

NULL => nullptr

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.h:63
> +    NSURLSessionDownloadTask *downloadTaskWithResumeData(NSData *);

* is on the wrong side.

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:279
> +                WebCore::AuthenticationChallenge authenticationChallenge(challenge);

Uniform initializer syntax?

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:677
> +NSURLSessionDownloadTask *NetworkSessionCocoa::downloadTaskWithResumeData(NSData *resumeData)

*s are on the wrong side since this is a C++ member function.
Comment 16 Chris Dumez 2017-10-05 09:09:44 PDT
Created attachment 322842 [details]
Patch
Comment 17 Chris Dumez 2017-10-05 09:10:42 PDT
(In reply to Daniel Bates from comment #15)
> Comment on attachment 322726 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=322726&action=review
> 
> > Source/WebKit/ChangeLog:13
> > +        when cancelling an download over non-HTTP.
> 
> an => a
> 
> > Source/WebKit/ChangeLog:23
> > +        (WebKit::Download::resume):
> 
> Please mention the workaround here.
> 
> > Source/WebKit/NetworkProcess/Downloads/cocoa/DownloadCocoa.mm:46
> > +        WTFLogAlways("Could not find network session with given sessionID");
> 
> sessionID => session ID
> 
> Would it be helpful to include the value of m_sessionID in the logged
> message?
> 
> > Source/WebKit/NetworkProcess/Downloads/cocoa/DownloadCocoa.mm:53
> > +    NSMutableDictionary *dictionary = [NSPropertyListSerialization propertyListWithData:nsData.get() options:NSPropertyListImmutable format:0 error:NULL];
> 
> NULL => nullptr 
> 
> (Since this is Objective-C++)
> 
> > Source/WebKit/NetworkProcess/Downloads/cocoa/DownloadCocoa.mm:54
> > +    [dictionary setObject:(NSString *)path forKey:@"NSURLSessionResumeInfoLocalPath"];
> 
> static_cast<NSString*>(...)
> 
> (Since this is Objective-C++)
> 
> > Source/WebKit/NetworkProcess/Downloads/cocoa/DownloadCocoa.mm:55
> > +    NSData *updatedData = [NSPropertyListSerialization dataWithPropertyList:dictionary format:NSPropertyListXMLFormat_v1_0 options:0 error:NULL];
> 
> NULL => nullptr
> 
> > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.h:63
> > +    NSURLSessionDownloadTask *downloadTaskWithResumeData(NSData *);
> 
> * is on the wrong side.
> 
> > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:279
> > +                WebCore::AuthenticationChallenge authenticationChallenge(challenge);
> 
> Uniform initializer syntax?
> 
> > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:677
> > +NSURLSessionDownloadTask *NetworkSessionCocoa::downloadTaskWithResumeData(NSData *resumeData)
> 
> *s are on the wrong side since this is a C++ member function.

Thanks, I merged all review comments.
Comment 18 Chris Dumez 2017-10-16 12:31:52 PDT
Alex, ping review?
Comment 19 WebKit Commit Bot 2017-10-16 13:29:39 PDT
Comment on attachment 322842 [details]
Patch

Clearing flags on attachment: 322842

Committed r223431: <https://trac.webkit.org/changeset/223431>
Comment 20 WebKit Commit Bot 2017-10-16 13:29:40 PDT
All reviewed patches have been landed.  Closing bug.