[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