Bug 154473 - Implement downloads with NetworkSession
Summary: Implement downloads with NetworkSession
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
Depends on:
Reported: 2016-02-19 14:39 PST by Alex Christensen
Modified: 2016-03-25 10:12 PDT (History)
1 user (show)

See Also:

Patch (19.21 KB, patch)
2016-02-19 14:39 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (33.87 KB, patch)
2016-02-23 10:20 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (35.28 KB, patch)
2016-02-23 11:01 PST, Alex Christensen
beidson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2016-02-19 14:39:16 PST
Implement downloads with NetworkSession
Comment 1 Alex Christensen 2016-02-19 14:39:36 PST
Created attachment 271804 [details]
Comment 2 Alex Christensen 2016-02-19 14:40:00 PST
Comment on attachment 271804 [details]

first patch is WIP.  Something is still wrong :(
Comment 3 Alex Christensen 2016-02-23 10:20:25 PST
Created attachment 272031 [details]
Comment 4 Brady Eidson 2016-02-23 10:42:17 PST
Comment on attachment 272031 [details]

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

Mostly looks fine, but there's a bit of an anti-pattern here I think should be cleaned up instead of landed as-is

> Source/WebKit2/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:128
> -    client().willPerformHTTPRedirection(redirectResponse, request, completionHandler);
> +    if (client())
> +        client()->willPerformHTTPRedirection(redirectResponse, request, completionHandler);

Inside NetworkDataTask, you can access m_client directly instead of calling client(), right?

> Source/WebKit2/NetworkProcess/cocoa/NetworkSessionCocoa.mm:104
> +    if (auto* networkDataTask = _session->dataTaskForIdentifier(task.taskIdentifier)) {
> +        if (auto* client = networkDataTask->client())
> +            client->didSendData(totalBytesSent, totalBytesExpectedToSend);
> +    }

This is a pattern repeated multiple times in this patch - The URLSession delegate null checking the data task's client.

Instead, I think it should just always call a method in NetworkDataTask, which then null checks its own client. e.g.
if (auto* networkDataTask = _session->dataTaskForIdentifier(task.taskIdentifier))
    networkDataTask->client().didSendData(totalBytesSent, totalBytesExpectedToSend);


if (auto* networkDataTask = _session->dataTaskForIdentifier(task.taskIdentifier))
    networkDataTask->didSendData(totalBytesSent, totalBytesExpectedToSend);

And then NetworkDataTask::didSendData(...) just null checks m_client at the start.

That would make a lot of code easier to read (and write!)
Comment 5 Alex Christensen 2016-02-23 11:01:03 PST
Created attachment 272034 [details]
Comment 6 Alex Christensen 2016-02-23 11:13:32 PST
Comment 7 Alex Christensen 2016-03-25 10:12:21 PDT
Removed unused capture in r198672.