[webkit-reviews] review denied: [Bug 173991] [Curl] Unify ResourceHandleManager into CurlJobManager. : [Attachment 315682] PATCH with reviewed advices

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has denied Basuke Suzuki
<Basuke.Suzuki at sony.com>'s request for review:
Bug 173991: [Curl] Unify ResourceHandleManager into CurlJobManager.
https://bugs.webkit.org/show_bug.cgi?id=173991

Attachment 315682: PATCH with reviewed advices

https://bugs.webkit.org/attachment.cgi?id=315682&action=review




--- Comment #8 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 315682
  --> https://bugs.webkit.org/attachment.cgi?id=315682
PATCH with reviewed advices

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

Cool.  Let's do another iteration.

> Source/WebCore/platform/network/curl/CurlContext.cpp:300
> +CurlHandle* CurlHandle::recovery(CURL* h)

This isn't used, is it?  I also don't think it's well named.  Maybe fromCURL?

> Source/WebCore/platform/network/curl/CurlContext.cpp:348
> +    if (headers.size() > 0) {

I don't think the > 0 is necessary.

> Source/WebCore/platform/network/curl/CurlContext.h:178
> +    using native = struct curl_slist*;

I don't think this alias improves anything.

> Source/WebCore/platform/network/curl/CurlContext.h:184
> +    CurlSList(CurlSList&& other) { std::swap(m_list, other.m_list); }
> +    CurlSList& operator=(CurlSList&& other) { std::swap(m_list,
other.m_list); return *this; }

I don't think this is a proper use of std::swap.  We don't need other to get
the current pointer from this.

> Source/WebCore/platform/network/curl/CurlDownload.cpp:90
> +    m_job = CurlJobManager::singleton().add(m_curlHandle, 
> +	   [protectedThis = makeRef(*this)](CurlJobResult result) mutable {

This can be on one line.

> Source/WebCore/platform/network/curl/CurlDownload.cpp:226
> +    callOnMainThread([protectedThis = makeRef(*this)]() mutable {
> +	   if (protectedThis->m_listener)

You can also capture this so you don't need all the protectedThis->

> Source/WebCore/platform/network/curl/CurlDownload.cpp:242
> +	   if (protectedThis->m_listener)

ditto

> Source/WebCore/platform/network/curl/CurlJobManager.h:80
> +	   std::swap(m_curl, other.m_curl);
> +	   std::swap(m_job, other.m_job);

Again, other doesn't need to get the pointer from this.

> Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp:74
> +	   [protectedThis = makeRef(*this)](CurlJobResult result) {

Capture this, too

> Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp:156
> +    if (firstRequest().httpHeaderFields().size() > 0) {

> 0 unnecessary

> Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp:898
> +	   callOnMainThread([job, httpCode, contentLength] {

It would prevent some use-after-free bugs to make a RefPtr for job, too.  Right
now there is no guarantee the ResourceHandle still exists when the lambda is
called on the main thread.
job = RefPtr<ResourceHandle>(job)

> Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp:941
> +	   callOnMainThread([job] {

ditto