Bug 110093 - [WK2] Add UNIX_DOMAIN_SOCKETS specific bits for supporting NetworkProcess
: [WK2] Add UNIX_DOMAIN_SOCKETS specific bits for supporting NetworkProcess
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebKit2
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Balazs Kelemen
:
Depends on: 110978
Blocks: 108832
  Show dependency treegraph
 
Reported: 2013-02-18 01:43 PST by Balazs Kelemen
Modified: 2013-12-09 04:25 PST (History)
18 users (show)

See Also:


Attachments
Patch (4.10 KB, patch)
2013-02-18 01:48 PST, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (4.16 KB, patch)
2013-02-19 06:08 PST, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (5.23 KB, patch)
2013-06-30 19:27 PDT, Kwang Yul Seo
no flags Details | Formatted Diff | Diff
Patch (10.17 KB, patch)
2013-12-08 09:42 PST, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (9.34 KB, patch)
2013-12-09 01:57 PST, Sergio Villar Senin
mrobinson: review+
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 01:43:49 PST
Add UNIX_DOMAIN_SOCKETS specific bits for supporting NetworkProcess.
Comment 1 Balazs Kelemen 2013-02-18 01:48:54 PST
Created attachment 188823 [details]
Patch
Comment 2 Build Bot 2013-02-18 04:24:34 PST
Comment on attachment 188823 [details]
Patch

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

New failing tests:
media/video-controls-captions-trackmenu.html
Comment 3 Jesus Sanchez-Palencia 2013-02-18 14:05:54 PST
Comment on attachment 188823 [details]
Patch

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

> Source/WebKit2/NetworkProcess/NetworkProcess.cpp:-185
> -#else

Don't you need to keep this #else notImplemented(); here? Or are the only possible implementations protected by PLATFORM(MAC) and USE(UNIX_DOMAIN_SOCKETS) already?

> Source/WebKit2/UIProcess/Network/NetworkProcessProxy.cpp:-114
> -#else

Ditto.

> Source/WebKit2/UIProcess/Network/NetworkProcessProxy.cpp:-164
> -#else

Ditto.

> Source/WebKit2/WebProcess/WebProcess.cpp:-354
> -#else

Ditto.
Comment 4 Balazs Kelemen 2013-02-19 04:41:16 PST
(In reply to comment #3)
> (From update of attachment 188823 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188823&action=review
> 
> > Source/WebKit2/NetworkProcess/NetworkProcess.cpp:-185
> > -#else
> 
> Don't you need to keep this #else notImplemented(); here? Or are the only possible implementations protected by PLATFORM(MAC) and USE(UNIX_DOMAIN_SOCKETS) already?

I was thinking that but now that I think it over it is not true. There are PLATFORM(WINDOWS) and other combinations like (PLATFORM(QT) && OS(WIN)). I am going to keep the #else notImplemented() case.
Comment 5 Balazs Kelemen 2013-02-19 06:08:22 PST
Created attachment 189070 [details]
Patch
Comment 6 Brady Eidson 2013-02-21 14:06:46 PST
Comment on attachment 189070 [details]
Patch

In CoreIPC do we have a goal of making connectioney stuff work without all of these #if blocks?

Because they suck!
Comment 7 Balazs Kelemen 2013-02-21 15:10:47 PST
(In reply to comment #6)
> (From update of attachment 189070 [details])
> In CoreIPC do we have a goal of making connectioney stuff work without all of these #if blocks?
> 
> Because they suck!

Probably it would be worthwhile, but I think it should be a separate refactoring step. It's ok to me if you say it should be handled first.
Comment 8 Balazs Kelemen 2013-02-27 08:59:45 PST
(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 189070 [details] [details])
> > In CoreIPC do we have a goal of making connectioney stuff work without all of these #if blocks?
> > 
> > Because they suck!
> 
> Probably it would be worthwhile, but I think it should be a separate refactoring step. It's ok to me if you say it should be handled first.

Seee: bug 110978
Comment 9 Kwang Yul Seo 2013-06-30 19:27:51 PDT
Created attachment 205781 [details]
Patch
Comment 10 Kwang Yul Seo 2013-06-30 19:32:49 PDT
(In reply to comment #9)
I changed the patch not to depend on Bug 110978.  I will refactor IPC connection creation code all together once Anders Carlsson reviews Bug 110978.
Comment 11 Brady Eidson 2013-07-04 17:02:11 PDT
Comment on attachment 205781 [details]
Patch

Is the patch in https://bugs.webkit.org/show_bug.cgi?id=110978 the way forward to remove this CoreIPC nonsense so we don't have to add this #ifdef hell to cross platform files?
Comment 12 Brady Eidson 2013-07-04 17:02:58 PDT
Because if it is, then we need to renew focus in getting Anders to review/guide on 110978, because it is not okay to be doing this to cross-platform code.
Comment 13 Brady Eidson 2013-07-04 17:03:17 PDT
Adding blocking relationship on 110978
Comment 14 Kwang Yul Seo 2013-07-04 17:12:51 PDT
(In reply to comment #12)
> Because if it is, then we need to renew focus in getting Anders to review/guide on 110978, because it is not okay to be doing this to cross-platform code.

Okay. I will rebase the patch in Bug 110978 on top of the HEAD and ask Anders to review it.
Comment 15 Csaba Osztrogonác 2013-09-17 08:31:26 PDT
I think we can close it, because this change would be 
unnecessary after bug110978.
Comment 16 Csaba Osztrogonác 2013-10-24 06:39:21 PDT
Reopen, because WK2 owners don't have time for reviewing the much more bigger
refactoring patch in https://bugs.webkit.org/show_bug.cgi?id=110978

Please revise your previous opinion and let this patch land into trunk.
Comment 17 Sergio Villar Senin 2013-12-08 09:42:37 PST
Created attachment 218695 [details]
Patch
Comment 18 Martin Robinson 2013-12-08 09:55:24 PST
Comment on attachment 218695 [details]
Patch

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

> Source/WebKit2/NetworkProcess/NetworkProcess.cpp:200
> +    CoreIPC::Connection::ConnectionDescriptor connectionDescriptor(CoreIPC::Connection::createPlatformConnection());

You should probably do:

CoreIPC::Connection::SocketPair socketPair = CoreIPC::Connection::createPlatformConnection();

(see below)

> Source/WebKit2/Platform/CoreIPC/Connection.h:132
> +    class ConnectionDescriptor {
> +    public:
> +        Identifier clientIdentifier() const { return m_clientIdentifier; }
> +        Identifier serverIdentifier() const { return m_serverIdentifier; }
> +    private:
> +        friend class Connection;
> +        ConnectionDescriptor(Identifier clientIdentifier, Identifier serverIdentifier)
> +            : m_clientIdentifier(clientIdentifier)
> +            , m_serverIdentifier(serverIdentifier)
> +        { }
> +        Identifier m_clientIdentifier;
> +        Identifier m_serverIdentifier;
> +    };

I think I'd prefer something like:

struct SocketPair {
    int client;
    int server;
}

This seems a little heavy-weight.
Comment 19 Sergio Villar Senin 2013-12-09 01:57:36 PST
Created attachment 218740 [details]
Patch
Comment 20 Sergio Villar Senin 2013-12-09 04:25:52 PST
Committed r160308: <http://trac.webkit.org/changeset/160308>