RESOLVED FIXED 144845
[CMake] Some macros need to be defined/undefined, rather than ON/OFF 
https://bugs.webkit.org/show_bug.cgi?id=144845
Summary [CMake] Some macros need to be defined/undefined, rather than ON/OFF
Philip Chimento
Reported 2015-05-10 12:12:16 PDT
I ran into these three errors while compiling the 2.9.1 GTK port with no X11, with MiniBrowser turned on but developer mode off: In file included from /Users/fliep/gtk/source/webkitgtk-2.9.1/Source/WebCore/bindings/js/ScriptController.cpp:39: In file included from /Users/fliep/gtk/source/webkitgtk-2.9.1/Source/WebCore/bridge/NP_jsobject.h:31: In file included from /Users/fliep/gtk/source/webkitgtk-2.9.1/Source/WebCore/bridge/npruntime_internal.h:28: /Users/fliep/gtk/source/webkitgtk-2.9.1/Source/WebCore/plugins/npapi.h:89:10: fatal error: 'X11/Xlib.h' file not found #include <X11/Xlib.h> ^ ------------- /Users/fliep/gtk/source/webkitgtk-2.9.1/Source/WebKit2/PluginProcess/unix/PluginProcessMainUnix.cpp:60:25: error: unknown type name 'Display'; did you mean 'WebCore::EDisplay'? static int webkitXError(Display* xdisplay, XErrorEvent* error) ^~~~~~~ WebCore::EDisplay /Users/fliep/gtk/source/webkitgtk-2.9.1/Source/WebCore/rendering/style/RenderStyleConstants.h:536:6: note: 'WebCore::EDisplay' declared here enum EDisplay { ^ /Users/fliep/gtk/source/webkitgtk-2.9.1/Source/WebKit2/PluginProcess/unix/PluginProcessMainUnix.cpp:60:44: error: unknown type name 'XErrorEvent' static int webkitXError(Display* xdisplay, XErrorEvent* error) ^ --------------- /Users/fliep/gtk/source/webkitgtk-2.9.1/Tools/MiniBrowser/gtk/main.c:260:45: error: use of undeclared identifier 'WEBKIT_INJECTED_BUNDLE_PATH' g_setenv("WEBKIT_INJECTED_BUNDLE_PATH", WEBKIT_INJECTED_BUNDLE_PATH, FALSE); ^ This is because of the preprocessor variables MOZ_X11, XP_UNIX, and DEVELOPMENT_BUILD, respectively, being set to the values ON or OFF by the CMake build system, whereas the code expects them to be either defined or undefined. In the common case where they were ON, they were naturally also defined, and so the problem was not immediately apparent.
Attachments
Patch (1.48 KB, patch)
2015-05-10 12:23 PDT, Philip Chimento 		no flags
DetailsFormatted DiffDiff
Patch (1.68 KB, patch)
2015-05-10 15:02 PDT, Philip Chimento 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Philip Chimento
Comment 1 2015-05-10 12:16:16 PDT
The latter failure (DEVELOPMENT_BUILD) has already been changed on master but Tools/MiniBrowser/gtk/main.c:260 may still need to read #if ENABLE(DEVELOPER_MODE) instead of #if ENABLE_DEVELOPER_MODE - I'm not exactly sure how that WTF mechanism works.
Philip Chimento
Comment 2 2015-05-10 12:23:04 PDT
Created attachment 252822 [details] Patch
Michael Catanzaro
Comment 3 2015-05-10 14:37:26 PDT
Thanks for your patches. I'd like to change the code to be more robust to this, checking values rather than definitions. But I don't see value in fighting that fight in npapi.h, so this is probably the best approach. Let's see what Martin thinks. There should for sure be a warning comment, though, or someone will revert this patch in passing as a cleanup. (In reply to comment #1) > The latter failure (DEVELOPMENT_BUILD) has already been changed on master > but Tools/MiniBrowser/gtk/main.c:260 may still need to read #if > ENABLE(DEVELOPER_MODE) instead of #if ENABLE_DEVELOPER_MODE - I'm not > exactly sure how that WTF mechanism works. ENABLE(DEVELOPER_MODE) is true if ENABLE_DEVELOPER_MODE is defined and its value is not 0, and false otherwise. Since MiniBrowser uses the WebKitGTK+ public API and not WTF internals, ENABLE(DEVELOPER_MODE) is unavailable to it, so it just checks ENABLE_DEVELOPER_MODE directly.
Philip Chimento
Comment 4 2015-05-10 15:02:33 PDT
Created attachment 252827 [details] Patch
Philip Chimento
Comment 5 2015-05-10 15:04:17 PDT
(In reply to comment #3) > Thanks for your patches. I'd like to change the code to be more robust to > this, checking values rather than definitions. But I don't see value in > fighting that fight in npapi.h, so this is probably the best approach. Let's > see what Martin thinks. There should for sure be a warning comment, though, > or someone will revert this patch in passing as a cleanup. Sure thing. I did kind of assume that npapi.h was off limits. I've added a warning comment.
Martin Robinson
Comment 6 2015-05-10 18:01:14 PDT
Comment on attachment 252827 [details] Patch Thank you for fixing this.
Philip Chimento
Comment 7 2015-05-10 19:04:57 PDT
You're welcome. Thanks for the reviews.
WebKit Commit Bot
Comment 8 2015-05-10 20:04:33 PDT
Comment on attachment 252827 [details] Patch Clearing flags on attachment: 252827 Committed r184062: <http://trac.webkit.org/changeset/184062>
WebKit Commit Bot
Comment 9 2015-05-10 20:04:37 PDT
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.