Summary: | [Curl] Refactor and improve methods in the CurlHandle | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Basuke Suzuki <Basuke.Suzuki> | ||||||
Component: | WebCore Misc. | Assignee: | Basuke Suzuki <Basuke.Suzuki> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | achristensen, Basuke.Suzuki, bfulgham, buildbot, commit-queue, don.olmstead, galpeter, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Basuke Suzuki
2017-09-22 11:46:46 PDT
Created attachment 321714 [details]
patch
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. Created attachment 321723 [details]
fix
Thank you. I've fixed. > > 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.
(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. Comment on attachment 321723 [details] fix Clearing flags on attachment: 321723 Committed r222479: <http://trac.webkit.org/changeset/222479> All reviewed patches have been landed. Closing bug. |