WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
72495
[Qt][WK2][Mac] Use Mac port's IPC implementation instead of Unix sockets
https://bugs.webkit.org/show_bug.cgi?id=72495
Summary
[Qt][WK2][Mac] Use Mac port's IPC implementation instead of Unix sockets
Zeno Albisser
Reported
2011-11-16 05:53:44 PST
This would allow us to reuse existing webkit2 code.
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
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Zeno Albisser
Comment 1
2011-11-23 08:33:59 PST
Created
attachment 116363
[details]
patch for review.
Zeno Albisser
Comment 2
2011-11-23 08:37:06 PST
Comment on
attachment 116363
[details]
patch for review. causes failures on linux. i'll check it again.
Zeno Albisser
Comment 3
2011-11-23 08:54:53 PST
Created
attachment 116369
[details]
patch for review.
Balazs Kelemen
Comment 4
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)".
Kenneth Rohde Christiansen
Comment 5
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) ?
Simon Hausmann
Comment 6
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)?
Simon Hausmann
Comment 7
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)
Zeno Albisser
Comment 8
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?
Simon Hausmann
Comment 9
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 :)
Zeno Albisser
Comment 10
2011-11-24 08:10:25 PST
Created
attachment 116515
[details]
patch for review. - changed to use OS(DARWIN)
Simon Hausmann
Comment 11
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?
Zeno Albisser
Comment 12
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?
Zeno Albisser
Comment 13
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
>
Zeno Albisser
Comment 14
2011-11-24 08:51:36 PST
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug