Bug 144560 - [GTK] Fix combinations of PLATFORM(GTK) and OS(DARWIN)
Summary: [GTK] Fix combinations of PLATFORM(GTK) and OS(DARWIN)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified OS X 10.10
: P2 Major
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 126492 144561 150030
  Show dependency treegraph
 
Reported: 2015-05-03 17:14 PDT by Philip Chimento
Modified: 2015-11-10 06:12 PST (History)
12 users (show)

See Also:


Attachments
Patch for OS(DARWIN) and PLATFORM(GTK) (12.42 KB, patch)
2015-05-03 17:14 PDT, Philip Chimento
ossy: review-
Details | Formatted Diff | Diff
Patch for 2.8.0 (18.15 KB, patch)
2015-05-08 00:19 PDT, Philip Chimento
no flags Details | Formatted Diff | Diff
Patch (20.27 KB, patch)
2015-05-10 11:45 PDT, Philip Chimento
no flags Details | Formatted Diff | Diff
Patch (20.64 KB, patch)
2015-05-10 19:49 PDT, Philip Chimento
no flags Details | Formatted Diff | Diff
Patch (30.56 KB, patch)
2015-05-11 21:45 PDT, Philip Chimento
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-mavericks (609.15 KB, application/zip)
2015-05-11 22:38 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-mavericks-wk2 (609.35 KB, application/zip)
2015-05-11 22:42 PDT, Build Bot
no flags Details
Patch (34.16 KB, patch)
2015-06-20 12:16 PDT, Philip Chimento
no flags Details | Formatted Diff | Diff
Patch (32.84 KB, patch)
2015-10-31 11:50 PDT, Philip Chimento
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-mavericks (664.42 KB, application/zip)
2015-10-31 12:54 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-mavericks-wk2 (760.74 KB, application/zip)
2015-10-31 12:57 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-yosemite (798.79 KB, application/zip)
2015-10-31 12:58 PDT, Build Bot
no flags Details
Patch (34.56 KB, patch)
2015-10-31 22:59 PDT, Philip Chimento
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philip Chimento 2015-05-03 17:14:01 PDT
Understandably, OS(DARWIN) and PLATFORM(GTK) are not exercised together very often. There are lots of places where OS(DARWIN) enables some header or library that is not included in the GTK platform build.

There would seem to be two general approaches to solve this:

1. Change PlatformGTK.cmake and OptionsGTK.cmake to add the required libraries and includes for OSX when CMAKE_SYSTEM_NAME MATCHES "Darwin".

2. Change the ifdef guards so that components from the GTK platform are preferred in the GTK build, rather than OSX system components.

I have a preference for #2 since that way WebKitGTK has pretty much the same dependencies whether it's compiled on Linux or OSX. That's what this patch does.
Comment 1 Philip Chimento 2015-05-03 17:14:54 PDT
Created attachment 252295 [details]
Patch for OS(DARWIN) and PLATFORM(GTK)
Comment 2 WebKit Commit Bot 2015-05-03 17:18:05 PDT
Attachment 252295 [details] did not pass style-queue:


Total errors found: 0 in 0 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Csaba Osztrogonác 2015-05-04 03:30:03 PDT
Comment on attachment 252295 [details]
Patch for OS(DARWIN) and PLATFORM(GTK)

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

Please add changelog with prepare-changelog, see
https://www.webkit.org/coding/contributing.html for details.

Additionally we will need review from WK2 owners too.

> b/Source/WebCore/platform/graphics/opentype/OpenTypeMathData.cpp:43
> -#if OS(DARWIN)
> +#if OS(DARWIN) && !PLATFORM(GTK)

Why not simply "#if PLATFORM(COCOA)" ?
(Not only here, everywhere.)
Comment 4 Philip Chimento 2015-05-08 00:19:40 PDT
Created attachment 252695 [details]
Patch for 2.8.0

Here's an updated patch for 2.8.0 with your suggestion and also with a changelog.

If I can find time, then I will try to make a patch that applies to master. But due to the esoteric platform I'm compiling on, it's often not worth it to try to get unstable versions to work. Any help porting these patches to master would be appreciated.

In any case, I'll propose it for merge to 2.8.2 on https://trac.webkit.org/wiki/WebKitGTK/2.8.x
Comment 5 Philip Chimento 2015-05-10 11:45:02 PDT
Created attachment 252819 [details]
Patch
Comment 6 Philip Chimento 2015-05-10 11:50:36 PDT
Here's a patch for master.

Additionally it needs the following:

--- /dev/null	2015-05-09 12:32:34.000000000 -0700
+++ a/Source/bmalloc/PlatformGTK.cmake	2015-05-09 12:33:13.000000000 -0700
@@ -0,0 +1,5 @@
+if (CMAKE_SYSTEM_NAME MATCHES "Darwin")
+    list(APPEND bmalloc_SOURCES
+        bmalloc/Zone.cpp
+    )
+endif ()

I don't know how to get webkit-patch to recognize that.
Comment 7 Philip Chimento 2015-05-10 19:49:39 PDT
Created attachment 252836 [details]
Patch
Comment 8 Philip Chimento 2015-05-10 19:50:39 PDT
Here's an updated patch with the file to be added.
Comment 9 Darin Adler 2015-05-11 09:24:02 PDT
Comment on attachment 252836 [details]
Patch

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

Most of those OS(DARWIN) -> PLATFORM(COCOA) changes don’t seem right to me. I added comments about a few of them. Others sound comment on the use inside WebKit2.

> Source/WTF/wtf/WorkQueue.h:39
> -#if OS(DARWIN)
> +#if PLATFORM(COCOA)
>  #include <dispatch/dispatch.h>
>  #endif

It’s a little sad to go in this direction. Better to use the OS ones than the PLATFORM ones whenever possible.

> Source/WTF/wtf/WorkQueue.h:76
> -#if OS(DARWIN)
> +#if PLATFORM(COCOA)
>      dispatch_queue_t dispatchQueue() const { return m_dispatchQueue; }
>  #elif PLATFORM(GTK)

Better to just put PLATFORM(GTK) first.

> Source/WTF/wtf/WorkQueue.h:106
> -#if OS(DARWIN)
> +#if PLATFORM(COCOA)
>      static void executeFunction(void*);
>      dispatch_queue_t m_dispatchQueue;
>  #elif PLATFORM(GTK)

Better to just put PLATFORM(GTK) first.

> Source/WebCore/platform/graphics/PlatformDisplay.cpp:49
>  #if PLATFORM(GTK)
> +#include <gdk/gdk.h>
> +#if ENABLE(X11_TARGET)
>  #include <gdk/gdkx.h>
>  #endif
> +#endif

To keep includes clean we prefer this style:

    #if PLATFORM(GTK)
    #include <gdk/gdk.h>
    #endif

    #if PLATFORM(GTK) && ENABLE(X11_TARGET)
    #ijnclude <gdk/gdkx.h>
    #endif

The nested includes are harder to read and have no other obvious advantages.

> Source/WebCore/platform/graphics/opentype/OpenTypeMathData.cpp:45
> +#if PLATFORM(COCOA)
>  const FourCharCode MATHTag = 'MATH';
>  #else

Just get rid of this and use the other version everywhere, including Cocoa/Darwin.
Comment 10 Philip Chimento 2015-05-11 20:07:48 PDT
(In reply to comment #9)

Thanks, I'll update the patch with these comments.

> Most of those OS(DARWIN) -> PLATFORM(COCOA) changes don’t seem right to me.
> I added comments about a few of them. Others sound comment on the use inside
> WebKit2.
> 
> > Source/WTF/wtf/WorkQueue.h:39
> > -#if OS(DARWIN)
> > +#if PLATFORM(COCOA)
> >  #include <dispatch/dispatch.h>
> >  #endif
> 
> It’s a little sad to go in this direction. Better to use the OS ones than
> the PLATFORM ones whenever possible.

My previous version had #if OS(DARWIN) && !PLATFORM(GTK) everywhere. That works for me as well, but everyone should agree on one and stick with it ;-)

I do prefer Csaba's suggestion of PLATFORM(COCOA) because the way I think of "platform" it should use mainly the same dependencies no matter what OS it's compiled on. For me, differences between the OSs should limit themselves to e.g. system calls (an example of what I mean is https://bugs.webkit.org/show_bug.cgi?id=144554). But, I'm happy to go either way with this.
Comment 11 Philip Chimento 2015-05-11 21:45:35 PDT
Created attachment 252932 [details]
Patch
Comment 12 Philip Chimento 2015-05-11 21:47:38 PDT
Turns out that most of the #if PLATFORM(COCOA) checks can be replaced by moving #if USE(UNIX_DOMAIN_SOCKETS) above #if OS(DARWIN), if that's the preferred way.

I'll rebuild on my end with this patch tonight.
Comment 13 Build Bot 2015-05-11 22:38:00 PDT
Comment on attachment 252932 [details]
Patch

Attachment 252932 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5444813113524224

New failing tests:
mathml/opentype/opentype-stretchy-horizontal.html
mathml/opentype/opentype-stretchy.html
Comment 14 Build Bot 2015-05-11 22:38:04 PDT
Created attachment 252937 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 15 Build Bot 2015-05-11 22:42:23 PDT
Comment on attachment 252932 [details]
Patch

Attachment 252932 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6599436687900672

New failing tests:
mathml/opentype/opentype-stretchy-horizontal.html
mathml/opentype/opentype-stretchy.html
Comment 16 Build Bot 2015-05-11 22:42:26 PDT
Created attachment 252938 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 17 Philip Chimento 2015-05-11 22:44:38 PDT
(In reply to comment #9)
> > Source/WebCore/platform/graphics/opentype/OpenTypeMathData.cpp:45
> > +#if PLATFORM(COCOA)
> >  const FourCharCode MATHTag = 'MATH';
> >  #else
> 
> Just get rid of this and use the other version everywhere, including
> Cocoa/Darwin.

It seems like the test failure probably has something to do with that?
Comment 18 Philip Chimento 2015-06-20 12:16:15 PDT
Created attachment 255295 [details]
Patch
Comment 19 Philip Chimento 2015-06-20 12:18:40 PDT
Here's another, updated, patch with the FourCharCode reinstated. I'm pretty sure taking out the FourCharCode was what caused the test failures.
Comment 20 Philippe Normand 2015-10-22 08:50:01 PDT
Darin, as you did a first review of this patch maybe you'd like to review this new version?
Comment 21 Darin Adler 2015-10-24 14:54:05 PDT
Comment on attachment 255295 [details]
Patch

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

For the long term health of our configuration macros: Use of PLATFORM(XXX) is to be minimized. PLATFORM(COCOA) for low level stuff is particularly bad.

> Source/WebCore/platform/graphics/PlatformDisplay.cpp:51
>  #if PLATFORM(GTK)
> +#include <gdk/gdk.h>
> +#endif
> +
> +#if PLATFORM(GTK)
>  #if PLATFORM(X11)
>  #include <gdk/gdkx.h>
>  #endif

Rather than nesting, I prefer to write each condition out separately. It would look like this:

#if PLATFORM(GTK)
#include <gdk/gk.h>
#endif

#if PLATFORM(GTK) && PLATFORM(X11)
#include <gdk/gdkx.h>
#endif

#if PLATFORM(GTK) && PLATFORM(WAYLAND) && !defined(GTK_API_VERSION_2)
#include <gdk/gdkwayland.h>
#endif

This is more tidy than the nested way of writing it, especially with all the "#endif // PLATFORM(GTK)" gunk.

> Source/WebCore/platform/graphics/opentype/OpenTypeMathData.cpp:46
> -#if OS(DARWIN)
> +#if PLATFORM(COCOA)
>  const FourCharCode MATHTag = 'MATH';
>  #else
>  const uint32_t MATHTag = OT_MAKE_TAG('M', 'A', 'T', 'H');

I don’t know why we need this special case for PLATFORM(COCOA); can’t we always use OT_MAKE_TAG?

> Source/WebKit2/Platform/IPC/Attachment.cpp:39
> -#if OS(DARWIN)
> +#if PLATFORM(COCOA)

This is wrong. It should match the header and say:

#if OS(DARWIN) && !USE(UNIX_DOMAIN_SOCKETS)

> Source/WebKit2/UIProcess/Launcher/ProcessLauncher.cpp:61
> -#if OS(DARWIN)
> +#if PLATFORM(COCOA)

This doesn’t seem quite right to me, but I can’t come up with anything better. PLATFORM(COCOA) is a really high level concept and it not the right conditional for “use Mach directly”.

> Source/WebKit2/UIProcess/Launcher/ProcessLauncher.h:64
> +#if PLATFORM(COCOA)

Ditto.
Comment 22 Michael Catanzaro 2015-10-26 23:48:06 PDT
Philip, just in case you haven't figured this out yet: we use r+ cq- to indicate the patch is accepted conditional on minor changes, no further review required. If you use 'webkit-patch apply-from-bug' (probably with --no-update), fix the patch as requested, and then then 'webkit-patch upload --no-review', you the resulting patch will have the reviewed by line and you can mark the patch commit-queue? without needing to use review? again.
Comment 23 Philip Chimento 2015-10-31 11:50:21 PDT
Created attachment 264480 [details]
Patch
Comment 24 Philip Chimento 2015-10-31 11:51:17 PDT
(In reply to comment #21)
> > Source/WebKit2/UIProcess/Launcher/ProcessLauncher.cpp:61
> > -#if OS(DARWIN)
> > +#if PLATFORM(COCOA)
> 
> This doesn’t seem quite right to me, but I can’t come up with anything
> better. PLATFORM(COCOA) is a really high level concept and it not the right
> conditional for “use Mach directly”.
> 
> > Source/WebKit2/UIProcess/Launcher/ProcessLauncher.h:64
> > +#if PLATFORM(COCOA)
> 
> Ditto.

How about OS(DARWIN) && !PLATFORM(GTK) for now? Maybe this should eventually be transformed into USE(MACH) or some such macro. I looked through wtf/Platform.h but didn't see anything appropriate that already exists.

(In reply to comment #22)
> Philip, just in case you haven't figured this out yet: we use r+ cq- to
> indicate the patch is accepted conditional on minor changes, no further
> review required.

Thank you for setting me straight! Indeed I had not figured that out. I've uploaded another patch with the modifications, but I haven't built it yet, so won't mark it cq? until I've done so later today.
Comment 25 Build Bot 2015-10-31 12:54:10 PDT
Comment on attachment 264480 [details]
Patch

Attachment 264480 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/364003

New failing tests:
mathml/opentype/opentype-stretchy-horizontal.html
mathml/opentype/opentype-stretchy.html
Comment 26 Build Bot 2015-10-31 12:54:14 PDT
Created attachment 264484 [details]
Archive of layout-test-results from ews101 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 27 Build Bot 2015-10-31 12:57:10 PDT
Comment on attachment 264480 [details]
Patch

Attachment 264480 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/364004

New failing tests:
mathml/opentype/opentype-stretchy-horizontal.html
mathml/opentype/opentype-stretchy.html
Comment 28 Build Bot 2015-10-31 12:57:14 PDT
Created attachment 264485 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 29 Build Bot 2015-10-31 12:58:44 PDT
Comment on attachment 264480 [details]
Patch

Attachment 264480 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/364006

New failing tests:
mathml/opentype/opentype-stretchy-horizontal.html
mathml/opentype/opentype-stretchy.html
Comment 30 Build Bot 2015-10-31 12:58:48 PDT
Created attachment 264486 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 31 Philip Chimento 2015-10-31 14:37:48 PDT
(In reply to comment #21)
> > Source/WebCore/platform/graphics/opentype/OpenTypeMathData.cpp:46
> > -#if OS(DARWIN)
> > +#if PLATFORM(COCOA)
> >  const FourCharCode MATHTag = 'MATH';
> >  #else
> >  const uint32_t MATHTag = OT_MAKE_TAG('M', 'A', 'T', 'H');
> 
> I don’t know why we need this special case for PLATFORM(COCOA); can’t we
> always use OT_MAKE_TAG?

OT_MAKE_TAG breaks the Mac EWS build. Come to think of it, now I remember trying that earlier...
Comment 32 Darin Adler 2015-10-31 15:45:22 PDT
(In reply to comment #31)
> OT_MAKE_TAG breaks the Mac EWS build. Come to think of it, now I remember
> trying that earlier...

Not your responsibility to do this, but maybe someone can fix that instead of continuing to work around it with an #if at every use of that macro. We really don’t want to keep having to do an #if every time we deal with a tag cross-platform.
Comment 33 Philip Chimento 2015-10-31 22:59:35 PDT
Created attachment 264506 [details]
Patch
Comment 34 WebKit Commit Bot 2015-11-01 06:24:12 PST
Comment on attachment 264506 [details]
Patch

Clearing flags on attachment: 264506

Committed r191856: <http://trac.webkit.org/changeset/191856>
Comment 35 WebKit Commit Bot 2015-11-01 06:24:19 PST
All reviewed patches have been landed.  Closing bug.
Comment 36 Carlos Garcia Campos 2015-11-10 05:02:07 PST
Comment on attachment 264506 [details]
Patch

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

> Source/WebKit2/Platform/IPC/unix/ConnectionUnix.cpp:47
> -#ifdef SOCK_SEQPACKET
> +#if defined(SOCK_SEQPACKET) && !PLATFORM(GTK)
>  #define SOCKET_TYPE SOCK_SEQPACKET

This is wrong, we always want to use SOCK_SEQPACKET if available, why did you add !PLATFORM(GTK) ?
Comment 37 Philip Chimento 2015-11-10 06:04:10 PST
(In reply to comment #36)
> Comment on attachment 264506 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264506&action=review
> 
> > Source/WebKit2/Platform/IPC/unix/ConnectionUnix.cpp:47
> > -#ifdef SOCK_SEQPACKET
> > +#if defined(SOCK_SEQPACKET) && !PLATFORM(GTK)
> >  #define SOCKET_TYPE SOCK_SEQPACKET
> 
> This is wrong, we always want to use SOCK_SEQPACKET if available, why did
> you add !PLATFORM(GTK) ?

It's been such a long time since I first wrote this, I don't have my notes anymore on the error message that SOCK_SEQPACKET caused. I can try to compile again without this change and see what the problem was.
Comment 38 Carlos Garcia Campos 2015-11-10 06:12:04 PST
(In reply to comment #37)
> (In reply to comment #36)
> > Comment on attachment 264506 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=264506&action=review
> > 
> > > Source/WebKit2/Platform/IPC/unix/ConnectionUnix.cpp:47
> > > -#ifdef SOCK_SEQPACKET
> > > +#if defined(SOCK_SEQPACKET) && !PLATFORM(GTK)
> > >  #define SOCKET_TYPE SOCK_SEQPACKET
> > 
> > This is wrong, we always want to use SOCK_SEQPACKET if available, why did
> > you add !PLATFORM(GTK) ?
> 
> It's been such a long time since I first wrote this, I don't have my notes
> anymore on the error message that SOCK_SEQPACKET caused. I can try to
> compile again without this change and see what the problem was.

This is disabling SOCK_SEQPACKET for GTK port in all platforms, not only mac. I'm sure it doesn't break the build in linux, since we have always used it.