[webkit-reviews] review denied: [Bug 87659] [Wk2][EFL] EFL needs a WebKitTestRunner : [Attachment 149808] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Jun 27 16:52:30 PDT 2012
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).
More information about the webkit-reviews
mailing list