[webkit-reviews] review denied: [Bug 177373] [Curl] Refactor and improve methods in the CurlHandle : [Attachment 321714] patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Sep 25 12:14:14 PDT 2017
Alex Christensen <achristensen at apple.com> has denied Basuke Suzuki
<Basuke.Suzuki at sony.com>'s request for review:
Bug 177373: [Curl] Refactor and improve methods in the CurlHandle
https://bugs.webkit.org/show_bug.cgi?id=177373
Attachment 321714: patch
https://bugs.webkit.org/attachment.cgi?id=321714&action=review
--- Comment #2 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 321714
--> https://bugs.webkit.org/attachment.cgi?id=321714
patch
View in context: https://bugs.webkit.org/attachment.cgi?id=321714&action=review
> Source/WebCore/platform/network/curl/CurlContext.cpp:263
> + m_handle = nullptr;
It's probably not necessary to set m_handle to nullptr in the destructor.
> Source/WebCore/platform/network/curl/CurlContext.cpp:345
> +void CurlHandle::removeRequestHeader(const String& name)
> +{
> + // Add a header with no content, the internally used header will get
disabled.
> + String header(name);
> + header.append(":");
> +
> appendRequestHeader(header);
This is very confusing. Are we removing or appending? We should definitely
call it something else.
> Source/WebCore/platform/network/curl/CurlContext.cpp:354
> +void CurlHandle::appendRequestHeader(const String& header, bool appendOnly)
> {
> m_requestHeaders.append(header);
> +
> + if (!appendOnly)
> + enableRequestHeaders();
> }
I think adding this bool indicates that we've got the responsibilities of these
functions all wrong.
More information about the webkit-reviews
mailing list