Bug 87659 - [Wk2][EFL] EFL needs a WebKitTestRunner
Summary: [Wk2][EFL] EFL needs a WebKitTestRunner
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryuan Choi
URL:
Keywords:
Depends on: 88753 88935 90258
Blocks: 61838
  Show dependency treegraph
 
Reported: 2012-05-28 06:54 PDT by Dominik Röttsches (drott)
Modified: 2012-07-05 01:49 PDT (History)
16 users (show)

See Also:


Attachments
WIP:AllInOne for WTR/Efl (34.31 KB, patch)
2012-06-10 18:51 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
WIP:AllInOne (Try to fix build break) (34.31 KB, patch)
2012-06-10 22:12 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Work in Progress #2 (25.66 KB, patch)
2012-06-24 17:14 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Work in Progress #2 (25.66 KB, patch)
2012-06-24 22:37 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (25.93 KB, patch)
2012-06-27 15:53 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (26.46 KB, patch)
2012-06-27 19:14 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
work in progress(for green bot) (27.56 KB, patch)
2012-07-04 07:06 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (28.20 KB, patch)
2012-07-04 20:28 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dominik Röttsches (drott) 2012-05-28 06:54:42 PDT
For regression testing WebKit2 on EFL, we need a WebKitTestRunner implemetation.
Comment 1 Ryuan Choi 2012-06-10 18:51:08 PDT
Created attachment 146763 [details]
WIP:AllInOne for WTR/Efl
Comment 2 Gyuyoung Kim 2012-06-10 18:58:11 PDT
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
Comment 3 Ryuan Choi 2012-06-10 22:12:33 PDT
Created attachment 146782 [details]
WIP:AllInOne (Try to fix build break)
Comment 4 Ryuan Choi 2012-06-24 17:14:15 PDT
Created attachment 149211 [details]
Work in Progress #2
Comment 5 Gyuyoung Kim 2012-06-24 17:44:28 PDT
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
Comment 6 Ryuan Choi 2012-06-24 22:37:42 PDT
Created attachment 149241 [details]
Work in Progress #2
Comment 7 Chris Dumez 2012-06-25 04:05:36 PDT
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 8 Chris Dumez 2012-06-26 03:12:53 PDT
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.
Comment 9 Chris Dumez 2012-06-26 03:55:01 PDT
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).
Comment 10 Ryuan Choi 2012-06-27 15:53:37 PDT
Created attachment 149808 [details]
Patch
Comment 11 Ryuan Choi 2012-06-27 16:00:45 PDT
(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 12 Raphael Kubo da Costa (:rakuco) 2012-06-27 16:52:29 PDT
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).
Comment 13 Ryuan Choi 2012-06-27 19:14:49 PDT
Created attachment 149853 [details]
Patch
Comment 14 Ryuan Choi 2012-06-27 19:28:15 PDT
(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.
Comment 15 Raphael Kubo da Costa (:rakuco) 2012-06-27 19:55:53 PDT
(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 16 Gyuyoung Kim 2012-06-27 19:57:07 PDT
Comment on attachment 149853 [details]
Patch

Attachment 149853 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13101918
Comment 17 Dominik Röttsches (drott) 2012-07-03 06:58:07 PDT
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.
Comment 18 Ryuan Choi 2012-07-04 07:06:52 PDT
Created attachment 150787 [details]
work in progress(for green bot)
Comment 19 Chris Dumez 2012-07-04 08:35:18 PDT
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.
Comment 20 Ryuan Choi 2012-07-04 20:28:15 PDT
Created attachment 150866 [details]
Patch
Comment 21 Chris Dumez 2012-07-04 23:46:43 PDT
Thanks for getting this ready Ryuan! I hope we can land this soon and start working on improving our pass rate.
Comment 22 Kenneth Rohde Christiansen 2012-07-05 01:30:40 PDT
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
Comment 23 Ryuan Choi 2012-07-05 01:48:07 PDT
Committed r121893: <http://trac.webkit.org/changeset/121893>