Bug 177779 – [Curl] Reimplement CurlDownload with CurlRequest

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Basuke Suzuki]

Refactoring the implementation of CurlDownload by using recent CurlRequest class.

[Basuke Suzuki]

Created attachment 322469 [details]
patch

[Alex Christensen]

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.

[Basuke Suzuki]

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.

[Basuke Suzuki]

Created attachment 322567 [details]
fix

[Build Bot]

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.

[Basuke Suzuki]

Created attachment 322570 [details]
fix

[Build Bot]

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

[Build Bot]

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

[Alex Christensen]

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.

[WebKit Commit Bot]

Comment on attachment 322570 [details]
fix

Clearing flags on attachment: 322570

Committed r222828: <http://trac.webkit.org/changeset/222828>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34805524>