Bug 72495 – [Qt][WK2][Mac] Use Mac port's IPC implementation instead of Unix sockets

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

[Zeno Albisser]

This would allow us to reuse existing webkit2 code.

[Zeno Albisser]

Created attachment 116363 [details]
patch for review.

[Zeno Albisser]

Comment on attachment 116363 [details]
patch for review.

causes failures on linux. i'll check it again.

[Zeno Albisser]

Created attachment 116369 [details]
patch for review.

[Balazs Kelemen]

Comment on attachment 116369 [details]
patch for review.

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

Just a quick note on the defines.

> Source/WebKit2/Platform/CoreIPC/Attachment.cpp:42
>  {

Could we add a specific USE flag for the kind of IPC Mac is using (and we are going use as well)? This way all the "#if PLATFORM(MAC) || (PLATFORM(QT) && !USE(UNIX_DOMAIN_SOCKETS))" conditions will be only a "USE(MAC_IPC_STUFF)".

[Kenneth Rohde Christiansen]

Comment on attachment 116369 [details]
patch for review.

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

>> Source/WebKit2/Platform/CoreIPC/Attachment.cpp:42

> 
> Could we add a specific USE flag for the kind of IPC Mac is using (and we are going use as well)? This way all the "#if PLATFORM(MAC) || (PLATFORM(QT) && !USE(UNIX_DOMAIN_SOCKETS))" conditions will be only a "USE(MAC_IPC_STUFF)".

Sounds like a good idea. Would be a lot cleaner, but this is also ok for now I think

> Source/WebKit2/Platform/CoreIPC/Attachment.h:29
> +#if PLATFORM(MAC) || (PLATFORM(QT) && !USE(UNIX_DOMAIN_SOCKETS))

I wonder if it would be cleaner (understandable) to say PLATFORM(QT) && OS(DARWIN). We could also make a PLATFORM(QT_MAC) ?

[Simon Hausmann]

(In reply to comment #5)
> (From update of attachment 116369 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=116369&action=review
> 
> >> Source/WebKit2/Platform/CoreIPC/Attachment.cpp:42
> 
> > 
> > Could we add a specific USE flag for the kind of IPC Mac is using (and we are going use as well)? This way all the "#if PLATFORM(MAC) || (PLATFORM(QT) && !USE(UNIX_DOMAIN_SOCKETS))" conditions will be only a "USE(MAC_IPC_STUFF)".
> 
> Sounds like a good idea. Would be a lot cleaner, but this is also ok for now I think

Why a specific feature define? Why not simply replace PLATFORM(MAC) with OS(DARWIN)?

[Simon Hausmann]

I think this is a very good piece of work and definitely the right direction. But before landing I'd love to see the #ifdef stuff sorted out, and my guts feeling is that we should simply use OS(DARWIN). (I might be missing something though)

[Zeno Albisser]

(In reply to comment #7)
> I think this is a very good piece of work and definitely the right direction. But before landing I'd love to see the #ifdef stuff sorted out, and my guts feeling is that we should simply use OS(DARWIN). (I might be missing something though)

I would love to replace "PLATFORM(MAC) || (PLATFORM(QT) && !USE(UNIX_DOMAIN_SOCKETS))" with a simple "OS(DARWIN)". But these two defines are not really equivalent.
In practice though it seems that for WK2 there is nobody else than Apple and us building on Mac (Darwin). So i wonder if that solution would still be acceptable? - Opinions?

[Simon Hausmann]

(In reply to comment #8)
> (In reply to comment #7)
> > I think this is a very good piece of work and definitely the right direction. But before landing I'd love to see the #ifdef stuff sorted out, and my guts feeling is that we should simply use OS(DARWIN). (I might be missing something though)
> 
> I would love to replace "PLATFORM(MAC) || (PLATFORM(QT) && !USE(UNIX_DOMAIN_SOCKETS))" with a simple "OS(DARWIN)". But these two defines are not really equivalent.
> In practice though it seems that for WK2 there is nobody else than Apple and us building on Mac (Darwin). So i wonder if that solution would still be acceptable? - Opinions?

That's exactly the thing. PLATFORM(MAC) implies OS(DARWIN) and building-for-safari. By removing the "building-for-safari" part we're indeed forcing "all" ports of WK2 on the Mac to use the Mac IPC, but I'd consider that a good move :)

[Zeno Albisser]

Created attachment 116515 [details]
patch for review. - changed to use OS(DARWIN)

[Simon Hausmann]

Comment on attachment 116515 [details]
patch for review. - changed to use OS(DARWIN)

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

Looks good to me in general, just wondering about the includes.

> Source/WebKit2/Platform/CoreIPC/Attachment.h:31
> +#include <mach/mach_init.h>
> +#include <mach/mach_traps.h>

Out of curiosity: Why are these includes needed for us (Qt build) and not for the mac build?

> Source/WebKit2/Platform/mac/WorkQueueMac.cpp:29
> +#include <mach/mach_init.h>

Same here, why do we need the include and not the mac build? Is it because of precompiled headers?

[Zeno Albisser]

I wish i would know the answer to that question. I was wondering as well. But didn't figure out the proper answer yet. The includes were not added by accident though. I will check again where they come from incase of a mac build.

(In reply to comment #11)
> (From update of attachment 116515 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=116515&action=review
> 
> Looks good to me in general, just wondering about the includes.
> 
> > Source/WebKit2/Platform/CoreIPC/Attachment.h:31
> > +#include <mach/mach_init.h>
> > +#include <mach/mach_traps.h>
> 
> Out of curiosity: Why are these includes needed for us (Qt build) and not for the mac build?
> 
> > Source/WebKit2/Platform/mac/WorkQueueMac.cpp:29
> > +#include <mach/mach_init.h>
> 
> Same here, why do we need the include and not the mac build? Is it because of precompiled headers?

[Zeno Albisser]

Comment on attachment 116515 [details]
patch for review. - changed to use OS(DARWIN)

Clearing flags on attachment: 116515

Committed r101141: <http://trac.webkit.org/changeset/101141>

[Zeno Albisser]

All reviewed patches have been landed.  Closing bug.