[webkit-reviews] review denied: [Bug 102647] [CMake] Add an option to build AllInOne files : [Attachment 217046] patch

[bugzilla-daemon at webkit.org]

Patrick R. Gansterer <paroga at paroga.com> has denied Gergő Balogh
<geryxyz at inf.u-szeged.hu>'s request for review:
Bug 102647: [CMake] Add an option to build AllInOne files
https://bugs.webkit.org/show_bug.cgi?id=102647

Attachment 217046: patch
https://bugs.webkit.org/attachment.cgi?id=217046&action=review

------- Additional Comments from Patrick R. Gansterer <paroga at paroga.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=217046&action=review


i'd suggest a WebCore_ALLINONE_SOURCES variable and handle it in the
WEBKIT_WRAP_SOURCES macro.
do the many status messages provide a real benefit? they look a kind of
printf-debugging to me (just for implementing the allinone stuff for cmake)

> Source/WebCore/CMakeLists.txt:2558
> +set(AllInOne "" CACHE STRING "If true, enables all-in-one files.")

if its a global switch it should be move to the root cmake file

> Source/WebCore/CMakeLists.txt:2559
> +if (AllInOne)

usually global variables are all uppercase

> Source/WebCore/CMakeLists.txt:2592
> +    #list(GET allInOnes 0 allInOne)

remove comment?

> Source/WebCore/CMakeLists.txt:2601
> +    endforeach (allInOne)

please remove the characters from the ()

> Source/WebCore/CMakeLists.txt:2602
> +endif (AllInOne)

too

> Source/cmake/AllInOneSupport.cmake:1
> +cmake_minimum_required(VERSION 2.8)

imho not a good idea to spred this across the whole code