Bug 108832 – [Meta][WK2][Soup] Set up NetworkProcess

• Status: ASSIGNED
• Component: Platform

[Balazs Kelemen]

Set up NetworkProcess for EFL, including the soup backend, general unix, and cmake related stuff.

[Balazs Kelemen]

Created attachment 186403 [details]
Patch

[Build Bot]

Comment on attachment 186403 [details]
Patch

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

[Build Bot]

Comment on attachment 186403 [details]
Patch

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

[EFL EWS Bot]

Comment on attachment 186403 [details]
Patch

Attachment 186403 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16369308

[Gyuyoung Kim]

Comment on attachment 186403 [details]
Patch

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

Personally, it looks this patch is a little huge to review.

> Source/WebKit2/ChangeLog:10
> +        basically. Advenced features like private browsing or credential storage

Typo: s/Advenced/Advanced/g ?

> Source/WebCore/platform/network/NetworkStorageSession.h:76
> +#else    

Nit: Unneeded spaces.

> Source/WebKit2/NetworkProcess/efl/NetworkProcessEfl.cpp:1
> +#include "config.h"

Missing license.

> Source/WebKit2/NetworkProcess/efl/NetworkProcessEfl.cpp:21
> +void NetworkProcess::allowSpecificHTTPSCertificateForHost(const PlatformCertificateInfo&, const String& host)

Remove *host*

> Source/WebKit2/NetworkProcess/soup/NetworkProcessProxySoup.cpp:1
> +#include "config.h"

ditto.

> Source/WebKit2/NetworkProcess/soup/NetworkResourceLoadSchedulerSoup.cpp:1
> +#include "config.h"

ditto.

> Source/WebKit2/NetworkProcess/soup/RemoteNetworkingContextSoup.cpp:1
> +#include "config.h"

Missing license.

> Source/WebKit2/PlatformEfl.cmake:3
> +    NetworkProcess/unix/NetworkProcessMainUnix.cpp

Wrong alphabet order.

> Source/WebKit2/UIProcess/Network/soup/NetworkProcessProxySoup.cpp:26
> +#include "config.h"

Add a new line.

> Source/WebKit2/UIProcess/soup/WebContextSoup.cpp:1
> +#include "config.h"

Missing license.

> Source/WebKit2/unix/NetworkMainUnix.cpp:1
> +#include "config.h"

Missing license.

[Balazs Kelemen]

(In reply to comment #5)
> (From update of attachment 186403 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186403&action=review
> 
> Personally, it looks this patch is a little huge to review.

Yes, it is, but I'm not sure it's better to split up into parts that cannot be tested on their own. Furthermore there is very little or no logic in it, mostly build stuff and a bit of refactoring. However I can split it if you think it is necessary.

Going to fix the other issues.

[Kenneth Rohde Christiansen]

Comment on attachment 186403 [details]
Patch

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

> Source/WebKit2/NetworkProcess/NetworkProcess.cpp:64
> +#if PLATFORM(GTK)
> +#define SOCKET_TYPE SOCK_STREAM
> +#else
> +#define SOCKET_TYPE SOCK_DGRAM
> +#endif

Guess a GTK guy should review this. But why the difference? comment?

> Source/WebKit2/NetworkProcess/RemoteNetworkingContext.h:27
> +
> +#ifndef RemoteNetworkingContext_h
> +#define RemoteNetworkingContext_h

if you move things around, maybe that should be a separate patch with good reasoning for doing so

> Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp:65
> +#if PLATFORM(EFL)
> +    if (!eina_init())
> +        return 1;
> +    if (!ecore_init()) {
> +        // Could not init ecore.
> +        eina_shutdown();
> +        return 1;
> +    }
> +#endif

So why does the networking process need EFL stuff, when we basically just use libsoup for networking? Why not just use glib+libsoup and share this with GTK?

[Balazs Kelemen]

(In reply to comment #7)
> (From update of attachment 186403 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186403&action=review
> 
> > Source/WebKit2/NetworkProcess/NetworkProcess.cpp:64
> > +#if PLATFORM(GTK)
> > +#define SOCKET_TYPE SOCK_STREAM
> > +#else
> > +#define SOCKET_TYPE SOCK_DGRAM
> > +#endif
> 
> Guess a GTK guy should review this. But why the difference? comment?

Old story, and I don't remember exactly.  This is copied in a lot of places, so are the fnctl calls. Maybe it's better to move it into a helper anyway.

> 
> > Source/WebKit2/NetworkProcess/RemoteNetworkingContext.h:27
> > +
> > +#ifndef RemoteNetworkingContext_h
> > +#define RemoteNetworkingContext_h
> 
> if you move things around, maybe that should be a separate patch with good reasoning for doing so

Hm, it is somewhat trivial that this file should be there for every port.

> 
> > Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp:65
> > +#if PLATFORM(EFL)
> > +    if (!eina_init())
> > +        return 1;
> > +    if (!ecore_init()) {
> > +        // Could not init ecore.
> > +        eina_shutdown();
> > +        return 1;
> > +    }
> > +#endif
> 
> So why does the networking process need EFL stuff, when we basically just use libsoup for networking? Why not just use glib+libsoup and share this with GTK?

RunLoop is different on EFL, it uses ecore, I don't think we can get rid of that.

[Kenneth Rohde Christiansen]

> > So why does the networking process need EFL stuff, when we basically just use libsoup for networking? Why not just use glib+libsoup and share this with GTK?
> 
> RunLoop is different on EFL, it uses ecore, I don't think we can get rid of that.

Not even if it is a separate process/build project... can't you not just use the glib based one from gtk? It will probably perform better than the currently integration between ecore and glib used by default.

[Dan Winship]

yeah, what he said. If you use RunLoopGtk rather than RunLoopEfl, you shouldn't need to initialize any efl stuff. (And RunLoopGtk, despite the name, does not actually use gtk, just glib. Though maybe you'll need to copy it to a new file to avoid pulling in other webkitgtk stuff...)

[Balazs Kelemen]

(In reply to comment #10)
> yeah, what he said. If you use RunLoopGtk rather than RunLoopEfl, you shouldn't need to initialize any efl stuff. (And RunLoopGtk, despite the name, does not actually use gtk, just glib. Though maybe you'll need to copy it to a new file to avoid pulling in other webkitgtk stuff...)

Then why the hell we have RunLoopEFL? And how should I manage to get a copy of the lib that contains the gtk implementation, while the one that the ui and the web processes links to contains the efl one? Note that every of our processes (ui, web, plugin, network) are just contains a simple main function that calls in to the same library.

I'm not saying that it's not possible or not a good idea but it's out of my scope.

[Thiago Marcos P. Santos]

Comment on attachment 186403 [details]
Patch

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

>>> Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp:65
>>> +#endif
>> 
>> So why does the networking process need EFL stuff, when we basically just use libsoup for networking? Why not just use glib+libsoup and share this with GTK?
> 
> RunLoop is different on EFL, it uses ecore, I don't think we can get rid of that.

I don't see the need ecore stuff here. In theory, our NetworkProcess should be identical to the GTK one. IMO we should call this one NetworkProcessMainSoup.cpp and use glib directly instead the WebCore mainloop abstraction.

> Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.h:2
> +#ifndef NetworkProcessMainUnix_h
> +#define NetworkProcessMainUnix_h

Again, I think this is not Unix, but Soup.

[Alexey Proskuryakov]

Comment on attachment 186403 [details]
Patch

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

> Source/WebCore/platform/network/NetworkStorageSession.h:80
> +    bool m_isPrivate;

I'm concerned that this change will break the build for other ports when using newer clang (which can detach unused member variables).

[Laszlo Gombos]

(In reply to comment #9)
> > > So why does the networking process need EFL stuff, when we basically just use libsoup for networking? Why not just use glib+libsoup and share this with GTK?
> > 
> > RunLoop is different on EFL, it uses ecore, I don't think we can get rid of that.
> 
> Not even if it is a separate process/build project... can't you not just use the glib based one from gtk? It will probably perform better than the currently integration between ecore and glib used by default.

I agree. At some point I looked into doing this but never get around implementing it.

[Gyuyoung Kim]

Comment on attachment 186403 [details]
Patch

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

> Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp:71
> +    if (!ecore_main_loop_glib_integrate())

I also agree with Kenneth and Thiago. EFL port has used ecore_main_loop_glib_integrate() to support glib based library as libsoup. But, in this case, we can share this with GTK port. I don't use GMainLoop for EFL port yet directly, it looks there is no critical problem.

[Balazs Kelemen]

(In reply to comment #15)
> (From update of attachment 186403 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186403&action=review
> 
> > Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp:71
> > +    if (!ecore_main_loop_glib_integrate())
> 
> I also agree with Kenneth and Thiago. EFL port has used ecore_main_loop_glib_integrate() to support glib based library as libsoup. But, in this case, we can share this with GTK port. I don't use GMainLoop for EFL port yet directly, it looks there is no critical problem.

We can either share the generic RunLoop implementation everywhere, or not share at all. It is not practical to set up a special duplicate of the webkit2 dynamic library just to avoid the efl dependency here.

[Thiago Marcos P. Santos]

Comment on attachment 186403 [details]
Patch

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

>>> Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp:71
>>> +    if (!ecore_main_loop_glib_integrate())
>> 
>> I also agree with Kenneth and Thiago. EFL port has used ecore_main_loop_glib_integrate() to support glib based library as libsoup. But, in this case, we can share this with GTK port. I don't use GMainLoop for EFL port yet directly, it looks there is no critical problem.
> 
> We can either share the generic RunLoop implementation everywhere, or not share at all. It is not practical to set up a special duplicate of the webkit2 dynamic library just to avoid the efl dependency here.

We could try a CMake trickery and build the RunLoop twice (the GTK and the EFL version) as a static library. The GTK version will only to be used here.

[Balazs Kelemen]

(In reply to comment #12)
> (From update of attachment 186403 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186403&action=review
> 
> >>> Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp:65
> >>> +#endif
> >> 
> >> So why does the networking process need EFL stuff, when we basically just use libsoup for networking? Why not just use glib+libsoup and share this with GTK?
> > 
> > RunLoop is different on EFL, it uses ecore, I don't think we can get rid of that.
> 
> I don't see the need ecore stuff here. In theory, our NetworkProcess should be identical to the GTK one. IMO we should call this one NetworkProcessMainSoup.cpp and use glib directly instead the WebCore mainloop abstraction.

The core does use the RunLoop abstraction so you will not get more from the network process than a beautiful crash if you don't set it up properly.

Ok, I accept that you want to get rid of the glib-efl integration, but does not feel like it should be my task. I am supposing to remove the efl parts from here. This will in fact leave you with a broken network process, but since it is not used by default, it won't hurt anybody now. From this point you can chose how to solve the runloop issue, for example by making the gtk runloop built into the network process with some build system tricks.

Also I don't think we need NetworkProcessMainSoup.cpp, I think Qt can also use this file with a bit of ifdefs which is IMHO better than duplicating stuff.

[Balazs Kelemen]

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 186403 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=186403&action=review
> > 
> > > Source/WebKit2/NetworkProcess/NetworkProcess.cpp:64
> > > +#if PLATFORM(GTK)
> > > +#define SOCKET_TYPE SOCK_STREAM
> > > +#else
> > > +#define SOCKET_TYPE SOCK_DGRAM
> > > +#endif
> > 
> > Guess a GTK guy should review this. But why the difference? comment?
> 
> Old story, and I don't remember exactly.  This is copied in a lot of places, so are the fnctl calls. Maybe it's better to move it into a helper anyway.

This is handled in bug 109032.

[Balazs Kelemen]

Created attachment 187112 [details]
Patch

[Build Bot]

Comment on attachment 187112 [details]
Patch

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

[Build Bot]

Comment on attachment 187112 [details]
Patch

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

[Balazs Kelemen]

Created attachment 187327 [details]
Patch

Trying to fix the mac build.

[Build Bot]

Comment on attachment 187327 [details]
Patch

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

[Build Bot]

Comment on attachment 187327 [details]
Patch

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

[Balazs Kelemen]

Created attachment 187439 [details]
Patch

[Build Bot]

Comment on attachment 187439 [details]
Patch

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

[Balazs Kelemen]

(In reply to comment #27)
> (From update of attachment 187439 [details])
> Attachment 187439 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: http://queues.webkit.org/results/16466558

D'oh! This time I was doing it in Xcode. Do you know how should I do it in a way Xcode accepts? Now I was 'duplicated' the file in Xcode, then removed the original, but it did no actually created the file at the new place on the file system just hacked the projcet file, so I actually made the move with git mv, than patched the file (at the new place). I did not find any option to actually move a file in Xcode. Or is a clean build necessary for Mac in this case?

[Build Bot]

Comment on attachment 187439 [details]
Patch

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

[Balazs Kelemen]

Clean build on Mac is successful to me with the patch.

[Alexey Proskuryakov]

Comment on attachment 187439 [details]
Patch

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

> Source/WebKit2/WebKit2.xcodeproj/project.pbxproj:574
> +		98BA122516C57C2100888895 /* RemoteNetworkingContext.h in CopyFiles */ = {isa = PBXBuildFile; fileRef = 98BA122416C57C2100888895 /* RemoteNetworkingContext.h */; };

This doesn't look right. Where is it copying the file to, and why?

> Source/WebKit2/WebKit2.xcodeproj/project.pbxproj:1262
> +				98BA122516C57C2100888895 /* RemoteNetworkingContext.h in CopyFiles */,

Ditto.

[Balazs Kelemen]

Created attachment 187548 [details]
Patch

[EFL EWS Bot]

Comment on attachment 187548 [details]
Patch

Attachment 187548 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16496176

[Build Bot]

Comment on attachment 187548 [details]
Patch

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

[Balazs Kelemen]

Comment on attachment 187548 [details]
Patch

Forgot to patch the header (after xcode hacking, git mv, xcode hacking)

[Balazs Kelemen]

Created attachment 187565 [details]
Patch

[Balazs Kelemen]

Seems like it is ready now (builds on mac) as well as bug 108613. Can I get a review? :)

[Kenneth Rohde Christiansen]

Comment on attachment 187565 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Define NetworkStorageSession::isPrivateBrowsingSession for soup as well.
> +

This patch is big, could this be a separate patch?

> Source/WebKit2/ChangeLog:170
> +        (WebKit::WebProcess::ensureNetworkProcessConnection):
> +        * unix/NetworkMainUnix.cpp: Copied from Source/WebKit2/Shared/WebResourceBuffer.h.
> +        (main):

really?

> Source/WebCore/platform/network/NetworkStorageSession.h:64
>      SoupSession* soupSession() const { return m_session; }
> +    bool isPrivateBrowsingSession() const { return m_isPrivate; }
>  #else
>      NetworkStorageSession(NetworkingContext*);
>      ~NetworkStorageSession();

Why is there no setter? if it won't ever be set to true yet, why not just return false; for now?

> Source/WebKit2/NetworkProcess/NetworkProcess.cpp:189
> +    CoreIPC::Connection::Identifier serverIdentifier, clientIdentifier;
> +    bool success = CoreIPC::Connection::createServerAndClientIdentifiers(serverIdentifier, clientIdentifier);
> +    ASSERT_UNUSED(success, success);

Why store the return value if not used?

[Balazs Kelemen]

(In reply to comment #38)
> (From update of attachment 187565 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=187565&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        Define NetworkStorageSession::isPrivateBrowsingSession for soup as well.
> > +
> 
> This patch is big, could this be a separate patch?

Ok, I will think how to slice it up further.

> 
> > Source/WebKit2/ChangeLog:170
> > +        (WebKit::WebProcess::ensureNetworkProcessConnection):
> > +        * unix/NetworkMainUnix.cpp: Copied from Source/WebKit2/Shared/WebResourceBuffer.h.
> > +        (main):
> 
> really?

Not really, those silly messages are added by the commit-log-editor, but I can remove them if it makes reviewing easier.

> 
> > Source/WebCore/platform/network/NetworkStorageSession.h:64
> >      SoupSession* soupSession() const { return m_session; }
> > +    bool isPrivateBrowsingSession() const { return m_isPrivate; }
> >  #else
> >      NetworkStorageSession(NetworkingContext*);
> >      ~NetworkStorageSession();
> 
> Why is there no setter? if it won't ever be set to true yet, why not just return false; for now?

Should I really leave there something that should be changed sooner or later anyway? Returning with the member here is harmless. I would rather keep it.

> 
> > Source/WebKit2/NetworkProcess/NetworkProcess.cpp:189
> > +    CoreIPC::Connection::Identifier serverIdentifier, clientIdentifier;
> > +    bool success = CoreIPC::Connection::createServerAndClientIdentifiers(serverIdentifier, clientIdentifier);
> > +    ASSERT_UNUSED(success, success);
> 
> Why store the return value if not used?

How would you do a debug-only check otherwise? That is what ASSERT_UNUSED has been introduced for.

[Balazs Kelemen]

Let's do this in smaller steps.

[Kwang Yul Seo]

I took over this work from Balazs because he is busy doing other works.

I've just managed to load a web page with NetworkProcess enabled in GTK port. I will upload patches soon after cleaning up the code.

I will work on Gtk port first and then move on to Efl. It would be trivial to support Efl because both ports share the same soup backend.

[Kwang Yul Seo]

Remvoed [EFL] from the subject because I am working on both Gtk and Efl ports.