[webkit-reviews] review granted: [Bug 189053] [Curl][WebKit] Implement Proxy configuration API. : [Attachment 353430] FIX
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Nov 8 07:18:58 PST 2018
youenn fablet <youennf at gmail.com> has granted Basuke Suzuki
<Basuke.Suzuki at sony.com>'s request for review:
Bug 189053: [Curl][WebKit] Implement Proxy configuration API.
https://bugs.webkit.org/show_bug.cgi?id=189053
Attachment 353430: FIX
https://bugs.webkit.org/attachment.cgi?id=353430&action=review
--- Comment #37 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 353430
--> https://bugs.webkit.org/attachment.cgi?id=353430
FIX
LGTM, some nits below.
Usually, when we add new API, it is a good practice to write some unit tests if
the API is not tested as part of WebKitTestRunner.
I am not sure how much in that case that will help.
Currently our API tests do not run concurrently to an HTTP server.
View in context: https://bugs.webkit.org/attachment.cgi?id=353430&action=review
> Source/WebCore/platform/network/NetworkStorageSession.h:139
> + WEBCORE_EXPORT void setProxySettings(CurlProxySettings&&) const;
It is strange to have setters be const, mutable behind I guess.
Should we make it non const?
Ditto for setCookieDatabase.
> Source/WebKit/ChangeLog:8
> + Added proxy configuration API to WebsiteDataStore. Three API was
added in /WKWebsiteDataStoreRefCurl.h:
s/was/were/.
I would remove the / from WKWebsiteDataStoreRefCurl.h
> Source/WebKit/NetworkProcess/curl/RemoteNetworkingContextCurl.cpp:42
> + return;
We probably have the same first 3 lines in SOUP/Mac ports.
Probably some refactoring could be done but it might be better as a follow-up.
> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1668
> #endif
There is some potential refactoring code that could also be done here as well
to share this code with Mac.
> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:198
> + const WebCore::CurlProxySettings& networkProxySettings() { return
m_proxySettings; }
Should be const probably.
More information about the webkit-reviews
mailing list