Bug 144560

Created attachment 252295 [details] Patch for OS(DARWIN) and PLATFORM(GTK)

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 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.)

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

Created attachment 252819 [details] Patch

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.

Created attachment 252836 [details] Patch

Here's an updated patch with the file to be added.

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.

(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.

Created attachment 252932 [details] Patch

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 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

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 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

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

(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?

Created attachment 255295 [details] Patch

Here's another, updated, patch with the FourCharCode reinstated. I'm pretty sure taking out the FourCharCode was what caused the test failures.

Darin, as you did a first review of this patch maybe you'd like to review this new version?

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.

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.

Created attachment 264480 [details] Patch

(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 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

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 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

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 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

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

(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...

(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.

Created attachment 264506 [details] Patch

Comment on attachment 264506 [details] Patch Clearing flags on attachment: 264506 Committed r191856: <http://trac.webkit.org/changeset/191856>

All reviewed patches have been landed. Closing bug.

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) ?

(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.

(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.