Bug 177373 – [Curl] Refactor and improve methods in the CurlHandle

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Basuke Suzuki]

Cleanup method name, consistent naming, remove unused feature, better enum naming, etc.

[Basuke Suzuki]

Created attachment 321714 [details]
patch

[Alex Christensen]

Comment on attachment 321714 [details]
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.

[Basuke Suzuki]

Created attachment 321723 [details]
fix

[Basuke Suzuki]

Thank you. I've fixed.

[Basuke Suzuki]

> > 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.

This is the libcurl's spec. If we need to delete the pre appended header including the default prepared one, we need to append empty value header to the list. The original code has used that behavior directly, so we've added a meaningful method and hide the detail inside.

[Basuke Suzuki]

(In reply to Alex Christensen from comment #2)
> Comment on attachment 321714 [details]
> 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.

removed. Also removed from other class's destructors.


> > 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.

removed that flag argument and handle the situation internally.

[WebKit Commit Bot]

Comment on attachment 321723 [details]
fix

Clearing flags on attachment: 321723

Committed r222479: <http://trac.webkit.org/changeset/222479>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34692963>