Bug 173557 - [Curl] Extract CurlDownloadManager as shared background task handler
Summary: [Curl] Extract CurlDownloadManager as shared background task handler
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Basuke Suzuki
Depends on:
Blocks: 117300 173629
  Show dependency treegraph
Reported: 2017-06-19 11:34 PDT by Basuke Suzuki
Modified: 2017-06-23 11:38 PDT (History)
10 users (show)

See Also:

Patch (37.75 KB, patch)
2017-06-19 15:17 PDT, Basuke Suzuki
no flags Details | Formatted Diff | Diff
Fix (34.99 KB, patch)
2017-06-20 09:39 PDT, Basuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (34.68 KB, patch)
2017-06-21 09:48 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Fix build error (1.10 KB, patch)
2017-06-22 13:26 PDT, Basuke Suzuki
joepeck: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Basuke Suzuki 2017-06-19 11:34:34 PDT
The CurlDownload implementation includes CurlDownloadManager as a companion class for multiple download management. The class can be used from resource handle, so it should be separated from CurlDownload and to be prepare for further usage.
Comment 1 Basuke Suzuki 2017-06-19 15:17:17 PDT
Created attachment 313335 [details]
Comment 2 Build Bot 2017-06-19 15:18:46 PDT
Attachment 313335 [details] did not pass style-queue:

ERROR: Source/WebCore/ChangeLog:86:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
ERROR: Source/WebCore/platform/network/curl/CookieJarCurl.cpp:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
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 Basuke Suzuki 2017-06-19 15:28:03 PDT
I didn't fix this style error because I've just change the header list on that file.
Comment 5 Basuke Suzuki 2017-06-20 09:39:51 PDT
Created attachment 313408 [details]

Remove duplicated ChangeLog
Borrowed copyright from sibling source file
Remove unused interface.
Comment 6 Alex Christensen 2017-06-21 09:46:26 PDT
Comment on attachment 313408 [details]

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

> Source/WebCore/platform/network/curl/CookieJarCurl.cpp:252
> +    CURLSH* curlsh = CurlManager::shared().getCurlShareHandle();

We usually use "singleton" in WebKit instead of "shared"
Comment 7 Alex Christensen 2017-06-21 09:48:30 PDT
Created attachment 313524 [details]
Comment 8 Alex Christensen 2017-06-21 09:49:07 PDT
Comment 9 Csaba Osztrogonác 2017-06-22 04:48:27 PDT
(In reply to Alex Christensen from comment #8)
> http://trac.webkit.org/r218637

FYI, it broke the WinCairo build: 
Comment 10 Basuke Suzuki 2017-06-22 08:40:44 PDT
Oh, thank you. I am fixing this problem.
Comment 11 Basuke Suzuki 2017-06-22 13:26:54 PDT
Created attachment 313653 [details]
Fix build error

This fix is to solve WinCairo build error.
Comment 12 Basuke Suzuki 2017-06-22 13:29:00 PDT
The previous patch broke the wincairo build.
Comment 13 Joseph Pecoraro 2017-06-22 13:54:22 PDT
Comment on attachment 313653 [details]
Fix build error

rs=me, but it would be good to describe how this solves the issue in the ChangeLog.

I guess curl.h is smarter about checking if SOCKET is defined already? To avoid the redefinition error:

C:\Program Files (x86)\Windows Kits\8.1\include\um\winsock2.h(111): error C2371: 'SOCKET': redefinition; different basic types
Comment 14 Basuke Suzuki 2017-06-22 14:06:15 PDT
I thought so and I can see the ifdef check inside curl/curl.h, but cannot solve this moment. I will keep refactoring this part and will see what's going on by following patches. I am so sorry about breaking the WinCairo build at this moment, so I hope this patch will be landed quickly.
Comment 15 Alex Christensen 2017-06-23 10:55:35 PDT
If this fixes the build, we need the build fixed.  We can't just have the build broken for many days at a time just because we're thinking about why a build fix works.  If we need to improve it in the future, we will.

Comment 16 Basuke Suzuki 2017-06-23 11:38:52 PDT
Thanks, Alex.