[webkit-reviews] review denied: [Bug 72816] Add CMake base build system for Apple Windows port : [Attachment 116431] WIP: Basic CMake based build system for Windows

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has denied Patrick R. Gansterer
<paroga at paroga.com>'s request for review:
Bug 72816: Add CMake base build system for Apple Windows port
https://bugs.webkit.org/show_bug.cgi?id=72816

Attachment 116431: WIP: Basic CMake based build system for Windows
https://bugs.webkit.org/attachment.cgi?id=116431&action=review

------- Additional Comments from Daniel Bates <dbates at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=116431&action=review


> Source/JavaScriptCore/PlatformWinApple.cmake:18
> +
> +
> +
> +

Nit: One empty line line seems sufficient to visually separate these blocks of
code. Having four seems extraneous.

> Source/WebCore/PlatformWinApple.cmake:247
> +
> +

Nit: One empty line line seems sufficient to visually separate these blocks of
code. Having two seems extraneous.

> Source/WebCore/PlatformWinApple.cmake:261
> +
> +
> +

Nit: One empty line line seems sufficient to visually separate these blocks of
code. Having three seems extraneous.

> Source/WebCore/PlatformWinApple.cmake:324
> +
> +

Nit: One empty line line seems sufficient to visually separate these blocks of
code. Having two seems extraneous.

> Source/WebCore/qtmoviewin/CMakeLists.txt:35
> +
> +

Nit: One empty line line seems sufficient to visually separate these blocks of
code. Having two seems extraneous.

> Source/WebKit/win/CMakeListsWinApple.txt:111
> +
> +

Nit: One empty line line seems sufficient to visually separate these blocks of
code. Having two seems extraneous.

> Source/WebKit/win/CMakeListsWinApple.txt:120
> +
> +

Nit: One empty line line seems sufficient to visually separate these blocks of
code. Having two seems extraneous.

> Source/cmake/FindDirectX.cmake:7
> +#
-----------------------------------------------------------------------------
> +# Find DirectX SDK
> +# Define:
> +# DirectX_FOUND
> +# DirectX_INCLUDE_DIR
> +# DirectX_LIBRARY
> +# DirectX_ROOT_DIR

The drastic change in style used in this file compared to the rest of this
patch leads me to believe that this file was acquired. We need copyright and
license information about this file to both give credit to the author and to
determine whether this file can be landed given its license.

>From talking with Patrick on IRC today (12/1), he mentioned that he found this
file using Google.

> Source/cmake/FindDirectX.cmake:51
> +  #message("DirectX_FOUND=${DirectX_FOUND}")
> +  #message("DirectX_INCLUDE_DIR=${DirectX_INCLUDE_DIR}")
> +  #message("DirectX_LIBRARY=${DirectX_LIBRARY}")
> +  #message("DirectX_ROOT_DIR=${DirectX_ROOT_DIR}")

Nit: We shouldn't commit commented out directives.

> Source/cmake/FindQuickTimeSDK.cmake:1
> +set(QuickTimeSDK_FOUND 0)

The drastic change in style used in this file compared to the rest of this
patch leads me to believe that this file was acquired. We need copyright and
license information about this file to both give credit to the author and to
determine whether this file can be landed given its license.