| Summary: | [Wk2][EFL] EFL needs a WebKitTestRunner | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Dominik Röttsches (drott) <d-r> | ||||||||||||||||||
| Component: | WebKit EFL | Assignee: | Ryuan Choi <ryuan.choi> | ||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||
| Severity: | Normal | CC: | abarth, cdumez, dpranke, gyuyoung.kim, hw1008.kim, joone, jussi.kukkonen, kenneth, lucas.de.marchi, naginenis, ojan, rakuco, ryuan.choi, tmpsantos, tonikitoo, webkit.review.bot | ||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||
| Bug Depends on: | 88753, 88935, 90258 | ||||||||||||||||||||
| Bug Blocks: | 61838 | ||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||
|
Description
Dominik Röttsches (drott)
2012-05-28 06:54:42 PDT
Created attachment 146763 [details]
WIP:AllInOne for WTR/Efl
Comment on attachment 146763 [details] WIP:AllInOne for WTR/Efl Attachment 146763 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12936495 Created attachment 146782 [details]
WIP:AllInOne (Try to fix build break)
Created attachment 149211 [details]
Work in Progress #2
Comment on attachment 149211 [details] Work in Progress #2 Attachment 149211 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13032988 Created attachment 149241 [details]
Work in Progress #2
Comment on attachment 149241 [details] Work in Progress #2 View in context: https://bugs.webkit.org/attachment.cgi?id=149241&action=review > Tools/WebKitTestRunner/efl/main.cpp:40 > + return 1; You should probably call ecore_evas_shutdown(); before returning here. > Tools/WebKitTestRunner/efl/main.cpp:44 > + return 1; You should probably call ecore_evas_shutdown(); and edje_shutdown(); before returning. Comment on attachment 149241 [details] Work in Progress #2 View in context: https://bugs.webkit.org/attachment.cgi?id=149241&action=review > Tools/WebKitTestRunner/efl/TestControllerEfl.cpp:77 > + m_injectedBundlePath = WKStringCreateWithUTF8CString(pluginPath); m_testPluginDirectory, not injectedBundlePath. Otherwise we crash. I gave the patch a try, and besides the crash I mentioned in my review, it fails to load the injected bundle: CRI<8485>: /home/chris/Devel/WebKit/Source/WebKit2/WebProcess/InjectedBundle/efl/InjectedBundleEfl.cpp:48 load() Error loading the injected bundle: /home/chris/Devel/WebKit/WebKitBuild/Debug/lib/libTestRunnerInjectedBundle.so I'm guessing this is because we don't provide an implementation in EFL port for InjectedBundle::platformInitialize(WKTypeRef). Created attachment 149808 [details]
Patch
(In reply to comment #7) > (From update of attachment 149241 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=149241&action=review > > > Tools/WebKitTestRunner/efl/main.cpp:40 > > + return 1; > > You should probably call ecore_evas_shutdown(); before returning here. > > > Tools/WebKitTestRunner/efl/main.cpp:44 > > + return 1; > > You should probably call ecore_evas_shutdown(); and edje_shutdown(); before returning. Thank you. I fixed. > > Tools/WebKitTestRunner/efl/TestControllerEfl.cpp:77 > > + m_injectedBundlePath = WKStringCreateWithUTF8CString(pluginPath); > > m_testPluginDirectory, not injectedBundlePath. Otherwise we crash. It's mistake. fixed. > I gave the patch a try, and besides the crash I mentioned in my review, it fails to load the injected bundle: > CRI<8485>: /home/chris/Devel/WebKit/Source/WebKit2/WebProcess/InjectedBundle/efl/InjectedBundleEfl.cpp:48 load() Error loading the injected bundle: /home/chris/Devel/WebKit/WebKitBuild/Debug/lib/libTestRunnerInjectedBundle.so > > I'm guessing this is because we don't provide an implementation in EFL port for InjectedBundle::platformInitialize(WKTypeRef). Yes, I fixed by including WebCoreTestSupport. It's because of missing WebCoreTestSupport when building with SHARED_CORE. Comment on attachment 149808 [details] Patch 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). Created attachment 149853 [details]
Patch
(In reply to comment #12) > (From update of attachment 149808 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=149808&action=review Thank you for your quick 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). > I moved. > > 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. > I agree with you. But how do you think about separating this issue from this bug? > > 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. > I am not sure. We should also include platform specific directories. > > 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. > I removed it because WebKitTestRunnerPrefix.h is for the ports which support prefix headers. Instead, I enhance config.h to include WebKit2.h. > > Tools/WebKitTestRunner/CMakeLists.txt:82 > > +ADD_DEPENDENCIES(WebKitTestRunner forwarding-headersEflForWebKitTestRunner) > > +ADD_DEPENDENCIES(WebKitTestRunner forwarding-headersSoupForWebKitTestRunner) > > This is platform-specific. Fixed by using general terms. > > > Tools/WebKitTestRunner/efl/PlatformWebViewEfl.cpp:27 > > +static int useX11Window = false; > > Why int and not bool? > Sorry, fixed. > > 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. > Because Gtk have some macro such as GTK_WIDGET, GTK_WINDOW, it can be solved more easily. But we don't have proper macros to solve. Instead, I fixed by using APICasts.h which defines toImpl and toAPI. > > 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(...)? Thank you. done. > > > 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). Done. (In reply to comment #14) > (In reply to comment #12) > > > 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. > > > I agree with you. > But how do you think about separating this issue from this bug? OK, but I'd rather get it fixed before committing this one. > > > 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. > > > I am not sure. We should also include platform specific directories. True, I had forgotten about that. > > > Tools/WebKitTestRunner/CMakeLists.txt:82 > > > +ADD_DEPENDENCIES(WebKitTestRunner forwarding-headersEflForWebKitTestRunner) > > > +ADD_DEPENDENCIES(WebKitTestRunner forwarding-headersSoupForWebKitTestRunner) > > > > This is platform-specific. > Fixed by using general terms. Doesn't it make more sense to simply move these calls to PlatformEfl.cmake? Comment on attachment 149853 [details] Patch Attachment 149853 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13101918 Ryuan, would you have time to fix the build issue that the EWS reported? /usr/bin/ld: ../../lib/libWebCoreTestSupport.a(WebCoreTestSupport.cpp.o): relocation R_X86_64_32 against `__gxx_personality_v0' can not be used when making a shared object; recompile with -fPIC ../../lib/libWebCoreTestSupport.a: could not read symbols: Bad value collect2: ld returned 1 exit status If you don't, are you okay if we take this patch and move enabling WebKitTestRunner forward? Thanks. Created attachment 150787 [details]
work in progress(for green bot)
Are Bug 88753 and Bug 88756 really dependencies? It seems like the WebKitTestRunner is running without them. If they can be worked on afterwards, it would be good the remove those 2 dependencies and land the WKTR patch ASAP. Created attachment 150866 [details]
Patch
Thanks for getting this ready Ryuan! I hope we can land this soon and start working on improving our pass rate. Comment on attachment 150866 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150866&action=review > Tools/WebKitTestRunner/efl/TestControllerEfl.cpp:67 > + } > + return value; pls add new line before returns etc Committed r121893: <http://trac.webkit.org/changeset/121893> |