[webkit-reviews] review denied: [Bug 173991] [Curl] Unify ResourceHandleManager into CurlJobManager. : [Attachment 315682] PATCH with reviewed advices
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Jul 17 11:26:31 PDT 2017
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
More information about the webkit-reviews
mailing list