Bug 173629 – [Curl] Separate global curl settings from ResourceHandleManager as CurlContext class

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Basuke Suzuki]

ResourceHandleManager is hard to refactor because it contains many configurations. For easy maintenance, those information should be managed by separate class.

[Basuke Suzuki]

Created attachment 313793 [details]
PATCH

Extract global configurations from ResourceHandleManager.

[Build Bot]

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.

[Basuke Suzuki]

Created attachment 313794 [details]
Fix style

[Basuke Suzuki]

Comment on attachment 313794 [details]
Fix style

A bug was found. Obsoleted.

[Basuke Suzuki]

Created attachment 313861 [details]
PATCH2

Fixed a bug

[Basuke Suzuki]

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

[Konstantin Tokarev]

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

[Don Olmstead]

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.

[Alex Christensen]

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.

[Basuke Suzuki]

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

[Basuke Suzuki]

Created attachment 314094 [details]
PATCH with reviewed advices

[Build Bot]

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.

[Basuke Suzuki]

Created attachment 314095 [details]
Fix style again

[WebKit Commit Bot]

Comment on attachment 314095 [details]
Fix style again

Clearing flags on attachment: 314095

Committed r218947: <http://trac.webkit.org/changeset/218947>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.