[webkit-reviews] review denied: [Bug 87659] [Wk2][EFL] EFL needs a WebKitTestRunner : [Attachment 149808] Patch

[bugzilla-daemon at webkit.org]

Raphael Kubo da Costa (rakuco) <rakuco at webkit.org> has denied Ryuan Choi
<ryuan.choi at samsung.com>'s request for review:
Bug 87659: [Wk2][EFL] EFL needs a WebKitTestRunner
https://bugs.webkit.org/show_bug.cgi?id=87659

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

------- Additional Comments from Raphael Kubo da Costa (rakuco)
<rakuco at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=149808&action=review


> Tools/CMakeLists.txt:9
> +	   ADD_SUBDIRECTORY(WebKitTestRunner)

WTR is something generic, right? It makes sense to add this call in a
platform-independent section (ie. outside the outer IF clause).

> Tools/WebKitTestRunner/CMakeLists.txt:61
> +FILE(GLOB WebKitTestRunnerInjectedBundle_IDL_FILES
"${WEBKIT_TESTRUNNER_INJECTEDBUNDLE_DIR}/Bindings/*.idl")
> +FOREACH (_file ${WebKitTestRunnerInjectedBundle_IDL_FILES})
> +    GET_FILENAME_COMPONENT (_name ${_file} NAME_WE)
> +    ADD_CUSTOM_COMMAND(
> +	   OUTPUT ${DERIVED_SOURCES_DIR}/InjectedBundle/JS${_name}.cpp
> +	   MAIN_DEPENDENCY ${_file}
> +	   DEPENDS ${WEBCORE_DIR}/bindings/scripts/generate-bindings.pl
${SCRIPTS_BINDINGS}
> +	   COMMAND ${PERL_EXECUTABLE} -I${WEBCORE_DIR}/bindings/scripts
-I${WEBKIT_TESTRUNNER_INJECTEDBUNDLE_DIR}/Bindings
${WEBCORE_DIR}/bindings/scripts/generate-bindings.pl --define \"\" --generator
TestRunner --include ${WEBKIT_TESTRUNNER_INJECTEDBUNDLE_DIR}/Bindings
--outputdir ${DERIVED_SOURCES_DIR}/InjectedBundle ${_file}
> +	   VERBATIM)
> +    LIST(APPEND WebKitTestRunnerInjectedBundle_SOURCES
${DERIVED_SOURCES_DIR}/InjectedBundle/JS${_name}.cpp)
> +ENDFOREACH ()

Don't we have some code for calling the bindings generator in Source/cmake? I'd
rather have everything in a single place.

> Tools/WebKitTestRunner/CMakeLists.txt:65
> +INCLUDE_IF_EXISTS(${WEBKIT_TESTRUNNER_DIR}/Platform${PORT}.cmake)
> +
> +INCLUDE_DIRECTORIES(${WebKitTestRunner_INCLUDE_DIRECTORIES})

It makes sense to have the include directories added before including
platform-specific files.

> Tools/WebKitTestRunner/CMakeLists.txt:75
> +SET_TARGET_PROPERTIES(WebKitTestRunner PROPERTIES COMPILE_FLAGS
> +    "-include ${WEBKIT_TESTRUNNER_DIR}/WebKitTestRunnerPrefix.h"
> +)

Why not a normal include? -include is a compiler-dependent option in a generic
build system file.

> Tools/WebKitTestRunner/CMakeLists.txt:82
> +ADD_DEPENDENCIES(WebKitTestRunner forwarding-headersEflForWebKitTestRunner)
> +ADD_DEPENDENCIES(WebKitTestRunner forwarding-headersSoupForWebKitTestRunner)


This is platform-specific.

> Tools/WebKitTestRunner/efl/PlatformWebViewEfl.cpp:27
> +static int useX11Window = false;

Why int and not bool?

> Tools/WebKitTestRunner/efl/PlatformWebViewEfl.cpp:45
> +    m_view = (Evas_Object*)(WKViewCreate(evas, context, pageGroup));

C-style cast. If you make the typedefs similar to the GTK+ ones in
PlatformWebView.h it shouldn't even be needed.

> Tools/WebKitTestRunner/efl/PlatformWebViewEfl.cpp:76
> +    WKRect frame;
> +    frame.origin.x = 0;
> +    frame.origin.y = 0;
> +    frame.size.width = 0;
> +    frame.size.height = 0;
> +    return frame;

return WKRectMake(...)?

> Tools/WebKitTestRunner/efl/TestControllerEfl.cpp:60
> +    const char* value = getenv(variableName);

I'd rather include stdlib.h directly for getenv(3) than rely on indirect
includes.

> Tools/WebKitTestRunner/efl/TestControllerEfl.cpp:62
> +	   fprintf(stderr, "%s environment variable not found\n",
variableName);

Ditto for stdio.h and fprintf(3).