[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