Bug 110093 – [WK2] Add UNIX_DOMAIN_SOCKETS specific bits for supporting NetworkProcess

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Balazs Kelemen]

Add UNIX_DOMAIN_SOCKETS specific bits for supporting NetworkProcess.

[Balazs Kelemen]

Created attachment 188823 [details]
Patch

[Build Bot]

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

[Jesus Sanchez-Palencia]

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.

[Balazs Kelemen]

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

[Balazs Kelemen]

Created attachment 189070 [details]
Patch

[Brady Eidson]

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!

[Balazs Kelemen]

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

[Balazs Kelemen]

(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

[Kwang Yul Seo]

Created attachment 205781 [details]
Patch

[Kwang Yul Seo]

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

[Brady Eidson]

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?

[Brady Eidson]

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.

[Brady Eidson]

Adding blocking relationship on 110978

[Kwang Yul Seo]

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

[Csaba Osztrogonác]

I think we can close it, because this change would be 
unnecessary after bug110978.

[Csaba Osztrogonác]

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.

[Sergio Villar Senin]

Created attachment 218695 [details]
Patch

[Martin Robinson]

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.

[Sergio Villar Senin]

Created attachment 218740 [details]
Patch

[Sergio Villar Senin]

Committed r160308: <http://trac.webkit.org/changeset/160308>