[webkit-reviews] review denied: [Bug 173991] [Curl] Unify ResourceHandleManager into CurlJobManager. : [Attachment 315477] Curl ResourceHandle is now run in background

[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 315477: Curl ResourceHandle is now run in background

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




--- Comment #2 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 315477
  --> https://bugs.webkit.org/attachment.cgi?id=315477
Curl ResourceHandle is now run in background

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

Hooray!

> Source/WebCore/ChangeLog:6
> +	   Use CurlJobManager to make ResourceHandle run in background.

Hooray!

> Source/WebCore/platform/network/ResourceHandle.h:297
> +    Vector<Vector<char>> m_receivedQueue;

This is...interesting.

> Source/WebCore/platform/network/curl/CurlContext.cpp:302
> +    void* that;

handle

> Source/WebCore/platform/network/curl/CurlContext.cpp:314
> +    return (m_errorCode = curl_easy_perform(m_handle));

unnecessary parentheses?
Please just put on two lines to make it clear that this is an assignment then
returning, not a forgotten ==

> Source/WebCore/platform/network/curl/CurlContext.h:174
> +class CurlSList {

Please put this in its own header.

> Source/WebCore/platform/network/curl/CurlDownload.cpp:94
> +	       callOnMainThread([this] {

protectedThis = makeRef(*this) to prevent use-after-free bugs if the last
reference goes out of scope before the lambda is called on the main thread.

> Source/WebCore/platform/network/curl/CurlJobManager.cpp:45
> +    using Predicate = std::function<bool(const CurlJob&)>;
> +    using Action = std::function<void(CurlJob&)>;

Let's use WTF::Function unless it needs to be copyable.

> Source/WebCore/platform/network/curl/CurlJobManager.cpp:88
> +    std::map<CurlJobTicket, CurlJob> m_jobs;

We typically use WTF data structures in WebKit instead of stl, such as HashMap
in this case.  They're more optimized for WebKit, and using the same data
structures everywhere makes it easier to put things into different places
without copying between WTF/stl types.

> Source/WebCore/platform/network/curl/CurlJobManager.cpp:152
> +void CurlJobManager::stopThreadIfNoMore()

No more what?

> Source/WebCore/platform/network/curl/CurlJobManager.cpp:172
> +template<typename C>

T is more common, or a more descriptive name.

> Source/WebCore/platform/network/curl/CurlJobManager.cpp:199
> +    std::vector<CurlJob> pendingJobs;

Vector

> Source/WebCore/platform/network/curl/CurlJobManager.h:99
> +	   return std::count(m_activeJobs.begin(), m_activeJobs.end(), job) >
0;

If you make m_activeJobs is a HashMap, you could probably use contains here.

> Source/WebCore/platform/network/curl/CurlJobManager.h:106
> +    std::set<CurlJobTicket> m_activeJobs;

HashSet

> Source/WebCore/platform/network/curl/CurlJobManager.h:109
> +    std::map<CurlJobTicket, std::vector<CurlJobTask>> m_taskQueue;

MessageQueue?

> Source/WebCore/platform/network/curl/ResourceError.h:27
>  #ifndef ResourceError_h
>  #define ResourceError_h

#pragma once

> Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp:74
> +    CurlJobManager::singleton().add(d->m_curlHandle, [this](CurlJobResult
result) {

protectedThis = makeRef(*this)

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

protectedThis = makeRef(*this)

> Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp:710
> +    if (splitPos != -1) {

notFound

> Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp:731
> +	   if (statusCodePos != -1) {

ditto