Bug 189053 – [Curl][WebKit] Implement Proxy configuration API.

• Component: Platform
• Status: RESOLVED FIXED

[Basuke Suzuki]

Implement Curl specific proxy and TLS related settings on WebKit NetworkProcess.

[Basuke Suzuki]

Created attachment 348321 [details]
PATCH

[EWS Watchlist]

Attachment 348321 [details] did not pass style-queue:


ERROR: Source/WebKit/PlatformWin.cmake:67:  Alphabetical sorting problem. "UIProcess/API/C/curl/WKContextCurl.cpp" should be before "UIProcess/win/WebView.cpp".  [list/order] [5]
ERROR: Source/WebKit/UIProcess/API/C/curl/WKContextCurl.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKit/UIProcess/win/WebProcessPoolWin.cpp:33:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
ERROR: Source/WebKit/NetworkProcess/curl/NetworkProcessCurl.cpp:113:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 4 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Basuke Suzuki]

Created attachment 348333 [details]
PATCH

[Basuke Suzuki]

Created attachment 348334 [details]
PATCH

[EWS Watchlist]

Attachment 348334 [details] did not pass style-queue:


ERROR: Source/WebKit/PlatformWin.cmake:69:  Alphabetical sorting problem. "UIProcess/curl/WebProcessPoolCurl.cpp" should be before "UIProcess/win/WebView.cpp".  [list/order] [5]
ERROR: Source/WebKit/UIProcess/API/C/curl/WKContextCurl.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKit/UIProcess/win/WebProcessPoolWin.cpp:33:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
ERROR: Source/WebKit/NetworkProcess/curl/NetworkProcessCurl.cpp:113:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 4 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Basuke Suzuki]

Created attachment 348339 [details]
PATCH

[Don Olmstead]

Comment on attachment 348339 [details]
PATCH

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

> Source/WebKit/NetworkProcess/win/NetworkProcessWin.cpp:38
> +    platformInitializeNetworkProcessCurl(parameters);

I don't think we should be going against the other ports in terms of how the NetworkProcess is being laid out.

[Basuke Suzuki]

(In reply to Don Olmstead from comment #7)
> Comment on attachment 348339 [details]
> PATCH
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=348339&action=review
> 
> > Source/WebKit/NetworkProcess/win/NetworkProcessWin.cpp:38
> > +    platformInitializeNetworkProcessCurl(parameters);
> 
> I don't think we should be going against the other ports in terms of how the
> NetworkProcess is being laid out.

Our private port also uses Curl and it requires some special initialization. It's a little bit weird because There's no special extra initialization on Windows side, but the separation is needed for our platform.

[Fujii Hironori]

Comment on attachment 348339 [details]
PATCH

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

I think you should make this change testable, for example, by adding proxy and TLS setting UI for MiniBrowser.

> Source/WebKit/NetworkProcess/curl/NetworkDataTaskCurl.cpp:30
> +#include "NetworkProcess.h"

You don't need to add this line.

> Source/WebKit/Shared/curl/WebCoreArgumentCodersCurl.cpp:35
> +#include <WebCore/URL.h>

You should not include URL.h because CurlProxySettings.h includes URL.h.

> Source/WebKit/Shared/curl/WebCoreArgumentCodersCurl.cpp:134
> +    ASSERT(!settings.isEmpty());

IIUC, this assertion can fail if the client calls WKContextSetNetworkProxySettings with kWKNetworkProxyModeCustom and an empty proxyUrl.

> Source/WebKit/UIProcess/API/C/curl/WKContextCurl.cpp:30
> +#include "WebProcessPool.h"

Remove this blank line.
https://webkit.org/code-style-guidelines/#include-system

> Source/WebKit/UIProcess/API/C/curl/WKContextCurl.h:44
> +WK_EXPORT WKStringRef WKContextGetSslCACertPath(WKContextRef);

You are using toCopiedAPI to return a copy of string. You should name an API Copy instead of Get if you return a copy of string in WK API.
For example, WKPageCopyTitle.
And, You should name SSL instead of Ssl, for exameple WKContextSetAllowsAnySSLCertificateForWebSocketTesting.
WKContextGetSslCACertPath -> WKContextCopySSLCACertPath

> Source/WebKit/UIProcess/win/WebProcessPoolWin.cpp:33
> +

Remove this blank line.
https://webkit.org/code-style-guidelines/#include-system

[Basuke Suzuki]

Thanks for review!

[Alex Christensen]

Comment on attachment 348339 [details]
PATCH

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

> Source/WebKit/UIProcess/API/C/curl/WKContextCurl.cpp:41
> +    case WKNetworkProxyMode::kWKNetworkProxyModeDefault :

No space before :

> Source/WebKit/UIProcess/API/C/curl/WKContextCurl.h:43
> +WK_EXPORT void WKContextSetSslCACertPath(WKContextRef, WKStringRef caCertPath);

I have two problems with this:
1) This should be per WKWebsiteDataStoreRef, which corresponds with a NetworkSession.  It may be tempting to add things to WKContext, but we're in the process of moving as much as we can away from WKContext.
2) I'm not sure this is a good design to expose these things via the API.  I think instead the cacert data should just be included in the bundle of files and resources that constitute WebKit.  There should definitely be a good default so that API users don't have to use this in order to get something working.

[Basuke Suzuki]

Comment on attachment 348339 [details]
PATCH

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

>> Source/WebKit/NetworkProcess/curl/NetworkDataTaskCurl.cpp:30
>> +#include "NetworkProcess.h"
> 
> You don't need to add this line.

Got it.

>> Source/WebKit/Shared/curl/WebCoreArgumentCodersCurl.cpp:35
>> +#include <WebCore/URL.h>
> 
> You should not include URL.h because CurlProxySettings.h includes URL.h.

Got it.

>> Source/WebKit/Shared/curl/WebCoreArgumentCodersCurl.cpp:134
>> +    ASSERT(!settings.isEmpty());
> 
> IIUC, this assertion can fail if the client calls WKContextSetNetworkProxySettings with kWKNetworkProxyModeCustom and an empty proxyUrl.

Got it.

>> Source/WebKit/UIProcess/API/C/curl/WKContextCurl.cpp:30
>> +#include "WebProcessPool.h"
> 
> Remove this blank line.
> https://webkit.org/code-style-guidelines/#include-system

Got it.

>> Source/WebKit/UIProcess/API/C/curl/WKContextCurl.cpp:41
>> +    case WKNetworkProxyMode::kWKNetworkProxyModeDefault :
> 
> No space before :

Got it.

>> Source/WebKit/UIProcess/API/C/curl/WKContextCurl.h:43
>> +WK_EXPORT void WKContextSetSslCACertPath(WKContextRef, WKStringRef caCertPath);
> 
> I have two problems with this:
> 1) This should be per WKWebsiteDataStoreRef, which corresponds with a NetworkSession.  It may be tempting to add things to WKContext, but we're in the process of moving as much as we can away from WKContext.
> 2) I'm not sure this is a good design to expose these things via the API.  I think instead the cacert data should just be included in the bundle of files and resources that constitute WebKit.  There should definitely be a good default so that API users don't have to use this in order to get something working.

1) I see. I try moving them to WKWebsiteDataStoreRef.
2) From our system's requirement, we have to pass root ca information supplied by the library client.

>> Source/WebKit/UIProcess/API/C/curl/WKContextCurl.h:44
>> +WK_EXPORT WKStringRef WKContextGetSslCACertPath(WKContextRef);
> 
> You are using toCopiedAPI to return a copy of string. You should name an API Copy instead of Get if you return a copy of string in WK API.
> For example, WKPageCopyTitle.
> And, You should name SSL instead of Ssl, for exameple WKContextSetAllowsAnySSLCertificateForWebSocketTesting.
> WKContextGetSslCACertPath -> WKContextCopySSLCACertPath

I see. Thanks.

>> Source/WebKit/UIProcess/win/WebProcessPoolWin.cpp:33
>> +
> 
> Remove this blank line.
> https://webkit.org/code-style-guidelines/#include-system

Got it.

[Basuke Suzuki]

Alex,

we discussed about this and change our direction as removing SSL settings API. Then move those initialization into platform (i.e. Win, our platform) specific initialization when SSLHandle is initialized.

The rest of patch is proxy setting API. Do you think those API also should also move to WKWebsiteDataStore? It seems Apple port doesn't have such, I want to hear your opinion. We need this proxy support on our platform by following WebKit manner.

[Alex Christensen]

If you need proxy settings, put them on WebsiteDataStore, not WebProcessPool.

[Basuke Suzuki]

(In reply to Alex Christensen from comment #14)
> If you need proxy settings, put them on WebsiteDataStore, not WebProcessPool.

Thanks.

[Alex Christensen]

By the way, on Apple platforms the proxy settings are configured system-wide or per-app underneath us, so we don't have to have that code in WebKit.  I'm not sure what the libsoup code does.

[Basuke Suzuki]

(In reply to Alex Christensen from comment #16)
> By the way, on Apple platforms the proxy settings are configured system-wide
> or per-app underneath us, so we don't have to have that code in WebKit.  I'm
> not sure what the libsoup code does.

Yep, that's why I asked to hear your opinion because I cannot find that in public. Gtk put the api in webcontext. That's why we've added in WebContext. But we follow your new design choice.

[Alex Christensen]

GTK should follow, too.

[Michael Catanzaro]

We store proxy settings in the SoupNetworkSession object. That's where we put all our platform-specific NetworkProcess configuration.

In the public API, they're exposed via WebKitWebContext, which of course we can't change.

[Basuke Suzuki]

Created attachment 352390 [details]
WIP Patch

Alex, can you review is this the right direction to add proxy API?

[Michael Catanzaro]

Comment on attachment 352390 [details]
WIP Patch

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

> Source/WebCore/platform/network/NetworkStorageSession.h:138
> +    WEBCORE_EXPORT void setProxySettings(const CurlProxySettings&) const;

Probably should be CurlProxySettings&& since you are sinking the data (caller doesn't need it anymore), right?

> Source/WebKit/NetworkProcess/NetworkProcess.h:306
> +    void setNetworkProxySettings(PAL::SessionID, const WebCore::CurlProxySettings&);

Ditto.

> Source/WebKit/NetworkProcess/curl/NetworkProcessCurl.cpp:89
> +        networkStorageSession->setProxySettings(settings);

WTFMove?

> Source/WebKit/NetworkProcess/curl/NetworkSessionCurl.cpp:40
> +    networkStorageSession().setProxySettings(parameters.proxySettings);

WTFMove!

> Source/WebKit/NetworkProcess/curl/RemoteNetworkingContextCurl.cpp:45
> +    SessionTracker::setSession(sessionID, NetworkSession::create(WTFMove(parameters.networkSessionParameters)));

Yeah like this ;)

[Basuke Suzuki]

Comment on attachment 352390 [details]
WIP Patch

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

Thanks, Michael!

>> Source/WebCore/platform/network/NetworkStorageSession.h:138
>> +    WEBCORE_EXPORT void setProxySettings(const CurlProxySettings&) const;
> 
> Probably should be CurlProxySettings&& since you are sinking the data (caller doesn't need it anymore), right?

Right.

>> Source/WebKit/NetworkProcess/curl/RemoteNetworkingContextCurl.cpp:45
>> +    SessionTracker::setSession(sessionID, NetworkSession::create(WTFMove(parameters.networkSessionParameters)));
> 
> Yeah like this ;)

Yeag! :D

[Basuke Suzuki]

Created attachment 353191 [details]
PATCH

Implement API for proxy configuration. Also add UI for Proxy on MiniBrowser.

[EWS Watchlist]

Attachment 353191 [details] did not pass style-queue:


ERROR: Tools/MiniBrowser/win/MiniBrowserLibResource.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
Total errors found: 1 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Basuke Suzuki]

(In reply to Build Bot from comment #24)
> Attachment 353191 [details] did not pass style-queue:
> 
> 
> ERROR: Tools/MiniBrowser/win/MiniBrowserLibResource.h:0:  No copyright
> message found.  You should have a line: "Copyright [year] <Copyright Owner>"
> [legal/copyright] [5]
> Total errors found: 1 in 31 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

This file is generated and managed by VisualStudio.

[Darin Adler]

(In reply to Basuke Suzuki from comment #25)
> (In reply to Build Bot from comment #24)
> > ERROR: Tools/MiniBrowser/win/MiniBrowserLibResource.h:0:  No copyright
> > message found.  You should have a line: "Copyright [year] <Copyright Owner>"
> > [legal/copyright] [5]
> > Total errors found: 1 in 31 files
> > 
> > If any of these errors are false positives, please file a bug against
> > check-webkit-style.
> 
> This file is generated and managed by VisualStudio.

So someone needs to update check-webkit-style to understand that.

[Basuke Suzuki]

(In reply to Darin Adler from comment #26)
> (In reply to Basuke Suzuki from comment #25)
> > (In reply to Build Bot from comment #24)
> > > ERROR: Tools/MiniBrowser/win/MiniBrowserLibResource.h:0:  No copyright
> > > message found.  You should have a line: "Copyright [year] <Copyright Owner>"
> > > [legal/copyright] [5]
> > > Total errors found: 1 in 31 files
> > > 
> > > If any of these errors are false positives, please file a bug against
> > > check-webkit-style.
> > 
> > This file is generated and managed by VisualStudio.
> 
> So someone needs to update check-webkit-style to understand that.

I'll do next week.

[youenn fablet]

Comment on attachment 353191 [details]
PATCH

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

> Source/WebKit/NetworkProcess/NetworkProcess.messages.in:37
> +    SetNetworkProxySettings(PAL::SessionID sessionID, WebCore::CurlProxySettings settings)

sessionID makes sense but soup equivalent is not doing so.
I guess some GTK refactoring could be done based on that patch.

> Source/WebKit/NetworkProcess/curl/NetworkProcessCurl.cpp:88
> +    if (auto* networkStorageSession = NetworkStorageSession::storageSession(sessionID))

Can there be a case where we do not have any networkStorageSession?
Should we ASSERT?

> Source/WebKit/Shared/curl/WebCoreArgumentCodersCurl.cpp:133
> +    encoder.encodeEnum(settings.mode());

We now use EnumTraits to encode the enum without encodeEnum.
You should then be able to write: encoder << settings.mode();

> Source/WebKit/Shared/curl/WebCoreArgumentCodersCurl.cpp:141
> +bool ArgumentCoder<CurlProxySettings>::decode(Decoder& decoder, CurlProxySettings& settings)

We now try to write decoders as:
std::optional<CurlProxySettings> ArgumentCoder<CurlProxySettings>::decode(Decoder& decoder)
Can that be done here?

> Source/WebKit/Shared/curl/WebCoreArgumentCodersCurl.cpp:152
> +    String url;

I initially thought url should be URL but it is not.
I wonder though whether it should be and if so if CurlProxySettings::url() should return a const URL&.

> Source/WebKit/UIProcess/WebsiteData/curl/WebsiteDataStoreCurl.cpp:40
> +void WebsiteDataStore::setNetworkProxySettings(const WebCore::CurlProxySettings& proxySettings)

Could be WebCore::CurlProxySettings&&?

> Tools/MiniBrowser/win/Common.cpp:135
> +        void setup() override

override -> final here and below?

> Tools/MiniBrowser/win/WebKitBrowserWindow.cpp:130
> +        WKWebsiteDataStoreDisableNetworkProxySettings(store);

Maybe could be rewritten to:
if (!m_proxy.enable) {
    WKWebsiteDataStoreDisableNetworkProxySettings(store);
    return;
}

[Michael Catanzaro]

(In reply to youenn fablet from comment #28)
> sessionID makes sense but soup equivalent is not doing so.
> I guess some GTK refactoring could be done based on that patch.

Thing is, WPE/GTK has no concept of network sessions: there's always exactly one per UI process. For this reason, I've never entirely understood why SessionID exists. I guess it's to allow a separate network session, e.g. for ephemeral views? (Epiphany always launches a new UI process for ephemeral mode, which is why I've never paid attention to this.)

[Basuke Suzuki]

Comment on attachment 353191 [details]
PATCH

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

Thanks for reviewing!

>> Source/WebKit/NetworkProcess/curl/NetworkProcessCurl.cpp:88
>> +    if (auto* networkStorageSession = NetworkStorageSession::storageSession(sessionID))
> 
> Can there be a case where we do not have any networkStorageSession?
> Should we ASSERT?

I need to confirm this, but I believe it shouldn't be nil. Maybe ASSERT is good.

>> Source/WebKit/Shared/curl/WebCoreArgumentCodersCurl.cpp:133
>> +    encoder.encodeEnum(settings.mode());
> 
> We now use EnumTraits to encode the enum without encodeEnum.
> You should then be able to write: encoder << settings.mode();

Okay, I'll check that and try rewriting with it.

>> Source/WebKit/Shared/curl/WebCoreArgumentCodersCurl.cpp:141
>> +bool ArgumentCoder<CurlProxySettings>::decode(Decoder& decoder, CurlProxySettings& settings)
> 
> We now try to write decoders as:
> std::optional<CurlProxySettings> ArgumentCoder<CurlProxySettings>::decode(Decoder& decoder)
> Can that be done here?

Got it. Trying.

>> Source/WebKit/UIProcess/WebsiteData/curl/WebsiteDataStoreCurl.cpp:40
>> +void WebsiteDataStore::setNetworkProxySettings(const WebCore::CurlProxySettings& proxySettings)
> 
> Could be WebCore::CurlProxySettings&&?

Right.

>> Tools/MiniBrowser/win/Common.cpp:135
>> +        void setup() override
> 
> override -> final here and below?

Ah, okay. That's better.

>> Tools/MiniBrowser/win/WebKitBrowserWindow.cpp:130
>> +        WKWebsiteDataStoreDisableNetworkProxySettings(store);
> 
> Maybe could be rewritten to:
> if (!m_proxy.enable) {
>     WKWebsiteDataStoreDisableNetworkProxySettings(store);
>     return;
> }

Okay, the point is stop nesting deep and use early return, right?

[Basuke Suzuki]

Comment on attachment 353191 [details]
PATCH

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

>> Source/WebKit/Shared/curl/WebCoreArgumentCodersCurl.cpp:152
>> +    String url;
> 
> I initially thought url should be URL but it is not.
> I wonder though whether it should be and if so if CurlProxySettings::url() should return a const URL&.

You are right. These are our mistake. Thanks. It should be String at anywhere. We'll change APIs and method calls to use String instead of URL. 

The background of this was here https://bugs.webkit.org/show_bug.cgi?id=184714#c11.

Summary:
There's no way to specify port 80 as a http proxy port. libcurl treat default port of http proxy to 1080 if port number is omitted. Once a string is stored in URL, port 80 is removed form the URL.

[Basuke Suzuki]

Comment on attachment 353191 [details]
PATCH

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

>>> Source/WebKit/NetworkProcess/curl/NetworkProcessCurl.cpp:88
>>> +    if (auto* networkStorageSession = NetworkStorageSession::storageSession(sessionID))
>> 
>> Can there be a case where we do not have any networkStorageSession?
>> Should we ASSERT?
> 
> I need to confirm this, but I believe it shouldn't be nil. Maybe ASSERT is good.

I still am investigating this, but this kind of pattern are used in many place. https://github.com/WebKit/webkit/blob/5ebf9a85efa6c334e5a7d68bdd7d8cc4396b30a9/Source/WebKit/NetworkProcess/NetworkProcess.cpp#L548
    if (auto* networkStorageSession = NetworkStorageSession::storageSession(sessionID))
        networkStorageSession->removeAllStorageAccess();
    else
        ASSERT_NOT_REACHED();
I don't see why, but it seems reasonable to follow this pattern.

[Basuke Suzuki]

Comment on attachment 353191 [details]
PATCH

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

>>> Source/WebKit/Shared/curl/WebCoreArgumentCodersCurl.cpp:152
>>> +    String url;
>> 
>> I initially thought url should be URL but it is not.
>> I wonder though whether it should be and if so if CurlProxySettings::url() should return a const URL&.
> 
> You are right. These are our mistake. Thanks. It should be String at anywhere. We'll change APIs and method calls to use String instead of URL. 
> 
> The background of this was here https://bugs.webkit.org/show_bug.cgi?id=184714#c11.
> 
> Summary:
> There's no way to specify port 80 as a http proxy port. libcurl treat default port of http proxy to 1080 if port number is omitted. Once a string is stored in URL, port 80 is removed form the URL.

No I was wrong. That was for libcurl implementation. We chose to act like standard. We translate the meaning of port and a user shouldn't care about this weird behavior of libcurl. Then these arguments are whole WebCore::URL and it should be used here.

[Basuke Suzuki]

Created attachment 353430 [details]
FIX

Fixed Youenn's reviewed points.

[EWS Watchlist]

Attachment 353430 [details] did not pass style-queue:


ERROR: Tools/MiniBrowser/win/MiniBrowserLibResource.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
Total errors found: 1 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Basuke Suzuki]

Review please? Youenn?

[youenn fablet]

Comment on attachment 353430 [details]
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.

[Basuke Suzuki]

Comment on attachment 353430 [details]
FIX

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

Thanks! Okay, try to put test in TestWebKitAPI.

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

Right. We'll change.

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

Okay.

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

Okay. I'll think about that.

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

Got it.

>> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:198
>> +    const WebCore::CurlProxySettings& networkProxySettings() { return m_proxySettings; }
> 
> Should be const probably.

Right.

[Basuke Suzuki]

Created attachment 354367 [details]
PATCH

[Basuke Suzuki]

Comment on attachment 354367 [details]
PATCH

We'll open a separate bug to add API test for this change. Thanks Youenn for r+.

[WebKit Commit Bot]

Comment on attachment 354367 [details]
PATCH

Clearing flags on attachment: 354367

Committed r238051: <https://trac.webkit.org/changeset/238051>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/45953089>