Bug 118388 – [WK2][Efl][Soup] Make NetworkProcessMainUnix handle proxy settings.

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Kwang Yul Seo]

Proxy configuration should honor the no_proxy environment variable. See Bug 91747.

[Kwang Yul Seo]

Created attachment 206075 [details]
Patch

[Sergio Villar Senin]

(In reply to comment #1)
> Created an attachment (id=206075) [details]
> Patch

I don't really know very well the EFL environment but I wonder, isn't it using at least libproxy? Because if that's the case libsoup should use it as one of the backends for proxy handling, and libproxy should take care of the environment variable.

[Kwang Yul Seo]

(In reply to comment #2)
> I don't really know very well the EFL environment but I wonder, isn't it using at least libproxy? Because if that's the case libsoup should use it as one of the backends for proxy handling, and libproxy should take care of the environment variable.

libsoup-gnome is compiled against libproxy, so we don't need this code for Gtk port. But I am not sure about the EFL environment. I just copied the code from WebProcessMainEfl.cpp. Christophe, does EFL use libproxy? Do we still need this code?

[Chris Dumez]

(In reply to comment #3)
> (In reply to comment #2)
> > I don't really know very well the EFL environment but I wonder, isn't it using at least libproxy? Because if that's the case libsoup should use it as one of the backends for proxy handling, and libproxy should take care of the environment variable.
> 
> libsoup-gnome is compiled against libproxy, so we don't need this code for Gtk port. But I am not sure about the EFL environment. I just copied the code from WebProcessMainEfl.cpp. Christophe, does EFL use libproxy? Do we still need this code?

No the EFL port does not use libproxy. And yes, we still need this code.

[Mikhail Pozdnyakov]

Comment on attachment 206075 [details]
Patch

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

> Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp:66
> +        g_object_unref(resolver);

don't we have a smart pointer for g objects?

[Chris Dumez]

Comment on attachment 206075 [details]
Patch

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

>> Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp:66
>> +        g_object_unref(resolver);
> 
> don't we have a smart pointer for g objects?

It would require to add a GOwnPtr specialization for SoupProxyURIResolver type. I don't think it exists. Not sure it is worth it since it is used only by the EFL port and in one place.

[Mikhail Pozdnyakov]

(In reply to comment #6)
> (From update of attachment 206075 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=206075&action=review
> 
> >> Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp:66
> >> +        g_object_unref(resolver);
> > 
> > don't we have a smart pointer for g objects?
> 
> It would require to add a GOwnPtr specialization for SoupProxyURIResolver type. I don't think it exists. Not sure it is worth it since it is used only by the EFL port and in one place.

GRefPtr could be used since it's using

template <typename T> inline void derefGPtr(T* ptr)
{
    if (ptr)
        g_object_unref(ptr);
}

[Chris Dumez]

Comment on attachment 206075 [details]
Patch

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

>>>> Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp:66
>>>> +        g_object_unref(resolver);
>>> 
>>> don't we have a smart pointer for g objects?
>> 
>> It would require to add a GOwnPtr specialization for SoupProxyURIResolver type. I don't think it exists. Not sure it is worth it since it is used only by the EFL port and in one place.
> 
> GRefPtr could be used since it's using
> 
> template <typename T> inline void derefGPtr(T* ptr)
> {
>     if (ptr)
>         g_object_unref(ptr);
> }

Indeed, I did not notice it was a regular GObject. Would look better indeed to use GRefPtr then.

[Kwang Yul Seo]

(In reply to comment #8)
> Indeed, I did not notice it was a regular GObject. Would look better indeed to use GRefPtr then.

You are right. We can use GRefPtr here. 

However, I tried to keep the code untouched because I just copied it verbatim from WebProcessMainEfl.cpp.

Currently, there is much code duplication between NetworkProcess and WebProcess because the Mac port implements NetworkProcess by copying lots of code from WebProcess.

Once NetworkProcess is up and running, the next step should be removal of such code duplication bu moving common code to Shared.

I can change this patch to use GRefPtr, but then we should change the original source code in WebProcessMainEfl.cpp too.

[Kwang Yul Seo]

BTW, please review Bug 110136 first because this bug depends on it.

[Chris Dumez]

(In reply to comment #10)
> BTW, please review Bug 110136 first because this bug depends on it.

I think it requires a wk2 owner to sign off.

[Dan Winship]

see also bug 114683; ProxyResolverSoup can be removed

[Gustavo Noronha (kov)]

Comment on attachment 206075 [details]
Patch

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

> Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp:60
> +    // Only for EFL because GTK port uses the GNOME resolver which uses GNOME settings.

This is incorrect, we use the default resolver, which uses GIO's proxy resolver.

[Kwang Yul Seo]

(In reply to comment #13)
> This is incorrect, we use the default resolver, which uses GIO's proxy resolver.

Really? I got this information from Christophe Dumez's comment on Bug 91747 https://bugs.webkit.org/show_bug.cgi?id=91747#c7. I will fix the comment. 

BTW, it seems this patch is no more needed once Bug 114683 is resolved as Dan Winship mentioned in comment #12.

[Kwang Yul Seo]

Created attachment 206287 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 206287 [details]
Patch

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

> Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp:59
> +#if PLATFORM(EFL)

Would be nice if you use USE(SOUP) as well.

[Kwang Yul Seo]

Created attachment 207431 [details]
Patch

[Kwang Yul Seo]

(In reply to comment #16)
> (From update of attachment 206287 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=206287&action=review
> 
> > Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp:59
> > +#if PLATFORM(EFL)
> 
> Would be nice if you use USE(SOUP) as well.

Done.

This patch is ready for review now because its dependency Bug 110136 is resolved.

[Csaba Osztrogonác]

The patch is still up-to-date. Could you review it please?

[Csaba Osztrogonác]

Comment on attachment 207431 [details]
Patch

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

> Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp:66
> +    SoupSession* session = WebCore::ResourceHandle::defaultSession();

Just a note: bug118343 is going to add a session named variable to the same block. 
This one or the other one should be modified/removed before landing.

Maybe the best place would be before the #if guard in this patch.

[Csaba Osztrogonác]

Created attachment 212714 [details]
Patch

updated: one more include added, avoid the conflict with bug118343

[Csaba Osztrogonác]

ping?

[Brady Eidson]

Comment on attachment 212714 [details]
Patch

I'm fine with this if EFL/Soup reviewers are.

[Csaba Osztrogonác]

cc Carlos as SOUP reviewer

[Sergio Villar Senin]

Comment on attachment 212714 [details]
Patch

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

Looks sane to me if there is not a better way to do that with efl libraries yet

> Source/WebKit2/ChangeLog:10
> +        why this is needed.

I guess it's better to add a short description here instead of asking people to go through dozens of comments

[Carlos Garcia Campos]

Comment on attachment 212714 [details]
Patch

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

I don't know how the soup proxy resolver works, so If EFL devs are fine with this, it looks good to me too. Please consider moving the soup session initialization before landing. Thanks

> Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp:66
> +    SoupSession* session = WebCore::ResourceHandle::defaultSession();

If we are going to use soup unconditionally in this file, and considering that there's no other unix port now, maybe it makes more sense to rename this file as NetworkProcessSoup. Otherwise I would move this line inside the #if block. In any case, it seems that the Soup session is only used inside the if (httpProxy), so I would move this line inside the if in any case.

[Csaba Osztrogonác]

Comment on attachment 212714 [details]
Patch

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

>> Source/WebKit2/ChangeLog:10
>> +        why this is needed.
> 
> I guess it's better to add a short description here instead of asking people to go through dozens of comments

bug91747 was to make WebProcess honor no_proxy environment variable,
but NetworkProcess need it too not to change the current behaviour.

But you're right, this comment can be better, I'll fix it.

>> Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp:66
>> +    SoupSession* session = WebCore::ResourceHandle::defaultSession();
> 
> If we are going to use soup unconditionally in this file, and considering that there's no other unix port now, maybe it makes more sense to rename this file as NetworkProcessSoup. Otherwise I would move this line inside the #if block. In any case, it seems that the Soup session is only used inside the if (httpProxy), so I would move this line inside the if in any case.

NetworkProcessMainUnix.cpp was originally for all unix ports, like Qt, EFL, GTK and Nix. But Qt isn't in the trunk anymore
and the rest of the unix ports use soup, os renaming can be reasonable. But Nix is going to switch to curl in the near future,
so it would be better to leave this filename as is. However guarding the session variable is still necessary. I'll do it before
landing. But this session variable needs only USE(SOUP) guard, because it will be used in bug118343 too.

[Csaba Osztrogonác]

Created attachment 213981 [details]
Patch

blind patch for landing, will be checked locally before landing

[Csaba Osztrogonác]

(In reply to comment #28)
> Created an attachment (id=213981) [details]
> Patch
> 
> blind patch for landing, will be checked locally before landing

I tested, it builds locally with enabled network process.

[Csaba Osztrogonác]

Comment on attachment 213981 [details]
Patch

Clearing flags on attachment: 213981

Committed r157406: <http://trac.webkit.org/changeset/157406>