Bug 110136 - [WK2][Soup] Add entry point for network process
Summary: [WK2][Soup] Add entry point for network process
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Balazs Kelemen
URL:
Keywords:
Depends on:
Blocks: 108832 118231 118388
  Show dependency treegraph
 
Reported: 2013-02-18 09:44 PST by Balazs Kelemen
Modified: 2013-07-24 15:45 PDT (History)
10 users (show)

See Also:


Attachments
Patch (6.01 KB, patch)
2013-02-18 09:54 PST, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (7.97 KB, patch)
2013-06-30 18:20 PDT, Kwang Yul Seo
no flags Details | Formatted Diff | Diff
Patch (7.92 KB, patch)
2013-07-03 02:55 PDT, Kwang Yul Seo
no flags Details | Formatted Diff | Diff
Patch (7.65 KB, patch)
2013-07-16 15:55 PDT, Kwang Yul Seo
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-18 09:44:53 PST
Add the entry point what will be used by the executable.
Comment 1 Balazs Kelemen 2013-02-18 09:54:34 PST
Created attachment 188912 [details]
Patch
Comment 2 Build Bot 2013-02-18 13:27:31 PST
Comment on attachment 188912 [details]
Patch

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

New failing tests:
media/video-controls-captions-trackmenu.html
Comment 3 Kwang Yul Seo 2013-06-30 18:20:43 PDT
Created attachment 205779 [details]
Patch
Comment 4 Kwang Yul Seo 2013-06-30 18:23:17 PDT
(In reply to comment #3)
I made two changes to the original patch:
1. Include unix/NetworkMainUnix.cpp here.
2. Wrap NetworkProcessMain declaration with extern "C" to prevent name mangling in C++.
Comment 5 Carlos Garcia Campos 2013-07-03 00:24:36 PDT
Comment on attachment 205779 [details]
Patch

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

> Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp:56
> +#if PLATFORM(GTK)
> +    gtk_init(&argc, &argv);
> +#endif

Why do we need GTK+ in the network process?
Comment 6 Kwang Yul Seo 2013-07-03 00:59:48 PDT
(In reply to comment #5)
> Why do we need GTK+ in the network process?
Not needed. I will change it to use Glib event loop.
Comment 7 Kwang Yul Seo 2013-07-03 02:39:09 PDT
(In reply to comment #6)
Because Gtk is already using glib-only run loop implementation, it seems we can simply drop gtk_init() here.
Comment 8 Kwang Yul Seo 2013-07-03 02:55:37 PDT
Created attachment 205987 [details]
Patch
Comment 9 Brady Eidson 2013-07-04 17:53:03 PDT
Comment on attachment 205987 [details]
Patch

Seems fine.  Once a platform knowledgeable reviewer signs off, so will I.
Comment 10 Kwang Yul Seo 2013-07-08 18:26:31 PDT
CC'ing knowledgeable reviewer Gustavo Gustavo Noronha Silva.
Comment 11 Gustavo Noronha (kov) 2013-07-16 05:51:32 PDT
Comment on attachment 205987 [details]
Patch

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

> Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp:51
> +    // FIXME: Event loop is not initialized correctly for EFL. For EFL we should should either
> +    // initialize ecore or use the glib-only RunLoop implementation in the network process.

Why is it not initialized correctly? I guess you need to call ecore_main_loop_glib_integrate() before calling InitializeWebKit2?

> Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp:63
> +    errno = 0;
> +    long int socket = strtol(argv[1], 0, 10);
> +    if (errno)
> +        return 1;
> +
> +    WebKit::ChildProcessInitializationParameters parameters;
> +    parameters.connectionIdentifier = int(socket);

On other entry points we use atoi() to go straight to int and avoid this error checking and conversion dance. I would prefer if we kept this consistent.

> Source/WebKit2/unix/NetworkMainUnix.cpp:2
> + * Copyright (C) 2013 University of Szeged. All rights reserved.

This is trivial enough that I don't think you need to import the copyright.
Comment 12 Kwang Yul Seo 2013-07-16 15:55:50 PDT
Created attachment 206822 [details]
Patch
Comment 13 Kwang Yul Seo 2013-07-16 16:00:34 PDT
(In reply to comment #11)
> (From update of attachment 205987 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=205987&action=review
> 
> Why is it not initialized correctly? I guess you need to call ecore_main_loop_glib_integrate() before calling InitializeWebKit2?

Done. I will check if this is correct with one of EFL maintainers.

> On other entry points we use atoi() to go straight to int and avoid this error checking and conversion dance. I would prefer if we kept this consistent.

Done.
 
> > Source/WebKit2/unix/NetworkMainUnix.cpp:2
> > + * Copyright (C) 2013 University of Szeged. All rights reserved.
> 
> This is trivial enough that I don't think you need to import the copyright.

This copyright line is from Balazs Kelemen's original patch. I kept the license intact as he is the original author.
Comment 14 WebKit Commit Bot 2013-07-16 16:01:56 PDT
Attachment 206822 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp', u'Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.h', u'Source/WebKit2/unix/NetworkMainUnix.cpp']" exit_code: 1
Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp:45:  Do not use 'using namespace WebCore;'.  [build/using_namespace] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 Kwang Yul Seo 2013-07-16 17:03:10 PDT
(In reply to comment #14)
> If any of these errors are false positives, please file a bug against check-webkit-style.

I filed Bug 118755 to fix this.
Comment 16 Balazs Kelemen 2013-07-17 01:13:30 PDT
(In reply to comment #11)
> (From update of attachment 205987 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=205987&action=review
> 
> > Source/WebKit2/NetworkProcess/unix/NetworkProcessMainUnix.cpp:51
> > +    // FIXME: Event loop is not initialized correctly for EFL. For EFL we should should either
> > +    // initialize ecore or use the glib-only RunLoop implementation in the network process.
> 
> Why is it not initialized correctly? I guess you need to call ecore_main_loop_glib_integrate() before calling InitializeWebKit2?
 
When I was working on this I was told that it is unfortunate that efl is using it's own event loop with glib integration instead of just using glib. For the latter to work one needs to add a build target (a static lib) that includes the glib implementation of RunLoop (RunLoopGtk) and it's dependencies and use it for the network process. Similarly it could be used for the plugin process.

I think it does not belong to this work so it is fine to land this now and postpone the problem.
Comment 17 Kwang Yul Seo 2013-07-24 15:45:31 PDT
Comment on attachment 206822 [details]
Patch

Clearing flags on attachment: 206822

Committed r153106: <http://trac.webkit.org/changeset/153106>
Comment 18 Kwang Yul Seo 2013-07-24 15:45:38 PDT
All reviewed patches have been landed.  Closing bug.