Bug 128674 – [GTK] Provide API to set proxy settings

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Sami Wagiaalla]

When proxy settings are set in Eclipse or through Java properties, the Eclipse SWT Browser needs to forward those settings to WebKit.

In WebKitGTK this was accomplished by getting the soup session through the webkit_get_default_session() global function then setting the "proxy-uri" property of the SoupSession.

Can similar (or more appropriate) API be provided in WebKitGTK2 ?

[Jiří Janoušek]

I second this feature request as a possibility to set different proxy server than specified in GNOME settings for whole system is crucial for some users of my application Nuvola Player.

Although I've found an ugly hack* how to disable the default proxy resolver and set custom proxy server, I would appreciate some nice API to set "proxy-uri" property of the SoupSession.

Thanks for your work!

*The hack:

- Unset env variables GNOME_DESKTOP_SESSION_ID and DESKTOP_SESSION
- Set env variables http_proxy and https_proxy.
- See https://git.gnome.org/browse/glib-networking/tree/proxy/gnome/gproxyresolvergnome.c

[Michael Catanzaro]

The SoupSession lives in the NetworkProcess, so we can't add any API to access it directly. But adding API just to set the proxy-uri property should be straightforward.

[Michael Catanzaro]

FYI for EFL folks: there is a comment in SoupNetworkSession.cpp that implies that the EFL port does not want to use the default GProxyResolver, like GTK does. But I think you really are getting the default GProxyResolver. SoupNetworkSession::setHTTPProxy is designed to remove the default GProxyResolver if called with nullptr, but you never do this, instead you return early in SoupNetworkSession::setupHTTPProxyFromEnvironment if the http_proxy environment variable is unset. I'm going to preserve the current behavior and remove the misleading comment.

[Michael Catanzaro]

(In reply to comment #1) 
> *The hack:
> 
> - Unset env variables GNOME_DESKTOP_SESSION_ID and DESKTOP_SESSION
> - Set env variables http_proxy and https_proxy.
> - See
> https://git.gnome.org/browse/glib-networking/tree/proxy/gnome/
> gproxyresolvergnome.c

Fair warning: it's not practical to safely use setenv() in a threaded application except at the very beginning of main() before any secondary threads are created, since one thread could be calling setenv() (or Environment.set_variable() in Vala) at the same time another is calling getenv(). It's not some theoretical issue, we've had multiple cases in GNOME, including one where the entire desktop session crashes (GNOME #754951).

[Michael Catanzaro]

Created attachment 285302 [details]
Patch

[Michael Catanzaro]

This WIP patch is not ready for review. There are two problems:

 * I want to write a real test using a SoupServer.
 * I've verified using libsoup's example simple-proxy test application and a modified Epiphany that it doesn't actually work, need to debug.

[Michael Catanzaro]

Comment on attachment 285302 [details]
Patch

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

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:1270
> +    context->priv->proxyURI = String::fromUTF8(uri);

Also I forgot to return early here if it's the same as the previous value.

[Michael Catanzaro]

Comment on attachment 285302 [details]
Patch

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

> Source/WebKit2/UIProcess/soup/WebProcessPoolSoup.cpp:51
> +    if (networkProcess())

Actually the code to set the proxy works fine in general, but there is a race if called right after the WebKitWebContext is created (as I did in my original testing), then this check fails and the proxy never gets changed. I'm not sure how to fix it.

[Nick White]

Thanks for your work on this, Michael. Naive, I expect, but would adding a ensureNetworkProcess() call before networkProcess() do the trick?

[Michael Catanzaro]

(In reply to comment #9)
> Thanks for your work on this, Michael. Naive, I expect, but would adding a
> ensureNetworkProcess() call before networkProcess() do the trick?

That fixes it, thanks. :)

But it reveals we have a bug with our TLS errors policy message, as I was copying from that. Bug #160658.

[Michael Catanzaro]

(In reply to comment #10)
> But it reveals we have a bug with our TLS errors policy message, as I was
> copying from that. Bug #160658.

Not a bug, because we save the state in WebProcessPool::m_ignoreTLSErrors and send it along with NetworkProcessCreationParameters. We should do that here to allow creating the network process with the right proxy settings from the start.

[Michael Catanzaro]

Created attachment 287905 [details]
Patch

[Michael Catanzaro]

Created attachment 287906 [details]
Patch

[Michael Catanzaro]

Please let me know if this API is sufficient for Eclipse and Nuvola Player.

[Michael Catanzaro]

Created attachment 287907 [details]
Patch

[Jiří Janoušek]

Does setting the proxy-uri to "direct://" lead to a direct connection without any proxy?

[Michael Catanzaro]

(In reply to comment #16)
> Does setting the proxy-uri to "direct://" lead to a direct connection
> without any proxy?

No, but I considered that after uploading this patch. I could implement it if desired. (To get that with SoupSession, you would unset both the proxy-uri and proxy-resolver properties. For our API, I prefer that unsetting the URI restore the default proxy resolver.)

[Michael Catanzaro]

Actually, in my quick testing, it looks like passing "direct://" does work without any special code in WebKit.

[Carlos Garcia Campos]

Comment on attachment 287907 [details]
Patch

The problem I see with this API is that it doesn't scale. It only allows to set a default proxy URI. If we want to add more API later to allow setting more proxies, for HTTPS, ftp, etc. or to allow passing an ignore list, we would be adding more API to the web context. I think it would be better to add a WebKitProxyManager or something like that, that can be extended easily. It could event have the same API than the simple uri resolver:

webkit_proxy_manager_new(const gchar *default_proxy, gchar **ignore_hosts);
webkit_proxy_manager_set_default_proxy(WebKitProxyManager *, const gchar *default_proxy);
webkit_proxy_manager_set_ignore_hosts(WebKitProxyManager *, gchar **ignore_hosts);
webkit_proxy_manager_set_uri_proxy(WebKitProxyManager *, const gchar *scheme, const gchar *proxy);

That should be enough to cover all cases. Then the user would simply create a proxy manager and set it to the context.

[Martin Robinson]

(In reply to comment #19)

> That should be enough to cover all cases. Then the user would simply create
> a proxy manager and set it to the context.


Would it make sense for this API to simply accept a GProxyResolver from GIO?

[Carlos Garcia Campos]

(In reply to comment #20)
> (In reply to comment #19)
> 
> > That should be enough to cover all cases. Then the user would simply create
> > a proxy manager and set it to the context.
> 
> 
> Would it make sense for this API to simply accept a GProxyResolver from GIO?

I think that would be more confusing, because you could expect a GProxyResolver to resolve, but this is just a configuration object. Maybe Manager is not the right word for the same reason.

[Martin Robinson]

(In reply to comment #21)
> (In reply to comment #20)
> > (In reply to comment #19)
> > 
> > > That should be enough to cover all cases. Then the user would simply create
> > > a proxy manager and set it to the context.
> > 
> > 
> > Would it make sense for this API to simply accept a GProxyResolver from GIO?
> 
> I think that would be more confusing, because you could expect a
> GProxyResolver to resolve, but this is just a configuration object. Maybe
> Manager is not the right word for the same reason.

Hrm. Perhaps GSimpleProxyResolver then, which is also more-or-less a configuration object.

[Carlos Garcia Campos]

(In reply to comment #22)
> (In reply to comment #21)
> > (In reply to comment #20)
> > > (In reply to comment #19)
> > > 
> > > > That should be enough to cover all cases. Then the user would simply create
> > > > a proxy manager and set it to the context.
> > > 
> > > 
> > > Would it make sense for this API to simply accept a GProxyResolver from GIO?
> > 
> > I think that would be more confusing, because you could expect a
> > GProxyResolver to resolve, but this is just a configuration object. Maybe
> > Manager is not the right word for the same reason.
> 
> Hrm. Perhaps GSimpleProxyResolver then, which is also more-or-less a
> configuration object.

No, it's the same thing, GProxyResolver is not an object, it's an interface, the object is GSimpleProxyResolver, that implements GProxyResolver

[Michael Catanzaro]

I like Carlos's suggestion. I think it makes more sense to duplicate the GSimpleProxyResolver API (as he suggests) than to allow setting an existing GSimpleProxyResolver. That way, our code won't be broken if GSimpleProxyResolver changes in the future and we don't support the new API yet.

We cannot accept a generic GProxyResolver as it can't be serialized; it would require that we move proxy resolution to the UI process where the GProxyResolver exists, which would be nuts.

[Carlos Garcia Campos]

Created attachment 298951 [details]
Patch

In the end I decided to use WebKitNetworkProxySettings (instead of Manager), and make it a simple boxed type, since it just stores proxy settings that are normally set only once.

[Carlos Garcia Campos]

Created attachment 298953 [details]
Patch

[Michael Catanzaro]

Comment on attachment 298953 [details]
Patch

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

It looks great as usual, and it's much better than my API, thanks. Glad that at least my API test was still useful. ;)

Would be good for Sergio to review it too.

> Source/WebCore/platform/network/soup/SoupNetworkProxySettings.h:37
> +    SoupNetworkProxySettings(Mode proxyMode = Mode::Default)

This needs to be declared explicit because we don't want an implicit conversion from SoupNetworkProxySettings::Mode to SoupNetworkProxySettings.

> Source/WebCore/platform/network/soup/SoupNetworkProxySettings.h:57
> +        proxyMap = other.proxyMap;
> +
> +        return *this;

Nit: do you really want this blank line?

> Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:238
> +    case SoupNetworkProxySettings::Mode::NoProxy:
> +        break;

I would add a comment to indicate why this works (I see resolver.get() returns nullptr in the call to g_object_set() down below) since it's not clear otherwise.

> Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:254
> +void SoupNetworkSession::setProxySettingsFromEnvironment()

I was inclined to retain the #if PLATFORM(EFL) guards because this function should not exist at all and we don't want to accidentally use it in other ports. The correct way to set proxy settings from the environment is to use a GProxyResolver that does so. It also lacks the rather important https_proxy and ftp_proxy variables, and the uppercase versions of all four variables, all of which you almost surely want to be respected if you're setting http_proxy, and all of which would be supported via the default proxy resolver in non-GNOME/Ubuntu environments (at least, I think that's right). Additionally, it is incorrect for WebKit to respect this environment variable when running in a GNOME or Ubuntu environment, where GNOME proxy configuration should be respected instead. The only reason to retain this function is to not alter the incorrect behavior for EFL.

> Source/WebKit2/NetworkProcess/soup/NetworkProcessSoup.cpp:172
> +        if (auto* soupSession = session.soupNetworkSession())
> +            soupSession->setupProxy();

Nit: I would name this variable session -- or maybe networkSession -- instead of soupSession.

> Source/WebKit2/Shared/soup/WebCoreArgumentCodersSoup.cpp:163
> +    uint64_t ignoreHostsCount = settings.ignoreHosts ? static_cast<uint64_t>(g_strv_length(settings.ignoreHosts.get())) : 0;

You should use normal unsigned int here, since I do not expect anyone will ever define more than four billion different hosts for which to ignore proxy settings, and since g_strv_length returns a 32-bit guint anyway...

> Source/WebKit2/Shared/soup/WebCoreArgumentCodersSoup.cpp:166
> +        for (unsigned i = 0; settings.ignoreHosts.get()[i]; ++i)

...and since you're using unsigned here as well.

> Source/WebKit2/Shared/soup/WebCoreArgumentCodersSoup.cpp:183
> +    uint64_t ignoreHostsCount;

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitNetworkProxySettings.cpp:48
> +    _WebKitNetworkProxySettings(const SoupNetworkProxySettings& otherSettings)

Again, it should be explicit to avoid implicit conversions from SoupNetworkProxySettings to _WebKitNetworkProxySettings. You almost always want to use explicit for any constructor that can be called with exactly one argument in any C++ program.

> Source/WebKit2/UIProcess/API/gtk/WebKitNetworkProxySettings.cpp:98
> + * to connections made to hosts identified by address. That is, if example.com has an address of 192.168.1.1, and the :ignore-hosts list

What is the colon in :ignore-hosts for? Should you write "and @ignore_hosts contains" instead?

> Source/WebKit2/UIProcess/API/gtk/WebKitNetworkProxySettings.h:50
> +webkit_network_proxy_settings_add_proxy (WebKitNetworkProxySettings *proxy_settings,

This is not the best name for this function. I see that it is a wrapper for g_simple_proxy_resolver_set_uri_proxy(), which is also not an ideal name. How about webkit_network_proxy_settings_add_proxy_for_scheme() or webkit_network_proxy_settings_add_proxy_for_uri_scheme()?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:585
> + * Set the network proxy settings to be used by connections started by @context.

"started by @context" -> "started in @context"

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:592
> + * #WebKitNetworkProxySettings as @proxy_settings.

And if @proxy_mode is not %WEBKIT_NETWORK_PROXY_MODE_CUSTOM?

"When @proxy_mode is %WEBKIT_NETWORK_PROXY_MODE_CUSTOM @proxy_settings must be a valid #WebKitNetworkProxySettings; otherwise, @proxy_settings must be %NULL."

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:604
> +        priv->processPool->setNetworkProxySettings(WebCore::SoupNetworkProxySettings());

Can you not pass an empty initializer list { }? I think you can, since there is a constructor that accepts no arguments (the ProxyMode constructor with a default value). The empty initializer list is preferred WebKit style nowadays. At least, I think that's what Darin would write:

priv->processPool->setNetworkProxySettings({ });

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:607
> +        priv->processPool->setNetworkProxySettings(WebCore::SoupNetworkProxySettings(WebCore::SoupNetworkProxySettings::Mode::NoProxy));

And here would be priv->processPool->setNetworkProxySettings({ WebCore::SoupNetworkProxySettings::Mode::NoProxy });

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:613
> +            g_warning("Attempting to set custom network proxy settings with an empty WebKitNetworkProxySettings, use "
> +                "WEBKIT_NETWORK_PROXY_MODE_NO_PROXY to not use any proxy or WEBKIT_NETWORK_PROXY_MODE_DEFAULT to use the default system settings");

"Invalid attempt to set custom network proxy settings with an empty WebKitNetworkProxySettings. Use..."

> Source/WebKit2/WebProcess/soup/WebProcessSoup.cpp:57
> +        if (auto* soupSession = session.soupNetworkSession())
> +            soupSession->setupProxy();

Ditto (regarding variable name)

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebContext.cpp:725
> +    ~ProxyTest()
> +    {
> +    }

Shouldn't this be omitted?

[Michael Catanzaro]

(In reply to comment #27)
> > Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:254
> > +void SoupNetworkSession::setProxySettingsFromEnvironment()
> 
> I was inclined to retain the #if PLATFORM(EFL) guards because this function
> should not exist at all and we don't want to accidentally use it in other
> ports. The correct way to set proxy settings from the environment is to use
> a GProxyResolver that does so. It also lacks the rather important
> https_proxy and ftp_proxy variables, and the uppercase versions of all four
> variables, all of which you almost surely want to be respected if you're
> setting http_proxy, and all of which would be supported via the default
> proxy resolver in non-GNOME/Ubuntu environments (at least, I think that's
> right). Additionally, it is incorrect for WebKit to respect this environment
> variable when running in a GNOME or Ubuntu environment, where GNOME proxy
> configuration should be respected instead. The only reason to retain this
> function is to not alter the incorrect behavior for EFL.

Now would be a good time to either fix this function to respect the same set of environment variables as libproxy (detailed above), or -- much better -- just remove it.

[Carlos Garcia Campos]

(In reply to comment #27)
> Comment on attachment 298953 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=298953&action=review
> 
> It looks great as usual, and it's much better than my API, thanks. Glad that
> at least my API test was still useful. ;)

Thanks.
 
> Would be good for Sergio to review it too.

Indeed.

> > Source/WebCore/platform/network/soup/SoupNetworkProxySettings.h:37
> > +    SoupNetworkProxySettings(Mode proxyMode = Mode::Default)
> 
> This needs to be declared explicit because we don't want an implicit
> conversion from SoupNetworkProxySettings::Mode to SoupNetworkProxySettings.

Ok.

> > Source/WebCore/platform/network/soup/SoupNetworkProxySettings.h:57
> > +        proxyMap = other.proxyMap;
> > +
> > +        return *this;
> 
> Nit: do you really want this blank line?

No, it doesn't hurt either, I guess.

> > Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:238
> > +    case SoupNetworkProxySettings::Mode::NoProxy:
> > +        break;
> 
> I would add a comment to indicate why this works (I see resolver.get()
> returns nullptr in the call to g_object_set() down below) since it's not
> clear otherwise.

I agree, I thought about it and then I forgot.

> > Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:254
> > +void SoupNetworkSession::setProxySettingsFromEnvironment()
> 
> I was inclined to retain the #if PLATFORM(EFL) guards because this function
> should not exist at all and we don't want to accidentally use it in other
> ports. The correct way to set proxy settings from the environment is to use
> a GProxyResolver that does so. It also lacks the rather important
> https_proxy and ftp_proxy variables, and the uppercase versions of all four
> variables, all of which you almost surely want to be respected if you're
> setting http_proxy, and all of which would be supported via the default
> proxy resolver in non-GNOME/Ubuntu environments (at least, I think that's
> right). Additionally, it is incorrect for WebKit to respect this environment
> variable when running in a GNOME or Ubuntu environment, where GNOME proxy
> configuration should be respected instead. The only reason to retain this
> function is to not alter the incorrect behavior for EFL.

Ok, I'll keep the ifdefs then to make it clear we never want to use that in ohter soup base ports.

> > Source/WebKit2/NetworkProcess/soup/NetworkProcessSoup.cpp:172
> > +        if (auto* soupSession = session.soupNetworkSession())
> > +            soupSession->setupProxy();
> 
> Nit: I would name this variable session -- or maybe networkSession --
> instead of soupSession.

session is the NetworkStorageSession. This soupSession is an instance of SoupNetworkSession, so I don't see what's wrong with soupSession name.

> > Source/WebKit2/Shared/soup/WebCoreArgumentCodersSoup.cpp:163
> > +    uint64_t ignoreHostsCount = settings.ignoreHosts ? static_cast<uint64_t>(g_strv_length(settings.ignoreHosts.get())) : 0;
> 
> You should use normal unsigned int here, since I do not expect anyone will
> ever define more than four billion different hosts for which to ignore proxy
> settings, and since g_strv_length returns a 32-bit guint anyway...

Ok, I'll use uint32_t then.

> > Source/WebKit2/Shared/soup/WebCoreArgumentCodersSoup.cpp:166
> > +        for (unsigned i = 0; settings.ignoreHosts.get()[i]; ++i)
> 
> ...and since you're using unsigned here as well.

Right.

> > Source/WebKit2/Shared/soup/WebCoreArgumentCodersSoup.cpp:183
> > +    uint64_t ignoreHostsCount;
> 
> Ditto.
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitNetworkProxySettings.cpp:48
> > +    _WebKitNetworkProxySettings(const SoupNetworkProxySettings& otherSettings)
> 
> Again, it should be explicit to avoid implicit conversions from
> SoupNetworkProxySettings to _WebKitNetworkProxySettings. You almost always
> want to use explicit for any constructor that can be called with exactly one
> argument in any C++ program.

Not sure that can happen here, this is actually the private struct of the boxed type, it can only be created here.

> > Source/WebKit2/UIProcess/API/gtk/WebKitNetworkProxySettings.cpp:98
> > + * to connections made to hosts identified by address. That is, if example.com has an address of 192.168.1.1, and the :ignore-hosts list
> 
> What is the colon in :ignore-hosts for? Should you write "and @ignore_hosts
> contains" instead?

This is me copy-pasting the documentation from glib :-)

> > Source/WebKit2/UIProcess/API/gtk/WebKitNetworkProxySettings.h:50
> > +webkit_network_proxy_settings_add_proxy (WebKitNetworkProxySettings *proxy_settings,
> 
> This is not the best name for this function. I see that it is a wrapper for
> g_simple_proxy_resolver_set_uri_proxy(), which is also not an ideal name.
> How about webkit_network_proxy_settings_add_proxy_for_scheme() or
> webkit_network_proxy_settings_add_proxy_for_uri_scheme()?

Too much typing! :-D I agree it's a better name, though.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:585
> > + * Set the network proxy settings to be used by connections started by @context.
> 
> "started by @context" -> "started in @context"

Ok.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:592
> > + * #WebKitNetworkProxySettings as @proxy_settings.
> 
> And if @proxy_mode is not %WEBKIT_NETWORK_PROXY_MODE_CUSTOM?
> 
> "When @proxy_mode is %WEBKIT_NETWORK_PROXY_MODE_CUSTOM @proxy_settings must
> be a valid #WebKitNetworkProxySettings; otherwise, @proxy_settings must be
> %NULL."

Much better.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:604
> > +        priv->processPool->setNetworkProxySettings(WebCore::SoupNetworkProxySettings());
> 
> Can you not pass an empty initializer list { }? I think you can, since there
> is a constructor that accepts no arguments (the ProxyMode constructor with a
> default value). The empty initializer list is preferred WebKit style
> nowadays. At least, I think that's what Darin would write:
> 
> priv->processPool->setNetworkProxySettings({ });

Right, I'll try.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:607
> > +        priv->processPool->setNetworkProxySettings(WebCore::SoupNetworkProxySettings(WebCore::SoupNetworkProxySettings::Mode::NoProxy));
> 
> And here would be priv->processPool->setNetworkProxySettings({
> WebCore::SoupNetworkProxySettings::Mode::NoProxy });

Ditto.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:613
> > +            g_warning("Attempting to set custom network proxy settings with an empty WebKitNetworkProxySettings, use "
> > +                "WEBKIT_NETWORK_PROXY_MODE_NO_PROXY to not use any proxy or WEBKIT_NETWORK_PROXY_MODE_DEFAULT to use the default system settings");
> 
> "Invalid attempt to set custom network proxy settings with an empty
> WebKitNetworkProxySettings. Use..."

Much better too.

> > Source/WebKit2/WebProcess/soup/WebProcessSoup.cpp:57
> > +        if (auto* soupSession = session.soupNetworkSession())
> > +            soupSession->setupProxy();
> 
> Ditto (regarding variable name)

Don't understand the reasoning.

> > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebContext.cpp:725
> > +    ~ProxyTest()
> > +    {
> > +    }
> 
> Shouldn't this be omitted?

Yes.

[Carlos Garcia Campos]

(In reply to comment #27)
> Comment on attachment 298953 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=298953&action=review
> 
> It looks great as usual, and it's much better than my API, thanks. Glad that
> at least my API test was still useful. ;)
> 
> Would be good for Sergio to review it too.
> 
> > Source/WebCore/platform/network/soup/SoupNetworkProxySettings.h:37
> > +    SoupNetworkProxySettings(Mode proxyMode = Mode::Default)
> 
> This needs to be declared explicit because we don't want an implicit
> conversion from SoupNetworkProxySettings::Mode to SoupNetworkProxySettings.
> 

If I make this explicit, then I can't use { WebCore::SoupNetworkProxySettings::Mode::NoProxy } 

../../Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp: In function 'void webkit_web_context_set_network_proxy_settings(WebKitWebContext*, WebKitNetworkProxyMode, WebKitNetworkProxySettings*)':
../../Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:607:106: error: converting to 'const WebCore::SoupNetworkProxySettings' from initializer list would use explicit constructor 'WebCore::SoupNetworkProxySettings::SoupNetworkProxySettings(WebCore::SoupNetworkProxySettings::Mode)'

[Carlos Garcia Campos]

Created attachment 299024 [details]
Patch for landing

[Michael Catanzaro]

(In reply to comment #29)
> > I was inclined to retain the #if PLATFORM(EFL) guards because this function
> > should not exist at all and we don't want to accidentally use it in other
> > ports. The correct way to set proxy settings from the environment is to use
> > a GProxyResolver that does so. It also lacks the rather important
> > https_proxy and ftp_proxy variables, and the uppercase versions of all four
> > variables, all of which you almost surely want to be respected if you're
> > setting http_proxy, and all of which would be supported via the default
> > proxy resolver in non-GNOME/Ubuntu environments (at least, I think that's
> > right). Additionally, it is incorrect for WebKit to respect this environment
> > variable when running in a GNOME or Ubuntu environment, where GNOME proxy
> > configuration should be respected instead. The only reason to retain this
> > function is to not alter the incorrect behavior for EFL.
> 
> Ok, I'll keep the ifdefs then to make it clear we never want to use that in
> ohter soup base ports.

Maybe you should add my comment as a FIXME.

> > > Source/WebKit2/NetworkProcess/soup/NetworkProcessSoup.cpp:172
> > > +        if (auto* soupSession = session.soupNetworkSession())
> > > +            soupSession->setupProxy();
> > 
> > Nit: I would name this variable session -- or maybe networkSession --
> > instead of soupSession.
> 
> session is the NetworkStorageSession. This soupSession is an instance of
> SoupNetworkSession, so I don't see what's wrong with soupSession name.

Yeah, too many sessions... I didn't like the name "soupSession" because it is a SoupNetworkSession and not a SoupSession, but oh well.

> Not sure that can happen here, this is actually the private struct of the
> boxed type, it can only be created here.

True, but it's a good pattern to follow anyway.

(In reply to comment #30)
> If I make this explicit, then I can't use {
> WebCore::SoupNetworkProxySettings::Mode::NoProxy } 

Oh noooo... you should make it explicit anyway IMO as that conversion is undesired.

[Carlos Garcia Campos]

Created attachment 299121 [details]
Patch for landing

[Build Bot]

Comment on attachment 299121 [details]
Patch for landing

Attachment 299121 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2908621

New failing tests:
media/modern-media-controls/tracks-panel/tracks-panel-hide-click-outside.html

[Build Bot]

Created attachment 299127 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Sergio Villar Senin]

sgtm from the API POV

[Carlos Garcia Campos]

Committed r210921: <http://trac.webkit.org/changeset/210921>