Bug 110978 – [WK2] Refactoring: unify IPC connection creation - get rid of ifdef hell

• Component: WebKit2
• Status: RESOLVED WONTFIX

[Balazs Kelemen]

This has been raised in https://bugs.webkit.org/show_bug.cgi?id=110093#c6

[Balazs Kelemen]

*** Bug 109032 has been marked as a duplicate of this bug. ***

[Balazs Kelemen]

Created attachment 190537 [details]
Patch

[EFL EWS Bot]

Comment on attachment 190537 [details]
Patch

Attachment 190537 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/16845001

[Build Bot]

Comment on attachment 190537 [details]
Patch

Attachment 190537 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/16826071

[Build Bot]

Comment on attachment 190537 [details]
Patch

Attachment 190537 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/16804057

[Balazs Kelemen]

Created attachment 190734 [details]
Patch

[Build Bot]

Comment on attachment 190734 [details]
Patch

Attachment 190734 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/16787132

[Balazs Kelemen]

Created attachment 190736 [details]
Patch

[Balazs Kelemen]

Created attachment 190737 [details]
Patch

Yet another buildfix

[Build Bot]

Comment on attachment 190737 [details]
Patch

Attachment 190737 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/16803283

[Build Bot]

Comment on attachment 190737 [details]
Patch

Attachment 190737 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/16779705

[Balazs Kelemen]

Created attachment 190746 [details]
Patch

[Build Bot]

Comment on attachment 190746 [details]
Patch

Attachment 190746 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/16798153

[Build Bot]

Comment on attachment 190746 [details]
Patch

Attachment 190746 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/16842246

[Balazs Kelemen]

Created attachment 190948 [details]
Patch

[Balazs Kelemen]

Any chance to get reviewed by an owner? Actually, I would need somebody showing some love for making the network process cross-platform and nursing my patches blocking bug 108832.

[Thiago Marcos P. Santos]

Comment on attachment 190948 [details]
Patch

The patch looks good to me, but I would like to suggest renaming all the references of "port" to "identifier".

[Thiago Marcos P. Santos]

Comment on attachment 190948 [details]
Patch

s/Aquire/Acquire

[Brady Eidson]

(In reply to comment #16)
> Any chance to get reviewed by an owner? 

Anders needs to review this.

[Anders Carlsson]

I'd rather we did this once we have move semantics for IPC objects.

[Kenneth Rohde Christiansen]

(In reply to comment #20)
> I'd rather we did this once we have move semantics for IPC objects.

Any ETA on that? Could you shortly explain what you are planning. Thiago Santos expressed some interest in helping out with CoreIPC.

[Balazs Kelemen]

(In reply to comment #17)
> (From update of attachment 190948 [details])
> The patch looks good to me, but I would like to suggest renaming all the references of "port" to "identifier".

Makes sense.

[Balazs Kelemen]

Created attachment 191959 [details]
Patch

[Balazs Kelemen]

(In reply to comment #23)
> Created an attachment (id=191959) [details]
> Patch

Removed the "port" terminology from the common interfaces.

[Thiago Marcos P. Santos]

Could someone please review this patch?

[Brady Eidson]

(In reply to comment #25)
> Could someone please review this patch?

I believe Anders already expressed that we should wait on this.  Perhaps he can give more details.

[Balazs Kelemen]

He mentioned we should have move semantics for this, but I wonder how should we use it. I guess he think about these parts:
+    enum ConnectionRequirement {
+        AcquireSendRight,
+        ForwardSendRight
+    };
+
+    Attachment(Connection::Identifier, ConnectionRequirement);

and where it is used, for example

+    Attachment clientPort(descriptor.clientIdentifier(), Attachment::AcquireSendRight);
     parentProcessConnection()->send(Messages::PluginProcessProxy::DidCreateWebProcessConnection(clientPort, m_supportsAsynchronousPluginInitialization), 0);
or
+    reply->send(CoreIPC::Attachment::nullConnection(CoreIPC::Attachment::ForwardSendRight));

However, since there are 2 different ways we want to "move" an Attachment, I don't think move constructors really help here.
But I'm just guessing this, probably he was thinking about something different.
In any case we can refactor the code later, so I don't really understand why does it have to block the patch.

[Lamarque V. Souza]

Anders, can you comment on what should be fixed in this patch?

[Kwang Yul Seo]

Any progress?

[Kwang Yul Seo]

Created attachment 206130 [details]
Patch

[Kwang Yul Seo]

(In reply to comment #30)
> Created an attachment (id=206130) [details]
> Patch

I rebased the original patch on top of the HEAD.

In the mean time, I found a bug in the original patch.

Connection::ConnectionDescriptor Connection::createPlatformConnection()
{
    int sockets[2];
    RELEASE_ASSERT(socketpair(AF_UNIX, SOCKET_TYPE, 0, sockets) != -1);

    // Avoid exposing the server socket to the child process or the client socket to any future child.
    for (unsigned i = 0; i < 2; ++i) {
        while (fcntl(sockets[i], F_SETFD, FD_CLOEXEC) == -1)
            RELEASE_ASSERT(errno == EINTR);
    }

    return ConnectionDescriptor(sockets[0], sockets[1]);
}

"or the client socket to any future child." applies only to NetworkProcess::createNetworkConnectionToWebProcess and SharedWorkerProcess::createNetworkConnectionToWebProcess. ProcessLauncher should not set FD_CLOEXEC on the client socket. 

I changed the method to set FD_CLOEXEC only on the server socket. But now I am not sure how to handle NetworkProcess::createNetworkConnectionToWebProcess and SharedWorkerProcess::createNetworkConnectionToWebProcess. 

I will investigate this issue more.

[Balazs Kelemen]

(In reply to comment #31)
> (In reply to comment #30)
> > Created an attachment (id=206130) [details] [details]
> > Patch
> 
> I rebased the original patch on top of the HEAD.
> 
> In the mean time, I found a bug in the original patch.
> 
> Connection::ConnectionDescriptor Connection::createPlatformConnection()
> {
>     int sockets[2];
>     RELEASE_ASSERT(socketpair(AF_UNIX, SOCKET_TYPE, 0, sockets) != -1);
> 
>     // Avoid exposing the server socket to the child process or the client socket to any future child.
>     for (unsigned i = 0; i < 2; ++i) {
>         while (fcntl(sockets[i], F_SETFD, FD_CLOEXEC) == -1)
>             RELEASE_ASSERT(errno == EINTR);
>     }
> 
>     return ConnectionDescriptor(sockets[0], sockets[1]);
> }
> 
> "or the client socket to any future child." applies only to NetworkProcess::createNetworkConnectionToWebProcess and SharedWorkerProcess::createNetworkConnectionToWebProcess. ProcessLauncher should not set FD_CLOEXEC on the client socket. 
> 
> I changed the method to set FD_CLOEXEC only on the server socket. But now I am not sure how to handle NetworkProcess::createNetworkConnectionToWebProcess and SharedWorkerProcess::createNetworkConnectionToWebProcess. 
> 
> I will investigate this issue more.

You are right. It's good to set FD_CLOEXEC on the server socket (the one the creator of the connection use) but only after the connection with the child has been set up. I am not sure how it was not breaking things for me (with my local branch where I pushed all my network process patches together), maybe I just did not test this version of the patch.
You don't need to do any magic, just explicitly express the difference of these situations. I would add functions that calls fcntl on the client and server sockets. These will be noops on Mac. But maybe you can get up with a nicer approach.

[Balazs Kelemen]

Comment on attachment 206130 [details]
Patch

You should solve the issue with FD_CLOEXEC first.

[Kwang Yul Seo]

(In reply to comment #32)
> You are right. It's good to set FD_CLOEXEC on the server socket (the one the creator of the connection use) but only after the connection with the child has been set up. I am not sure how it was not breaking things for me (with my local branch where I pushed all my network process patches together), maybe I just did not test this version of the patch.

Why "only after the connection with the child has been set up"? We should set FD_CLOEXEC on the server socket (the one the creator of the connection use) immediately after creating a socketpair because we want to prevent future child processes from accessing the server socket. This is the current behavior and I didn't change it.

Setting FD_CLOEXEC on the client socket seems to have a different requirement. Here's my understanding.

PluginProcess passes its client socket to the UIProcess because UIProcess relays the socket to the requesting WebProcess. This means the UIProcess also gets the client socket and the future descendants processes of the UIProess implicitly inherits all these client sockets. To prevent this problem, PluginProcess sets the FD_CLOEXEC on the client socket before sending it to the UIProcess. 

(The Mac port prevents this by sending the port to the WebProcess with MACH_MSG_TYPE_MOVE_SEND option though it seems more strict.)

Bug this is redundant because we already set FD_CLOEXEC on every file descriptor we pass over UNIX domain sockets in readBytesFromSocket. That means when the client socket is passed to the UIProcess, we set FD_CLOEXEC on the socket. So the future child processes of the UIProcess can't access to this socket.

static ssize_t readBytesFromSocket(int socketDescriptor, uint8_t* buffer, int count, int* fileDescriptors, size_t* fileDescriptorsCount)
{
...
        bool found = false;
        struct cmsghdr* controlMessage;
        for (controlMessage = CMSG_FIRSTHDR(&message); controlMessage; controlMessage = CMSG_NXTHDR(&message, controlMessage)) {
            if (controlMessage->cmsg_level == SOL_SOCKET && controlMessage->cmsg_type == SCM_RIGHTS) {
                *fileDescriptorsCount = (controlMessage->cmsg_len - CMSG_LEN(0)) / sizeof(int);
                memcpy(fileDescriptors, CMSG_DATA(controlMessage), sizeof(int) * *fileDescriptorsCount);

                for (size_t i = 0; i < *fileDescriptorsCount; ++i) {
                    while (fcntl(fileDescriptors[i], F_SETFL, FD_CLOEXEC) == -1) {
                        if (errno != EINTR) {
                            ASSERT_NOT_REACHED();
                            break;
                        }   
                    }   
                }   

                found = true;
                break;
            }   
        }   

So I think that this patch has no behavior change. But I am not an expert on UNIX domain sockets. So please correct me if I'm wrong or misunderstood something.

[Kwang Yul Seo]

(In reply to comment #34)
> (The Mac port prevents this by sending the port to the WebProcess with MACH_MSG_TYPE_MOVE_SEND option though it seems more strict.)

Correction: Mach ports are not inherited. So there is no such problem in the Mac port.

This prevents UIProcess from possessing the port used for communication between PluginProcess and WebProcess. We don't enforce this on Unix domain sockets yet.

[Balazs Kelemen]

(In reply to comment #34)
> (In reply to comment #32)
> > You are right. It's good to set FD_CLOEXEC on the server socket (the one the creator of the connection use) but only after the connection with the child has been set up. I am not sure how it was not breaking things for me (with my local branch where I pushed all my network process patches together), maybe I just did not test this version of the patch.
> 
> Why "only after the connection with the child has been set up"? We should set FD_CLOEXEC on the server socket (the one the creator of the connection use) immediately after creating a socketpair because we want to prevent future child processes from accessing the server socket. This is the current behavior and I didn't change it.

You was mentioning that ProcessLauncher should not set FD_CLOEXEC on the client socket, I was reacting on this. I think this is true because want the child (which we are launching) to inherit the socket. What I was saying is that we can still set the flag after the child has been started (it has finished calling exec).

The situation is different when we are creating the socket at the time when both processes already exist (like at NetworkProcess::createNetworkConnectionToWebProcess). In this case we can set the FD_CLOEXEC flag immediately.

[Kwang Yul Seo]

(In reply to comment #36)
> You was mentioning that ProcessLauncher should not set FD_CLOEXEC on the client socket, I was reacting on this. I think this is true because want the child (which we are launching) to inherit the socket. What I was saying is that we can still set the flag after the child has been started (it has finished calling exec).

Yeah, you and I speak the same language.
 
> The situation is different when we are creating the socket at the time when both processes already exist (like at NetworkProcess::createNetworkConnectionToWebProcess). In this case we can set the FD_CLOEXEC flag immediately.

Yes, I agree. But the comment in SharedWorkerProcess::createWebProcessConnection is misleading.

// Don't expose the shared worker process socket to possible future web processes.
while (fcntl(sockets[0], F_SETFD, FD_CLOEXEC) == -1) {
...
}

This code sets the FD_CLOEXEC flag on the client socket immediately to prevent future child processes of SharedWorkerProcess from inheriting the client socket. But the comment says that "Don't expose the shared worker process socket to possible future web processes". 

As I mentioned in Comment #34, ConnectionUnix sets the FD_CLOEXEC flag on every file descriptor passed over unix domain sockets. So shared worker process sockets are never exposed to possible future web processes anyway without this code.

So if our intention is actually what the comment says, then I don't think we need to set the FD_CLOEXEC flag on the client socket here because SharedWorkerProcess never forks off web processes.

[Kwang Yul Seo]

Created attachment 207421 [details]
Patch

[Kwang Yul Seo]

(In reply to comment #38)
> Created an attachment (id=207421) [details]
> Patch

Rebased to the HEAD.

Anders, would you review the patch? This patch has been in dust for a few months hoping for love.

[Csaba Osztrogonác]

ping for review?

[Csaba Osztrogonác]

Created attachment 212706 [details]
Patch

[Csaba Osztrogonác]

WK2 owners, could you review this patch please? 
( There weren't any comments, reviews, objections in the last 2 months. :( )

[Csaba Osztrogonác]

(In reply to comment #41)
> Created an attachment (id=212706) [details]
> Patch

I updated the previous patch to ToT:
 - Removed the SharedWorkerProcess change, because it doesn't exist at all after r156277.
 - Resolved a trivial conflict in ProcessLauncherEfl.cpp after r156063.

[Darin Adler]

Comment on attachment 212706 [details]
Patch

I don’t know when he’ll have time, but I think Anders i the best person to review this.

[Csaba Osztrogonác]

Created attachment 212980 [details]
Patch

Updated after r156508.

[Csaba Osztrogonác]

(In reply to comment #44)
> (From update of attachment 212706 [details])
> I don’t know when he’ll have time, but I think Anders i the best person to review this.

(In reply to comment #45)
> Created an attachment (id=212980) [details]
> Patch
> 
> Updated after r156508.

Anders, could you review this patch please?

[Carlos Garcia Campos]

Comment on attachment 212980 [details]
Patch

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

> Source/WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp:57
> +    // Make child process inherit parent's locale.
> +    g_setenv("LC_ALL", setlocale(LC_ALL, 0), TRUE);

Why are you bringing this back? this was removed in r145081, see bug #111398.

[Csaba Osztrogonác]

(In reply to comment #47)
> (From update of attachment 212980 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=212980&action=review
> 
> > Source/WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp:57
> > +    // Make child process inherit parent's locale.
> > +    g_setenv("LC_ALL", setlocale(LC_ALL, 0), TRUE);
> 
> Why are you bringing this back? this was removed in r145081, see bug #111398.

I think accidentally, because the original patch in 
this bug is older than bug111398. I'll remove it.

[Csaba Osztrogonác]

Created attachment 213361 [details]
Patch

removed localization change, removed Qt related change, resolved trivial conflicts

[Csaba Osztrogonác]

(In reply to comment #49)
> Created an attachment (id=213361) [details]
> Patch
> 
> removed localization change, removed Qt related change, resolved trivial conflicts


Anders, could you review this patch please?

[Csaba Osztrogonác]

Add more WK2 owner, maybe somebody have a time to review it.

[Csaba Osztrogonác]

ping for review, please

[Csaba Osztrogonác]

WK2 owners, could you review this patch please?

[Csaba Osztrogonác]

ping?

[Csaba Osztrogonác]

ping?

[Csaba Osztrogonác]

WK2 owners, ping for review please

[Csaba Osztrogonác]

ping

[Csaba Osztrogonác]

ping

[Anders Carlsson]

(In reply to comment #58)
> ping

Stop!

Please! There are 39 people cced on this bug who will get an e-mail every time. 

I understand that you want this patch to be reviewed, but with every ping you're pissing more and more people of which will be less and less willing to do the review. This patch is on my review list but I have more important things to attend to right now.

[Csaba Osztrogonác]

(In reply to comment #59)
> (In reply to comment #58)
> > ping
> Stop!

> Please! There are 39 people cced on this bug who will get an e-mail every time. 

If somebody isn't interested / doesn't have expertise in this topic, feel
free to remove himself/herself from the cc list. I won't be angry, I promise
;)

> I understand that you want this patch to be reviewed, but with every ping you're pissing more and more people of which will be less and less willing to do the review. This patch is on my review list but I have more important things to attend to right now.

Of course, I can understand that you have more important things. But there
are 18 WK2 owner, you aren't only one. The origial patch was prepared 9
months before, the lateset patch is 19 days old. And we didn't get _any_
review from _any_ WK2 owner. It is very very disappointing. :(((( 

It would be great if you could give us an ETA for the review time. How much
should we wait, and how many times should we update the patch to ToT again
and again during waiting for review? Is there any chance if you will let other
platforms to enable NetworkProcess building in the near future (in the
following month, year, decade). It is so exhausting to wait, update, wait,
update, wait, update, ask for review again and again. And then getting an r-
because of a minor naming issue and then update and wait again for months ...

It is not about only this bug ... We had to wait months for other bugs too. :(
For example we waited 3 months for https://bugs.webkit.org/show_bug.cgi?id=118520 (which could have been a oneliner, but a WK2 owner
asked this big refactoring ...) and then you rolled it out without any
notification and comment about what was the problem and didn't give any 
hint how can we fix it.

So is there any chance that WK2 owners will be a little bit 
more responsive in the near future, please?

[Brady Eidson]

Just to point out what may not be obvious:  While WK2 patches need a WK2 owner to review, that's not *all* they need.

There's *way* fewer CoreIPC experts than there are WK2 owners, and this patch needs a close eye from such an expert.

[Balazs Kelemen]

This patch is a simple refactoring, no deep CoreIPC knowledge needed to understand it and decide whether it makes sense. It does not change any behavior or internal mechanisms.

[Csaba Osztrogonác]

In my opinion it isn't correct that you asked this refactoring in
https://bugs.webkit.org/show_bug.cgi?id=110093#c6 and then Balázs
did it in few days, but you hadn't any time in this ~8 months to
review the patch you asked.

If you don't have any time for reviewing refactoring you explicitly asked,
please revise your original opinion and let the much more simpler patch to
the trunk - https://bugs.webkit.org/attachment.cgi?id=205781 .

( I reopened https://bugs.webkit.org/show_bug.cgi?id=110093 and asked review again. )

[Csaba Osztrogonác]

Created attachment 217598 [details]
Patch

updated to ToT

[Build Bot]

Comment on attachment 217598 [details]
Patch

Attachment 217598 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/32608047

[Build Bot]

Comment on attachment 217598 [details]
Patch

Attachment 217598 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/32528052

[Csaba Osztrogonác]

Comment on attachment 217598 [details]
Patch

It seems it needs a fix after https://trac.webkit.org/changeset/158890

I'm going to fix it soon.

[Csaba Osztrogonác]

Created attachment 217605 [details]
Patch

with speculative Mac buildfix after r158890

[Darin Adler]

Comment on attachment 217605 [details]
Patch

I think this should go in as smaller patches. There’s no need for it to all be done at once. In particular, it seems that refactoring the Mac to use this new cross-platform API could be done separately from moving over other platforms. Each of those two patches would be considerably easier to review.

[Build Bot]

Comment on attachment 217605 [details]
Patch

Attachment 217605 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/32948005

[Build Bot]

Comment on attachment 217605 [details]
Patch

Attachment 217605 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/31988183

[Peter Molnar]

Created attachment 230165 [details]
updated patch

Updated the patch to the current ToT.

I think is makes much more sense to submit this patch as a whole, rather than splitting it into multiple parts.

[Peter Molnar]

Created attachment 230168 [details]
updated patch

[Csaba Osztrogonác]

(In reply to comment #73)
> Created an attachment (id=230168) [details]
> updated patch

Anders, could you possibly review this patch in the near future?
The original patch is more than a year old and was updated to ToT
many many times, because none of the WK2 owner had enough time /
were interested in reviewing it. :-(

I still think that WebKit2 would profit much if we could get rid this
copy/paste ifdef hell once instead of adding more copy/paste code to
DatabaseProcess, <new-fancy-feature>Process again and again.

[Éva Balázsfalvi]

Created attachment 239403 [details]
Patch

[Balazs Kelemen]

Comment on attachment 239403 [details]
Patch

I hope there exists a potential reviewer for this. If it doesn't go this time I don't think we should continue this story.

Some comments.

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

> Source/WebKit2/Platform/IPC/Connection.h:131
>      struct SocketPair {

Do we still need this struct?

> Source/WebKit2/Platform/IPC/Connection.h:165
> +    static ConnectionDescriptor createPlatformConnection(unsigned options = SetCloexecOnClient | SetCloexecOnServer);

Can we use WebKitish names like SetCloseOnExec? Maybe an enum class would be nice also :)

> Source/WebKit2/Platform/IPC/unix/ConnectionUnix.cpp:-53
> -#define SOCKET_TYPE SOCK_DGRAM

I wonder if it's actually correct. Maybe QNX wants to use DGRAM? Or maybe we don't care (in which case you should not introduce the check for OS(QNX))? I think it would be better to leave this alone for now, as this patch supposed to be only a refactor.

[Csaba Osztrogonác]

Comment on attachment 239403 [details]
Patch

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

>> Source/WebKit2/Platform/IPC/Connection.h:131
>>      struct SocketPair {
> 
> Do we still need this struct?

Good point, it became unused, we can safely remove it.

>> Source/WebKit2/Platform/IPC/Connection.h:165
>> +    static ConnectionDescriptor createPlatformConnection(unsigned options = SetCloexecOnClient | SetCloexecOnServer);
> 
> Can we use WebKitish names like SetCloseOnExec? Maybe an enum class would be nice also :)

It can be reasonable. But I prefer doing it in a separated patch.

[Csaba Osztrogonác]

Comment on attachment 239403 [details]
Patch

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

>> Source/WebKit2/Platform/IPC/unix/ConnectionUnix.cpp:-53
>> -#define SOCKET_TYPE SOCK_DGRAM
> 
> I wonder if it's actually correct. Maybe QNX wants to use DGRAM? Or maybe we don't care (in which case you should not introduce the check for OS(QNX))? I think it would be better to leave this alone for now, as this patch supposed to be only a refactor.

It seems QNX is Qt cruft, we shouldn't modify this code path at all here.

[Éva Balázsfalvi]

Created attachment 240920 [details]
Patch

[Éva Balázsfalvi]

(In reply to comment #79)
> Created attachment 240920 [details]
> Patch

Updated, based on comment #77 and comment #78.

[Csaba Osztrogonác]

(In reply to comment #80)
> (In reply to comment #79)
> > Created attachment 240920 [details]
> > Patch
> 
> Updated, based on comment #77 and comment #78.

Dear WebKit2 owners, 

is there any chance to get review for this patch?

You explicitly asked this before letting Linux ports use NetworkProcess - 
https://bugs.webkit.org/show_bug.cgi?id=110093#c6 . But Linux ports can't
wait _20 months_ for review and added more ifdefs a year before:
http://trac.webkit.org/changeset/160308

I still think we should remove these ifdef hell, but it is impossible
if WK2 owners don't want to review. (nor negative / nor positive review)

Please review it in the near future. Thanks.

[Darin Adler]

Comment on attachment 240920 [details]
Patch

I can’t answer for the other WebKit2 owners, but for myself the problem is that this patch is just a little to big and broad for me to review. Is there some intermediate step we could take that would be a smaller patch, or is this the smallest first step?

For example, if I was doing this I might do it one platform at a time and then remove the #if as the platforms become identical.

[Anders Carlsson]

Comment on attachment 240920 [details]
Patch

We have a different, more ambitious refactoring of the IPC layer planned. I'll make sure to send an e-mail to webkit-dev once this is closer to a reality.

[Csaba Osztrogonác]

(In reply to comment #83)
> Comment on attachment 240920 [details]
> Patch
> 
> We have a different, more ambitious refactoring of the IPC layer planned.
> I'll make sure to send an e-mail to webkit-dev once this is closer to a
> reality.

Wow, a real review after 3 years waiting. :)

In this case let's close it as wontfix.