[webkit-reviews] review granted: [Bug 187127] [JSCOnly] Restore Windows build. : [Attachment 343822] Patch

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at igalia.com> has granted Ross Kirsling
<ross.kirsling at sony.com>'s request for review:
Bug 187127: [JSCOnly] Restore Windows build.
https://bugs.webkit.org/show_bug.cgi?id=187127

Attachment 343822: Patch

https://bugs.webkit.org/attachment.cgi?id=343822&action=review




--- Comment #9 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 343822
  --> https://bugs.webkit.org/attachment.cgi?id=343822
Patch

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

> Source/cmake/OptionsJSCOnly.cmake:-21
> -    # FIXME: Enable FTL on Windows.
https://bugs.webkit.org/show_bug.cgi?id=145366
> -    WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_FTL_JIT PRIVATE OFF)

You don't expect anyone to build JSCOnly using CMake directly? I guess the
build-jsc script suffices... but for GTK/WPE, build-webkit and build-jsc are
just wrappers above the real (CMake) build system, which we expect to work
independently of it, so I wouldn't consider it misleading to disable an
unsupported option here.

> Source/cmake/OptionsJSCOnly.cmake:76
> -	   set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/bin)
> +	   set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/bin64)

Windows is interesting.

> Tools/Scripts/webkitdirs.pm:1299
>      return if defined($isWin64);
> -    $isWin64 = checkForArgumentAndRemoveFromARGV("--64-bit") || isWinCairo()
&& !shouldBuild32Bit();
> +    $isWin64 = checkForArgumentAndRemoveFromARGV("--64-bit") ||
((isWinCairo() || isJSCOnly) && !shouldBuild32Bit());

This will probably break for Apple Windows since you're missing the parentheses
after isJSCOnly()