ResourceHandleManager is hard to refactor because it contains many configurations. For easy maintenance, those information should be managed by separate class.
Created attachment 313793 [details] PATCH Extract global configurations from ResourceHandleManager.
Attachment 313793 [details] did not pass style-queue: ERROR: Source/WebCore/platform/network/curl/CurlJobManager.cpp:30: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Source/WebCore/platform/network/curl/CurlJobManager.cpp:31: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Source/WebCore/platform/network/curl/CurlJobManager.h:38: Bad include order. Mixing system and custom headers. [build/include_order] [4] ERROR: Source/WebCore/platform/network/curl/ResourceHandleManager.cpp:969: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/WebCore/platform/network/curl/CurlContext.cpp:173: Extra space between CurlProxyType and type [whitespace/declaration] [3] ERROR: Source/WebCore/platform/network/curl/CurlContext.cpp:182: An else should appear on the same line as the preceding } [whitespace/newline] [4] ERROR: Source/WebCore/platform/network/curl/CurlDownload.cpp:30: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Source/WebCore/platform/network/curl/CurlContext.h:39: Bad include order. Mixing system and custom headers. [build/include_order] [4] Total errors found: 8 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 313794 [details] Fix style
Comment on attachment 313794 [details] Fix style A bug was found. Obsoleted.
Created attachment 313861 [details] PATCH2 Fixed a bug
Created attachment 313871 [details] PATH with fix for thread change Adopt the changes for Thread::create [WTF] Drop Thread::create(obsolete things) API since we can use lambda https://bugs.webkit.org/show_bug.cgi?id=173825
Comment on attachment 313871 [details] PATH with fix for thread change View in context: https://bugs.webkit.org/attachment.cgi?id=313871&action=review > Source/WebCore/platform/network/curl/CurlContext.h:35 > +#if PLATFORM(WIN) OS(WINDOWS) > Source/WebCore/platform/network/curl/CurlContext.h:68 > + virtual ~CurlContext(); This class is a singleton and is not supposed to be inherited from, it should not have any virtual methods > Source/WebCore/platform/network/curl/CurlContext.h:70 > + CURLSH* getCurlShareHandle() const { return m_curlShareHandle; } According to coding style, it should be just curlShareHandle() https://webkit.org/code-style-guidelines/#names > Source/WebCore/platform/network/curl/CurlContext.h:83 > + std::tuple<CurlProxyType, const String&> getProxyInfo() const; Prefer informatively named structs to pairs and tuples. The latter are mostly useful for generic (template) code
Comment on attachment 313871 [details] PATH with fix for thread change View in context: https://bugs.webkit.org/attachment.cgi?id=313871&action=review > Source/WebCore/platform/network/curl/CurlContext.cpp:76 > +#if OS(WINDOWS) I'm wondering if this block should be using FileSystem.h functions. I'm fine with that being in another patch if there's more refactoring coming for cookies.
Comment on attachment 313871 [details] PATH with fix for thread change View in context: https://bugs.webkit.org/attachment.cgi?id=313871&action=review > Source/WebCore/platform/network/curl/CurlContext.h:58 > + static CurlContext& singleton() Can this return a const CurlContext&? > Source/WebCore/platform/network/curl/CurlContext.h:62 > + // Since it's a static variable, if the class has already been created, > + // It won't be created again. > + // And it **is** thread-safe in C++11. I don't think this comment is helpful. >> Source/WebCore/platform/network/curl/CurlContext.h:83 >> + std::tuple<CurlProxyType, const String&> getProxyInfo() const; > > Prefer informatively named structs to pairs and tuples. The latter are mostly useful for generic (template) code Yep. std::tuples of two things are std::pairs, but this should definitely be a struct with named members.
(In reply to Konstantin Tokarev from comment #7) > Comment on attachment 313871 [details] > PATH with fix for thread change > > View in context: > https://bugs.webkit.org/attachment.cgi?id=313871&action=review > > > Source/WebCore/platform/network/curl/CurlContext.h:35 > > +#if PLATFORM(WIN) > > OS(WINDOWS) > > > Source/WebCore/platform/network/curl/CurlContext.h:68 > > + virtual ~CurlContext(); > > This class is a singleton and is not supposed to be inherited from, it > should not have any virtual methods > > > Source/WebCore/platform/network/curl/CurlContext.h:70 > > + CURLSH* getCurlShareHandle() const { return m_curlShareHandle; } > > According to coding style, it should be just curlShareHandle() > > https://webkit.org/code-style-guidelines/#names > > > Source/WebCore/platform/network/curl/CurlContext.h:83 > > + std::tuple<CurlProxyType, const String&> getProxyInfo() const; > > Prefer informatively named structs to pairs and tuples. The latter are > mostly useful for generic (template) code Got it. I'll change them. > Alex I cannot make CurlContext::singleton() as const because it has configuration change method such as setProxyInfo(). > Don I will submit platform-independency bug after this patch accepted.
Created attachment 314094 [details] PATCH with reviewed advices
Attachment 314094 [details] did not pass style-queue: ERROR: Source/WebCore/platform/network/curl/CurlContext.h:57: Missing space before { [whitespace/braces] [5] Total errors found: 1 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 314095 [details] Fix style again
Comment on attachment 314095 [details] Fix style again Clearing flags on attachment: 314095 Committed r218947: <http://trac.webkit.org/changeset/218947>
All reviewed patches have been landed. Closing bug.