[webkit-reviews] review granted: [Bug 184714] [Curl] Extract proxy settings into a separate class to hold advanced information. : [Attachment 338611] PATCH AGAIN

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Basuke Suzuki
<Basuke.Suzuki at sony.com>'s request for review:
Bug 184714: [Curl] Extract proxy settings into a separate class to hold
advanced information.
https://bugs.webkit.org/show_bug.cgi?id=184714

Attachment 338611: PATCH AGAIN

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




--- Comment #14 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 338611
  --> https://bugs.webkit.org/attachment.cgi?id=338611
PATCH AGAIN

View in context: https://bugs.webkit.org/attachment.cgi?id=338611&action=review

> Source/WebCore/platform/network/curl/CurlProxySettings.cpp:39
> +CurlProxySettings::CurlProxySettings(const URL& proxyUrl, const String
ignoreHosts)

probably const String& instead of const String?
Should probably be URL&& and String&&.

> Source/WebCore/platform/network/curl/CurlProxySettings.cpp:53
> +	   m_proxyUrl = *url;

m_proxyURL = WTFMove(*url);

> Source/WebCore/platform/network/curl/CurlProxySettings.cpp:81
> +    */

Can we write the comment as follows:
// For Curl port, if port number is not specified for HTTP Proxy protocol, port
80 is used.
// This differs for libcurl's default port number for HTTP proxy whose value
(1080) is for socks.

> Source/WebCore/platform/network/curl/CurlProxySettings.cpp:103
> +    const auto& userpass = (url.hasUsername() || url.hasPassword()) ?
makeString(url.user(), ":", url.pass(), "@") : String();

s/const auto&/String/ or maybe just auto.

> Source/WebCore/platform/network/curl/CurlProxySettings.h:46
> +    WEBCORE_EXPORT CurlProxySettings& operator=(const CurlProxySettings&) =
default;

Isn't it strange to delete copy constructor but define a default copy
assignment?

>> Source/WebCore/platform/network/curl/CurlProxySettings.h:51
>> +	const String& url() const { return m_proxyUrl; }
> 
> We didn't use m_url.string() because we need port number. URLParser omit the
default port, but libcurl assume 1080 as a default HTTP Proxy, not 80, if port
number is not in the url. We need to explicitly indicate the port number to
libcurl.

I see, this is a useful comment to explain why there is m_url and m_proxyUrl.
Maybe m_url/m_proxyUrl could have better names to convey that information.
Maybe m_proxyUrl could be renamed to m_urlSerializedWithPort or something like
that?