[webkit-reviews] review denied: [Bug 173629] [Curl] Separate global curl settings from ResourceHandleManager as CurlContext class : [Attachment 313871] PATH with fix for thread change

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has denied Basuke Suzuki
<Basuke.Suzuki at sony.com>'s request for review:
Bug 173629: [Curl] Separate global curl settings from ResourceHandleManager as
CurlContext class
https://bugs.webkit.org/show_bug.cgi?id=173629

Attachment 313871: PATH with fix for thread change

https://bugs.webkit.org/attachment.cgi?id=313871&action=review




--- Comment #9 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 313871
  --> https://bugs.webkit.org/attachment.cgi?id=313871
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.