Bug 177779 - [Curl] Reimplement CurlDownload with CurlRequest
Summary: [Curl] Reimplement CurlDownload with CurlRequest
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Basuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-10-02 14:39 PDT by Basuke Suzuki
Modified: 2017-10-03 20:16 PDT (History)
8 users (show)

See Also:


Attachments
patch (24.93 KB, patch)
2017-10-02 16:35 PDT, Basuke Suzuki
achristensen: review-
achristensen: commit-queue-
Details | Formatted Diff | Diff
fix (27.86 KB, patch)
2017-10-03 12:54 PDT, Basuke Suzuki
no flags Details | Formatted Diff | Diff
fix (27.83 KB, patch)
2017-10-03 13:04 PDT, Basuke Suzuki
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews114 for mac-elcapitan (1.94 MB, application/zip)
2017-10-03 19:00 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Basuke Suzuki 2017-10-02 14:39:12 PDT
Refactoring the implementation of CurlDownload by using recent CurlRequest class.
Comment 1 Basuke Suzuki 2017-10-02 16:35:16 PDT
Created attachment 322469 [details]
patch
Comment 2 Alex Christensen 2017-10-02 21:23:32 PDT
Comment on attachment 322469 [details]
patch

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

> Source/WebCore/platform/network/curl/CurlDownload.cpp:87
> +    return CurlRequest::create(request, this);

Can we pass *this instead?  This is never null in this case, and in most cases.

> Source/WebCore/platform/network/curl/CurlDownload.h:55
>      void init(CurlDownloadListener*, const URL&);

Could these take a CurlDownloadListener& ?  Do we need both init functions?  Could they be put into the constructor instead?

> Source/WebCore/platform/network/curl/CurlDownload.h:69
> +    RefPtr<CurlRequest> createCurlRequest(ResourceRequest&);

Let's return a Ref here.

> Source/WebCore/platform/network/curl/CurlDownload.h:85
> +    int m_redirectCount { 0 };

Does this need to be signed?

> Source/WebCore/platform/network/curl/CurlRequest.h:49
> +        return adoptRef(new CurlRequest(request, delegate, shouldSuspend));

adoptRef(*new ...) and return a Ref to indicate that it can't be null.  That's how we do things in WebKit.
Comment 3 Basuke Suzuki 2017-10-03 12:47:28 PDT
Comment on attachment 322469 [details]
patch

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

>> Source/WebCore/platform/network/curl/CurlDownload.cpp:87
>> +    return CurlRequest::create(request, this);
> 
> Can we pass *this instead?  This is never null in this case, and in most cases.

delegate is optional and I believe it is good to use nullptr as non-valid delegate. When the client dies first, it set the delegate to nullptr.

>> Source/WebCore/platform/network/curl/CurlDownload.h:55
>>      void init(CurlDownloadListener*, const URL&);
> 
> Could these take a CurlDownloadListener& ?  Do we need both init functions?  Could they be put into the constructor instead?

The two version of init() signatures are shared with AppleWin's CFURLDownloadRef so that wider discussion is required before the change. Maybe another bug should be better place to discuss.

>> Source/WebCore/platform/network/curl/CurlDownload.h:69
>> +    RefPtr<CurlRequest> createCurlRequest(ResourceRequest&);
> 
> Let's return a Ref here.

Okay.

>> Source/WebCore/platform/network/curl/CurlDownload.h:85
>> +    int m_redirectCount { 0 };
> 
> Does this need to be signed?

It should be unsigned.

>> Source/WebCore/platform/network/curl/CurlRequest.h:49
>> +        return adoptRef(new CurlRequest(request, delegate, shouldSuspend));
> 
> adoptRef(*new ...) and return a Ref to indicate that it can't be null.  That's how we do things in WebKit.

Got it.
Comment 4 Basuke Suzuki 2017-10-03 12:54:50 PDT
Created attachment 322567 [details]
fix
Comment 5 Build Bot 2017-10-03 12:57:27 PDT
Attachment 322567 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Basuke Suzuki 2017-10-03 13:04:55 PDT
Created attachment 322570 [details]
fix
Comment 7 Build Bot 2017-10-03 19:00:09 PDT
Comment on attachment 322570 [details]
fix

Attachment 322570 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4747836

New failing tests:
workers/wasm-long-compile.html
Comment 8 Build Bot 2017-10-03 19:00:11 PDT
Created attachment 322623 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 9 Alex Christensen 2017-10-03 19:44:06 PDT
Comment on attachment 322570 [details]
fix

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

> Source/WebCore/platform/network/curl/CurlDownload.h:69
> +    Ref<CurlRequest> createCurlRequest(ResourceRequest&);

You might want to take a const ResourceRequest& if you're going to copy it or a ResourceRequest&& if you don't need to copy it.

> Source/WebCore/platform/network/curl/CurlRequest.cpp:616
> +        m_downloadFilePath = openTemporaryFile("download", m_downloadFileHandle);

You might want to check if opening failed and not try to re-open it every time if it fails.
Comment 10 WebKit Commit Bot 2017-10-03 20:15:31 PDT
Comment on attachment 322570 [details]
fix

Clearing flags on attachment: 322570

Committed r222828: <http://trac.webkit.org/changeset/222828>
Comment 11 WebKit Commit Bot 2017-10-03 20:15:33 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Radar WebKit Bug Importer 2017-10-03 20:16:11 PDT
<rdar://problem/34805524>