Bug 118388

Created attachment 206075 [details] Patch

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

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

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

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?

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.

(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); }

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.

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

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

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

see also bug 114683; ProxyResolverSoup can be removed

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.

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

Created attachment 206287 [details] Patch

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.

Created attachment 207431 [details] Patch

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

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

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.

Created attachment 212714 [details] Patch updated: one more include added, avoid the conflict with bug118343

ping?

Comment on attachment 212714 [details] Patch I'm fine with this if EFL/Soup reviewers are.

cc Carlos as SOUP reviewer

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

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.

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.

Created attachment 213981 [details] Patch blind patch for landing, will be checked locally before landing

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

Comment on attachment 213981 [details] Patch Clearing flags on attachment: 213981 Committed r157406: <http://trac.webkit.org/changeset/157406>