Bug 108832 - [Meta][WK2][Soup] Set up NetworkProcess
Summary: [Meta][WK2][Soup] Set up NetworkProcess
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Balazs Kelemen
URL:
Keywords:
Depends on: 118520 126127 108615 109032 110093 110097 110115 110136 110139 110141 110978 118228 118231 118343 118388 118448 118598 120294 120351 120353 120532 120533 125422 125490 125494 125505 125507 125515 125557 125564 125576 125582 125583 125586 126006 126129 126130 126131 127651
Blocks: 125463
  Show dependency treegraph
 
Reported: 2013-02-04 08:53 PST by Balazs Kelemen
Modified: 2017-03-11 10:59 PST (History)
23 users (show)

See Also:


Attachments
Patch (53.17 KB, patch)
2013-02-04 09:52 PST, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (64.38 KB, patch)
2013-02-07 08:08 PST, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (66.05 KB, patch)
2013-02-08 08:50 PST, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (70.18 KB, patch)
2013-02-09 12:20 PST, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (60.28 KB, patch)
2013-02-11 05:01 PST, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (71.37 KB, patch)
2013-02-11 07:12 PST, Balazs Kelemen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Balazs Kelemen 2013-02-04 08:53:01 PST
Set up NetworkProcess for EFL, including the soup backend, general unix, and cmake related stuff.
Comment 1 Balazs Kelemen 2013-02-04 09:52:49 PST
Created attachment 186403 [details]
Patch
Comment 2 Build Bot 2013-02-04 10:02:11 PST
Comment on attachment 186403 [details]
Patch

Attachment 186403 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16374252
Comment 3 Build Bot 2013-02-04 10:12:43 PST
Comment on attachment 186403 [details]
Patch

Attachment 186403 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16369303
Comment 4 EFL EWS Bot 2013-02-04 10:27:24 PST
Comment on attachment 186403 [details]
Patch

Attachment 186403 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16369308
Comment 5 Gyuyoung Kim 2013-02-04 15:44:59 PST
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.
Comment 6 Balazs Kelemen 2013-02-05 06:20:43 PST
(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.
Comment 7 Kenneth Rohde Christiansen 2013-02-05 06:26:19 PST
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?
Comment 8 Balazs Kelemen 2013-02-05 08:31:44 PST
(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.
Comment 9 Kenneth Rohde Christiansen 2013-02-05 08:34:17 PST
> > 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.
Comment 10 Dan Winship 2013-02-05 08:59:35 PST
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...)
Comment 11 Balazs Kelemen 2013-02-05 09:07:47 PST
(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.
Comment 12 Thiago Marcos P. Santos 2013-02-05 10:17:39 PST
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.
Comment 13 Alexey Proskuryakov 2013-02-05 10:22:55 PST
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).
Comment 14 Laszlo Gombos 2013-02-05 11:51:38 PST
(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.
Comment 15 Gyuyoung Kim 2013-02-06 00:37:01 PST
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.
Comment 16 Balazs Kelemen 2013-02-06 01:36:16 PST
(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.
Comment 17 Thiago Marcos P. Santos 2013-02-06 02:38:35 PST
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.
Comment 18 Balazs Kelemen 2013-02-06 06:11:26 PST
(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.
Comment 19 Balazs Kelemen 2013-02-06 11:49:10 PST
(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.
Comment 20 Balazs Kelemen 2013-02-07 08:08:52 PST
Created attachment 187112 [details]
Patch
Comment 21 Build Bot 2013-02-07 10:14:35 PST
Comment on attachment 187112 [details]
Patch

Attachment 187112 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16430248
Comment 22 Build Bot 2013-02-07 10:19:31 PST
Comment on attachment 187112 [details]
Patch

Attachment 187112 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16445273
Comment 23 Balazs Kelemen 2013-02-08 08:50:44 PST
Created attachment 187327 [details]
Patch

Trying to fix the mac build.
Comment 24 Build Bot 2013-02-08 09:30:58 PST
Comment on attachment 187327 [details]
Patch

Attachment 187327 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16432726
Comment 25 Build Bot 2013-02-08 09:46:54 PST
Comment on attachment 187327 [details]
Patch

Attachment 187327 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16408818
Comment 26 Balazs Kelemen 2013-02-09 12:20:09 PST
Created attachment 187439 [details]
Patch
Comment 27 Build Bot 2013-02-09 12:45:55 PST
Comment on attachment 187439 [details]
Patch

Attachment 187439 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16466558
Comment 28 Balazs Kelemen 2013-02-09 13:04:10 PST
(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?
Comment 29 Build Bot 2013-02-09 13:13:56 PST
Comment on attachment 187439 [details]
Patch

Attachment 187439 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16479441
Comment 30 Balazs Kelemen 2013-02-09 13:42:47 PST
Clean build on Mac is successful to me with the patch.
Comment 31 Alexey Proskuryakov 2013-02-09 14:31:30 PST
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.
Comment 32 Balazs Kelemen 2013-02-11 05:01:17 PST
Created attachment 187548 [details]
Patch
Comment 33 EFL EWS Bot 2013-02-11 05:21:48 PST
Comment on attachment 187548 [details]
Patch

Attachment 187548 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16496176
Comment 34 Build Bot 2013-02-11 06:03:51 PST
Comment on attachment 187548 [details]
Patch

Attachment 187548 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16485366
Comment 35 Balazs Kelemen 2013-02-11 06:30:43 PST
Comment on attachment 187548 [details]
Patch

Forgot to patch the header (after xcode hacking, git mv, xcode hacking)
Comment 36 Balazs Kelemen 2013-02-11 07:12:08 PST
Created attachment 187565 [details]
Patch
Comment 37 Balazs Kelemen 2013-02-12 10:39:54 PST
Seems like it is ready now (builds on mac) as well as bug 108613. Can I get a review? :)
Comment 38 Kenneth Rohde Christiansen 2013-02-15 08:09:51 PST
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?
Comment 39 Balazs Kelemen 2013-02-15 08:20:18 PST
(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.
Comment 40 Balazs Kelemen 2013-02-18 06:55:44 PST
Let's do this in smaller steps.
Comment 41 Kwang Yul Seo 2013-06-29 00:37:03 PDT
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.
Comment 42 Kwang Yul Seo 2013-06-30 19:36:56 PDT
Remvoed [EFL] from the subject because I am working on both Gtk and Efl ports.