[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