Bug 72495 - [Qt][WK2][Mac] Use Mac port's IPC implementation instead of Unix sockets
: [Qt][WK2][Mac] Use Mac port's IPC implementation instead of Unix sockets
Status: RESOLVED FIXED
: WebKit
WebKit2
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2011-11-16 05:53 PST by
Modified: 2011-11-24 08:51 PST (History)


Attachments
patch for review. (16.63 KB, patch)
2011-11-23 08:33 PST, Zeno Albisser
no flags Review Patch | Details | Formatted Diff | Diff
patch for review. (16.39 KB, patch)
2011-11-23 08:54 PST, Zeno Albisser
no flags Review Patch | Details | Formatted Diff | Diff
patch for review. - changed to use OS(DARWIN) (15.00 KB, patch)
2011-11-24 08:10 PST, Zeno Albisser
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-11-16 05:53:44 PST
This would allow us to reuse existing webkit2 code.
------- Comment #1 From 2011-11-23 08:33:59 PST -------
Created an attachment (id=116363) [details]
patch for review.
------- Comment #2 From 2011-11-23 08:37:06 PST -------
(From update of attachment 116363 [details])
causes failures on linux. i'll check it again.
------- Comment #3 From 2011-11-23 08:54:53 PST -------
Created an attachment (id=116369) [details]
patch for review.
------- Comment #4 From 2011-11-24 01:40:40 PST -------
(From update of attachment 116369 [details])
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)".
------- Comment #5 From 2011-11-24 02:15:15 PST -------
(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

> 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) ?
------- Comment #6 From 2011-11-24 03:40:30 PST -------
(In reply to comment #5)
> (From update of attachment 116369 [details] [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)?
------- Comment #7 From 2011-11-24 03:42:19 PST -------
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)
------- Comment #8 From 2011-11-24 04:14:55 PST -------
(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?
------- Comment #9 From 2011-11-24 05:48:35 PST -------
(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 :)
------- Comment #10 From 2011-11-24 08:10:25 PST -------
Created an attachment (id=116515) [details]
patch for review. - changed to use OS(DARWIN)
------- Comment #11 From 2011-11-24 08:16:58 PST -------
(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?
------- Comment #12 From 2011-11-24 08:21:37 PST -------
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] [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?
------- Comment #13 From 2011-11-24 08:51:26 PST -------
(From update of attachment 116515 [details])
Clearing flags on attachment: 116515

Committed r101141: <http://trac.webkit.org/changeset/101141>
------- Comment #14 From 2011-11-24 08:51:36 PST -------
All reviewed patches have been landed.  Closing bug.