WebKit Bugzilla
You need to log in before you can comment on or make changes to this bug.
This would allow us to reuse existing webkit2 code.
Created an attachment (id=116363) [details] patch for review.
(From update of attachment 116363 [details]) causes failures on linux. i'll check it again.
Created an attachment (id=116369) [details] patch for review.
(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)".
(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) ?
(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)?
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)
(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?
(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 :)
Created an attachment (id=116515) [details] patch for review. - changed to use OS(DARWIN)
(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?
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?
(From update of attachment 116515 [details]) Clearing flags on attachment: 116515 Committed r101141: <http://trac.webkit.org/changeset/101141>
All reviewed patches have been landed. Closing bug.