There are too many features missing from cmakeconfig.h.cmake: ENABLE_ACCESSIBILITY ENABLE_LETTERPRESS ENABLE_MAC_LONG_PRESS ENABLE_PLUGIN_PROCESS_GTK2 ENABLE_SPEECH_SYNTHESIS ENABLE_THREADED_COMPOSITOR USE_REDIRECTED_XCOMPOSITE_WINDOW We should autogenerate this file so that we don't have to add new options in multiple places. It's too easy to forget. There are also a few things listed in that file that are not proper WebKit features (but which probably should be), which will be lost if we autogenerate this file: ENABLE_CREDENTIAL_STORAGE ENABLE_EVENT_MODE_METATAGS ENABLE_FIXED_REPORTED_SIZE ENABLE_FTL_NATIVE_CALL_INLINING ENABLE_INDEXED_DATABASE_IN_WORKERS ENABLE_SMOOTH_SCROLLING ENABLE_VIEWPORT_REFLOW WTF_USE_GEOCLUE2 HAVE_LLVM HAVE_GTK_UNIX_PRINTING WTF_CPU_ARM64_CORTEXA53 We'll need to define those with WEBKIT_OPTION_DEFINE to keep them. But we should do that regardless, as otherwise they're not presented as options to the user in the list of features that prints after running cmake.
(In reply to comment #0) > ENABLE_EVENT_MODE_METATAGS > ENABLE_FIXED_REPORTED_SIZE > ENABLE_VIEWPORT_REFLOW These don't exist anymore; let's remove them. > ENABLE_FTL_NATIVE_CALL_INLINING > ENABLE_INDEXED_DATABASE_IN_WORKERS > ENABLE_SMOOTH_SCROLLING These should become options as-is. > WTF_USE_GEOCLUE2 > WTF_CPU_ARM64_CORTEXA53 > HAVE_LLVM > HAVE_GTK_UNIX_PRINTING These should be renamed to USE_* when they become options.
Created attachment 251243 [details] [CMake] Autogenerate cmakeconfig.h.cmake
I have a version of this as well that I was playing with. Perhaps we can combine our work?
I figure what I have is probably the simplest way (I'd just like to move the GDK_VERSION_MIN_REQUIRED out of the cross-platform file), but go ahead and post yours. :)
Created attachment 251352 [details] Patch
OK, I like this approach a bit better than mine, since it allows exposing arbitrary CMake variables. And you took care of all the variables that would need exposed to the build. That's really awesome, because I was planning to do that, and now I don't have to. I think I would prefer to change the code to drop the WTF prefix rather than vice-versa, but this way is fine too. We can always change it later: it's certainly less-intrusive to do it this way. #if PLATFORM(GTK) && !defined(GTK_API_VERSION_2) #define GDK_VERSION_MIN_REQUIRED GDK_VERSION_3_6 #endif ^ That needs to be dependent on whether we use ENABLE_X11_TARGET or ENABLE_WAYLAND_TARGET: the dependency is GDK 3.6 for X11 and 3.12 for Wayland. ENABLE_3D_RENDERING and ENABLE_TEXTURE_MAPPER really needs to be set prior to calling WEBKIT_OPTION_END, but it's kind of a mess to fix that (you'd have to move a bunch of code) and it's no worse than what we have now, so we can change that in a later patch.
(In reply to comment #6) > ^ That needs to be dependent on whether we use ENABLE_X11_TARGET or > ENABLE_WAYLAND_TARGET: the dependency is GDK 3.6 for X11 and 3.12 for > Wayland. No it doesn't; I just misread the existing code. And: mrobinson: mcatanzaro: I think GDK_VERSION_MIN_REQUIRED controls what deprecation messages we see.
mcatanzaro: So your patch looks good to me, no further comments. I think some of those WTF_USE_WHATEVERs we should remove and replace with WEBKIT_OPTION_DEFINE(ENABLE_WHATEVER), in particular the ENABLE_X11_TARGET and ENABLE_WAYLAND_TARGET, but we can do that in a separate future patch.
Perhaps someone could review this patch which greatly simplifies the CMake build?
Found a couple issues: * We lost ENABLE_INDEXED_DATABASE_IN_WORKERS with this patch. It's overridden in OptionsEfl.cmake, OptionsGTK.cmake, and OptionsMac.cmake, but it's never defined with WEBKIT_OPTION_DEFINE. So we should add that to WebKitFeatures.cmake in this patch. (See also bug #144069.) * We should export HAVE_LLVM in OptionsEFL.cmake as well. There's more cleanups we could do there, but as far as I see nothing that would cause breakage for EFL besides that. (I guess EWS does not use FTL, so it didn't catch this.)
Created attachment 251365 [details] Patch
(In reply to comment #10) > Found a couple issues: > > * We lost ENABLE_INDEXED_DATABASE_IN_WORKERS with this patch. It's > overridden in OptionsEfl.cmake, OptionsGTK.cmake, and OptionsMac.cmake, but > it's never defined with WEBKIT_OPTION_DEFINE. So we should add that to > WebKitFeatures.cmake in this patch. (See also bug #144069.) Okay. I've added a definition for ENABLE_INDEXED_DATABASE_IN_WORKERS.
Created attachment 251366 [details] Patch
(In reply to comment #12) > (In reply to comment #10) > > Found a couple issues: > > > > * We lost ENABLE_INDEXED_DATABASE_IN_WORKERS with this patch. It's > > overridden in OptionsEfl.cmake, OptionsGTK.cmake, and OptionsMac.cmake, but > > it's never defined with WEBKIT_OPTION_DEFINE. So we should add that to > > WebKitFeatures.cmake in this patch. (See also bug #144069.) > > Okay. I've added a definition for ENABLE_INDEXED_DATABASE_IN_WORKERS. Just to clarify, I think the current patch addresses both of Michael's original concerns.
Nit: ENABLE_SMOOTH_SCROLLING isn't added in the right (alphabetical) place in OptionsGTK.cmake
Hm, one more problem: is it really safe to use SET_AND_EXPOSE_TO_BUILD in a conditional? I'm thinking no, since we usually check values with #if and not #ifdef, we should always define them to 0 if not 1, and therefore only use SET_AND_EXPOSE_TO_BUILD outside of conditionals to ensure the variable is always set to something. (Of course, if a value is really checked with #ifdef, then that would be wrong....)
mrobinson: mcatanzaro: Values are check with macros like #if ENABLE(blah) and #if USE(blah) which handle the undefined case.
Comment on attachment 251366 [details] Patch Nice cleanup, r=me.
(In reply to comment #18) > Comment on attachment 251366 [details] > Patch > > Nice cleanup, r=me. But it should be updated before landing due to some recent changes.
Committed r183416: <http://trac.webkit.org/changeset/183416>
Comment on attachment 251366 [details] Patch Clearing review flag, because this patch has landed.
Comment on attachment 251366 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=251366&action=review > Source/WTF/wtf/Platform.h:493 > +#if PLATFORM(GTK) && !defined(GTK_API_VERSION_2) > +#define GDK_VERSION_MIN_REQUIRED GDK_VERSION_3_6 > +#endif Why was this moved here? This doesn't affect other tools like MiniBrowser, that include cmakeconfig.h. This brought back a lot of GTK+ deprecation warnings when building minibrowser
(In reply to comment #22) > Comment on attachment 251366 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=251366&action=review > > > Source/WTF/wtf/Platform.h:493 > > +#if PLATFORM(GTK) && !defined(GTK_API_VERSION_2) > > +#define GDK_VERSION_MIN_REQUIRED GDK_VERSION_3_6 > > +#endif > > Why was this moved here? This doesn't affect other tools like MiniBrowser, > that include cmakeconfig.h. This brought back a lot of GTK+ deprecation > warnings when building minibrowser Martin and I were wondering where those were coming from! Good catch. It was moved from cmakeconfig.h.cmake because we deleted that file. It's tricky to set in OptionsGTK.cmake since it has to not be set when compiling the GTK2 plugin process. Probably the best solution is to add it to Tools/MiniBrowser/gtk/CMakeLists.txt.
(In reply to comment #23) > Martin and I were wondering where those were coming from! Good catch. It was > moved from cmakeconfig.h.cmake because we deleted that file. It's tricky to > set in OptionsGTK.cmake since it has to not be set when compiling the GTK2 > plugin process. Probably the best solution is to add it to > Tools/MiniBrowser/gtk/CMakeLists.txt. Michael is absolutely correct. It was moved here because the place it was moved from no longer exists.