Bug 184714 – [Curl] Extract proxy settings into a separate class to hold advanced information.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Basuke Suzuki]

For advanced proxy support, this patch improve the usage of proxy information and to hold intermediate result or errors in it.

[Basuke Suzuki]

Created attachment 338159 [details]
PATCH

[Basuke Suzuki]

Created attachment 338224 [details]
ANaoTHER TRY

[youenn fablet]

Comment on attachment 338224 [details]
ANaoTHER TRY

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

> Source/WebCore/platform/network/curl/CurlContext.h:116
> +    void setProxySettings(const CurlProxySettings& settings) { m_proxySettings = settings.isolatedCopy(); }

Why using isolatedCopy?
Can't we do void setProxySettings(CurlProxySettings&&) instead?

I also saw some isolatedCopy for strings in other parts of the patch.
Do we need all of them?

[Basuke Suzuki]

Comment on attachment 338224 [details]
ANaoTHER TRY

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

Thank you for reviewing.

>> Source/WebCore/platform/network/curl/CurlContext.h:116
>> +    void setProxySettings(const CurlProxySettings& settings) { m_proxySettings = settings.isolatedCopy(); }
> 
> Why using isolatedCopy?
> Can't we do void setProxySettings(CurlProxySettings&&) instead?
> 
> I also saw some isolatedCopy for strings in other parts of the patch.
> Do we need all of them?

Right. && is perfectly reasonable here. I'll change with here and other places.

[Basuke Suzuki]

Created attachment 338281 [details]
Use &&

Switch to use move semantics for parameters. These are almost one shot usage and no reason not to use move semantics.

[youenn fablet]

Comment on attachment 338281 [details]
Use &&

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

> Source/WebCore/platform/network/curl/CurlProxySettings.cpp:56
> +        return;

This seems a bit weird to return port as we do not use port afterwards.
Can we try to improve this? Is it something like if (!verifyProxyURL(proxyURL)) return; ?

> Source/WebCore/platform/network/curl/CurlProxySettings.cpp:63
> +CurlProxySettings& CurlProxySettings::operator=(const CurlProxySettings& other)

Why do we need to define this one?

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

should be String I guess. and "" could be just String { }.

> Source/WebCore/platform/network/curl/CurlProxySettings.h:33
> +class CurlProxySettings {

I wonder whether it could not be made a struct.
The private methods could be transformed into static inline free functions.

> Source/WebCore/platform/network/curl/CurlProxySettings.h:66
> +    String m_proxyUrl;

Why do we need to get both m_url and m_proxyURL?
Can we just return m_url.string()?

> Source/WebCore/platform/network/curl/CurlResponse.h:57
> +    long httpConnectCode { 0 };

Why do we need to have both codes?
Can we just not compute response.statusCode according curl statusCode and httpConnectCode?

[EWS Watchlist]

Comment on attachment 338281 [details]
Use &&

Attachment 338281 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7365383

New failing tests:
http/tests/security/local-video-source-from-remote.html

[EWS Watchlist]

Created attachment 338296 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Basuke Suzuki]

Comment on attachment 338281 [details]
Use &&

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

Thanks for the review.

>> Source/WebCore/platform/network/curl/CurlProxySettings.cpp:56
>> +        return;
> 
> This seems a bit weird to return port as we do not use port afterwards.
> Can we try to improve this? Is it something like if (!verifyProxyURL(proxyURL)) return; ?

Got it. We'll change this with more meaningful methods.

>> Source/WebCore/platform/network/curl/CurlProxySettings.cpp:107
>> +    const auto& userpass = (m_url.hasUsername() || m_url.hasPassword()) ? makeString(m_url.user(), ":", m_url.pass(), "@") : "";
> 
> should be String I guess. and "" could be just String { }.

Plan to swtich to use m_url.string() directly as you suggested below.

>> Source/WebCore/platform/network/curl/CurlProxySettings.h:33
>> +class CurlProxySettings {
> 
> I wonder whether it could not be made a struct.
> The private methods could be transformed into static inline free functions.

I didn't get understand your point correctly. Do you suggest changing this class into struct and make static functions into namespace global functions?

>> Source/WebCore/platform/network/curl/CurlProxySettings.h:66
>> +    String m_proxyUrl;
> 
> Why do we need to get both m_url and m_proxyURL?
> Can we just return m_url.string()?

Let us think about that approach. It seems reasonable to me, but I need to discuss with other team members.

>> Source/WebCore/platform/network/curl/CurlResponse.h:57
>> +    long httpConnectCode { 0 };
> 
> Why do we need to have both codes?
> Can we just not compute response.statusCode according curl statusCode and httpConnectCode?

Because we need both of these values internally. We need to distinguish the proxy connect code from status code.

[Basuke Suzuki]

Created attachment 338611 [details]
PATCH AGAIN

[Basuke Suzuki]

Comment on attachment 338611 [details]
PATCH AGAIN

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

> Source/WebCore/platform/network/curl/CurlContext.h:107
> +    void setProxySettings(const CurlProxySettings& settings) { m_proxySettings = settings; }

We need to hold copy of original settings. I made this back to const reference.

> Source/WebCore/platform/network/curl/CurlProxySettings.cpp:33
> +static const uint16_t SocksProxyPort = 1080;

We moved private methods into static functions to hide the implementation of the class.

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

[youenn fablet]

> > Source/WebCore/platform/network/curl/CurlContext.h:107
> > +    void setProxySettings(const CurlProxySettings& settings) { m_proxySettings = settings; }
> 
> We need to hold copy of original settings. I made this back to const
> reference.

If you need to hold copy of the original settings, it might make sense to actually construct the copy at the call site and send the copy as an r-value. If there are many call sites for that method, it might make sense to keep a const&

[Basuke Suzuki]

(In reply to youenn fablet from comment #12)
> > > Source/WebCore/platform/network/curl/CurlContext.h:107
> > > +    void setProxySettings(const CurlProxySettings& settings) { m_proxySettings = settings; }
> > 
> > We need to hold copy of original settings. I made this back to const
> > reference.
> 
> If you need to hold copy of the original settings, it might make sense to
> actually construct the copy at the call site and send the copy as an
> r-value. If there are many call sites for that method, it might make sense
> to keep a const&

But copying in the constructor is flexible than copying in the caller because inplementation has choice to copy or to hold the reference. I prefer this way, but it WebKit community prefer your way, I will follow that.

[youenn fablet]

Comment on attachment 338611 [details]
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?

[Basuke Suzuki]

Comment on attachment 338611 [details]
PATCH AGAIN

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

Thanks for the review!

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

Thanks for rewriting. Coding is much easier than English.

>>> 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?

Make sense.

[Basuke Suzuki]

Created attachment 338665 [details]
FIX

Thanks for r+, Youenn.

[WebKit Commit Bot]

Comment on attachment 338665 [details]
FIX

Clearing flags on attachment: 338665

Committed r230973: <https://trac.webkit.org/changeset/230973>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/39699217>