Bug 237170

This change seems working as I expected, but I'm not sure this is the correct direction. diff --git a/Source/cmake/WebKitCommon.cmake b/Source/cmake/WebKitCommon.cmake index e58c7069abb1..c3389c8e46f7 100644 --- a/Source/cmake/WebKitCommon.cmake +++ b/Source/cmake/WebKitCommon.cmake @@ -13,19 +13,19 @@ if (NOT HAS_RUN_WEBKIT_COMMON) message(STATUS "The CMake build type is: ${CMAKE_BUILD_TYPE}") endif () - set(ENABLE_JAVASCRIPTCORE ON) - set(ENABLE_WEBCORE ON) + set(ENABLE_JAVASCRIPTCORE ON CACHE STRING "Enable JavaScriptCore build") + set(ENABLE_WEBCORE ON ON CACHE STRING "Enable WebCore build") if (NOT DEFINED ENABLE_WEBKIT) - set(ENABLE_WEBKIT ON) + set(ENABLE_WEBKIT ON ON CACHE STRING "Enable WebKit build") endif () if (NOT DEFINED ENABLE_TOOLS AND EXISTS "${CMAKE_SOURCE_DIR}/Tools") - set(ENABLE_TOOLS ON) + set(ENABLE_TOOLS ON ON CACHE STRING "Enable Tools build") endif () if (NOT DEFINED ENABLE_WEBINSPECTORUI) - set(ENABLE_WEBINSPECTORUI ON) + set(ENABLE_WEBINSPECTORUI ON ON CACHE STRING "Enable WebInspector UI build") endif () # -----------------------------------------------------------------------------

If you want to change ENABLE_WEBCORE from outside, using option is the CMake way in such case. https://cmake.org/cmake/help/latest/command/option.html

It seems like it'd work better if those were cache variables rather than normal ones if we're expecting them to have overwritten values from the command line that'll persist. The others are using an if(NOT DEFINED XXX) block, which may also work if put around the set in WebKitCommon, but seems like it's just doing the same sort of thing that a cache variable would already do. Is there some piece of the cache variable behavior that's problematic for what we're doing (for example maybe around the HAS_RUN_WEBKIT_COMMON bits?)

Created attachment 453425 [details] PATCH

Comment on attachment 453425 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=453425&action=review > Source/cmake/WebKitCommon.cmake:18 > + option(ENABLE_WEBKIT "Enable building WebKit" ON) Some ports set these values OFF by default. It'd be nice if we can change the default value of these options. I think there two ways to do so. 1. Using option twice, in WebKitCommon.cmake and Options${PORT}.cmake. 2. Adding new variables, for example, ENABLE_WEBCORE_DEFAULT. But, it seemingly needs some refactoring. I don't object landing this patch if you want.

(In reply to Fujii Hironori from comment #5) > Comment on attachment 453425 [details] > PATCH > > View in context: > https://bugs.webkit.org/attachment.cgi?id=453425&action=review > > > Source/cmake/WebKitCommon.cmake:18 > > + option(ENABLE_WEBKIT "Enable building WebKit" ON) > > Some ports set these values OFF by default. > It'd be nice if we can change the default value of these options. > I think there two ways to do so. > 1. Using option twice, in WebKitCommon.cmake and Options${PORT}.cmake. > 2. Adding new variables, for example, ENABLE_WEBCORE_DEFAULT. > But, it seemingly needs some refactoring. I don't object landing this patch > if you want. Correct. While touching the code, I was fighting with the attempt of refactoring more and more. Then ended up to just doing the straight-forward thing right now. As you say, I'll land this right now and do more refactoring later.

Created attachment 453478 [details] PATCH

Committed r290636 (247909@main): <https://commits.webkit.org/247909@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 453478 [details].