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

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has granted 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 315732: PATCH with reviewed advices

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




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

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

Let's land this.

> Source/WebCore/platform/network/curl/CurlJobManager.cpp:91
> +	   return withJob(ticket, [&callback](JobMap::iterator it) {

Here we just call the callback immediately, but if we needed to do anything
interesting with the callback, we would want to move it into the lambda capture
list, not just capturing it by reference.
[callback = WTFMove(callback)]
This can be done in a future patch if needed.

> Source/WebCore/platform/network/curl/CurlJobManager.h:60
> +	   : m_curl { curl }, m_job { WTFMove(job) } { }

These could be on different lines.

> Source/WebCore/platform/network/curl/CurlJobManager.h:64
> +	   other.m_curl = nullptr;

It's probably not necessary to null out a pointer of something that was moved
into this constructor.

> Source/WebCore/platform/network/curl/ResourceHandleCurl.cpp:712
> +    int splitPos = header.find(":");

auto or size_t.
Also please rename splitPos to splitPosition in a followup patch.