Bug 72495 - [Qt][WK2][Mac] Use Mac port's IPC implementation instead of Unix sockets
Summary: [Qt][WK2][Mac] Use Mac port's IPC implementation instead of Unix sockets
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zeno Albisser
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-16 05:53 PST by Zeno Albisser
Modified: 2011-11-24 08:51 PST (History)
6 users (show)

See Also:


Attachments
patch for review. (16.63 KB, patch)
2011-11-23 08:33 PST, Zeno Albisser
no flags Details | Formatted Diff | Diff
patch for review. (16.39 KB, patch)
2011-11-23 08:54 PST, Zeno Albisser
no flags 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 Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zeno Albisser 2011-11-16 05:53:44 PST
This would allow us to reuse existing webkit2 code.
Comment 1 Zeno Albisser 2011-11-23 08:33:59 PST
Created attachment 116363 [details]
patch for review.
Comment 2 Zeno Albisser 2011-11-23 08:37:06 PST
Comment on attachment 116363 [details]
patch for review.

causes failures on linux. i'll check it again.
Comment 3 Zeno Albisser 2011-11-23 08:54:53 PST
Created attachment 116369 [details]
patch for review.
Comment 4 Balazs Kelemen 2011-11-24 01:40:40 PST
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)".
Comment 5 Kenneth Rohde Christiansen 2011-11-24 02:15:15 PST
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) ?
Comment 6 Simon Hausmann 2011-11-24 03:40:30 PST
(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)?
Comment 7 Simon Hausmann 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 Zeno Albisser 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 Simon Hausmann 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 Zeno Albisser 2011-11-24 08:10:25 PST
Created attachment 116515 [details]
patch for review. - changed to use OS(DARWIN)
Comment 11 Simon Hausmann 2011-11-24 08:16:58 PST
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?
Comment 12 Zeno Albisser 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])
> 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 Zeno Albisser 2011-11-24 08:51:26 PST
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>
Comment 14 Zeno Albisser 2011-11-24 08:51:36 PST
All reviewed patches have been landed.  Closing bug.