For advanced proxy support, this patch improve the usage of proxy information and to hold intermediate result or errors in it.
Created attachment 338159 [details] PATCH
Created attachment 338224 [details] ANaoTHER TRY
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?
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.
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.
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?
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
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
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.
Created attachment 338611 [details] PATCH AGAIN
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.
> > 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&
(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.
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?
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.
Created attachment 338665 [details] FIX Thanks for r+, Youenn.
Comment on attachment 338665 [details] FIX Clearing flags on attachment: 338665 Committed r230973: <https://trac.webkit.org/changeset/230973>
All reviewed patches have been landed. Closing bug.
<rdar://problem/39699217>