[webkit-reviews] review denied: [Bug 177779] [Curl] Reimplement CurlDownload with CurlRequest : [Attachment 322469] patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Oct 2 21:23:32 PDT 2017
Alex Christensen <achristensen at apple.com> has denied Basuke Suzuki
<Basuke.Suzuki at sony.com>'s request for review:
Bug 177779: [Curl] Reimplement CurlDownload with CurlRequest
https://bugs.webkit.org/show_bug.cgi?id=177779
Attachment 322469: patch
https://bugs.webkit.org/attachment.cgi?id=322469&action=review
--- Comment #2 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 322469
--> https://bugs.webkit.org/attachment.cgi?id=322469
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.
More information about the webkit-reviews
mailing list