Bug 94925 – [EFL][UT]Refactoring an implementation of testing framework for wk1.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Krzysztof Czech]

Refactoring an implementation of testing framework for wk1

[Krzysztof Czech]

Created attachment 160401 [details]
Updating ut framework for wk1

[WebKit Review Bot]

Attachment 160401 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/efl/ChangeLog', u'Source/Web..." exit_code: 1
Source/WebKit/efl/tests/UnitTestUtils/EWKTestView.h:35:  The parameter name "viewType" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Krzysztof Czech]

Created attachment 160433 [details]
Updating ut framework for wk1

[Ryuan Choi]

Comment on attachment 160433 [details]
Updating ut framework for wk1

View in context: https://bugs.webkit.org/attachment.cgi?id=160433&action=review

> Source/WebKit/efl/ChangeLog:4
> +        [EFL][UT]Refactoring an implementation of testing framework for wk1.
> +        https://bugs.webkit.org/show_bug.cgi?id=94925

Bug title and change log are different.

> Source/WebKit/efl/ChangeLog:10
> +        and lastly to make framework simplier and easier to use.

I am not good english speaker, so I am not sure whether `simplier` is correct.

> Source/WebKit/efl/tests/UnitTestUtils/EWKTestBase.cpp:78
> +    UNUSED_PARAM(webView);
> +    UNUSED_PARAM(eventInfo);

I think that we can just remove parameter name because this is cpp.

> Source/WebKit/efl/tests/UnitTestUtils/EWKTestBase.cpp:84
> -bool EWKTestBase::runTest(const char* url, void (*event_callback)(void*, Evas_Object*, void*), const char* event_name, void* event_data)
> +void EWKTestBase::runTest(const char* url)

In my two cents, runTest looks ambiguous.
WebKit2/Efl uses loadUrlSync (although I want to suggest waitUntilLoadFinished with ewk_view_uri_set like waitUntilTitleChanged approach)

> Source/WebKit/efl/tests/UnitTestUtils/EWKTestView.cpp:80
> -Evas* EWKTestView::evas()
> +bool EWKTestView::loadUrl(const char* url)

I think that this wrapper is not necessary.

> Source/WebKit/efl/tests/UnitTestUtils/EWKTestView.cpp:90
> +Evas_Object* EWKTestView::mainFrame()
>  {
> -    if (!m_webView.get())
> -        return;
> -
> -    evas_object_smart_callback_del(m_webView.get(), eventName, callback);
> -    evas_object_smart_callback_add(m_webView.get(), eventName, callback, ptr);
> +    return ewk_view_frame_main_get(m_webView);
>  }

Ditto.

> Source/WebKit/efl/tests/UnitTestUtils/EWKTestView.h:-66
> -    OwnPtr<Evas_Object> m_webView;

Can I know why you change OwnPtr<Evas_Object> to Evas_Object* ?

> Source/WebKit/efl/tests/test_ewk_view.cpp:35
> +    EXPECT_EQ(EINA_FALSE, ewk_view_editable_get(webView()));

ASSERT_FALSE?

[Gyuyoung Kim]

Comment on attachment 160433 [details]
Updating ut framework for wk1

View in context: https://bugs.webkit.org/attachment.cgi?id=160433&action=review

> Source/WebKit/efl/tests/UnitTestUtils/EWKTestView.h:46
>      EWKTestView(const EWKTestView&);

Please add explicit.

> Source/WebKit/efl/tests/UnitTestUtils/EWKTestView.h:49
> +    int m_useX11Window;

why int ?

[Krzysztof Czech]

Comment on attachment 160433 [details]
Updating ut framework for wk1

View in context: https://bugs.webkit.org/attachment.cgi?id=160433&action=review

> Source/WebKit/efl/ChangeLog:5
> +

Will correct

> Source/WebKit/efl/ChangeLog:11
> +

You're right

> Source/WebKit/efl/tests/UnitTestUtils/EWKTestBase.cpp:79
> +

Providing extra parameters of detailed warnings like (-Wunused-parameter) to gcc/g++ the warning will occur, despite it's a cpp file.

> Source/WebKit/efl/tests/UnitTestUtils/EWKTestBase.cpp:85
>  {

I agree with you, runTest() might be a little ambiguous.
Having something like waitUntilLoadFinished will assure that page loading is completed.
Generally this method is addressed for testing like getters/setters, but yes
to void some potential problems your suggestion is good.

> Source/WebKit/efl/tests/UnitTestUtils/EWKTestView.cpp:81
>  {

Ok, ewk_view_uri_set can be used directly.

> Source/WebKit/efl/tests/UnitTestUtils/EWKTestView.cpp:91
>  

Will correct

>> Source/WebKit/efl/tests/UnitTestUtils/EWKTestView.h:46
>>      EWKTestView(const EWKTestView&);
> 
> Please add explicit.

I think adding explicit to private copy constructor is not needed.

>> Source/WebKit/efl/tests/UnitTestUtils/EWKTestView.h:-66
>> -    OwnPtr<Evas_Object> m_webView;
> 
> Can I know why you change OwnPtr<Evas_Object> to Evas_Object* ?

I wanna make sure m_webView and m_ecoreEvas are deleted before ecore_evas_shutdown.
Despite the fact test view is destructed before ecore_evas_shutdown having OwnPtr
I had some strange assertion from efl. That's way I don't use OwnPtr in this case.

> Source/WebKit/efl/tests/UnitTestUtils/EWKTestView.h:50
>      int m_width, m_height;

to have compatibility with getopt_long

[Krzysztof Czech]

Created attachment 163572 [details]
[EFL][UT]Refactoring an implementation of testing framework for wk1.

[WebKit Review Bot]

Attachment 163572 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/efl/ChangeLog', u'Source/Web..." exit_code: 1
Source/WebKit/efl/tests/UnitTestUtils/EWKTestBase.cpp:79:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Krzysztof Czech]

Created attachment 163576 [details]
[EFL][UT]Refactoring an implementation of testing framework for wk1.

[Ryuan Choi]

Comment on attachment 163576 [details]
[EFL][UT]Refactoring an implementation of testing framework for wk1.

View in context: https://bugs.webkit.org/attachment.cgi?id=163576&action=review

> Source/WebKit/efl/tests/UnitTestUtils/EWKTestBase.cpp:69
> +void EWKTestBase::onLoadFinished(void* data, Evas_Object* webView, void* eventInfo)
>  {
> -    return createTest(Config::defaultTestPage, event_callback, event_name, event_data);
> +    UNUSED_PARAM(data);
> +    UNUSED_PARAM(webView);
> +    UNUSED_PARAM(eventInfo);

Could you check whether compiler complains with void EWKTestBase::onLoadFinished(void*, Evas_Object*, void*) ?

> Source/WebKit/efl/tests/UnitTestUtils/EWKTestView.cpp:44
> +bool EWKTestView::init()

Isn't it better to pass viewtype, width, height instead of keeping them ?

and initialize looks better.

> Source/WebKit/efl/tests/UnitTestUtils/EWKTestView.cpp:76
> +    evas_object_focus_set(m_webView, EINA_TRUE);

true ?

> Source/WebKit/efl/tests/UnitTestUtils/EWKTestView.h:42
> +    void viewTypeSet(EwkViewType testViewType) { m_viewType = testViewType; }

I think that it looks meaningless.

[Gyuyoung Kim]

Comment on attachment 163576 [details]
[EFL][UT]Refactoring an implementation of testing framework for wk1.

View in context: https://bugs.webkit.org/attachment.cgi?id=163576&action=review

> Source/WebKit/efl/tests/test_ewk_view.cpp:33
> +    runTest();

Could you explain why WK1 unit test should call runTest() unlike WK2 unit test ?

[Krzysztof Czech]

(In reply to comment #10)
> (From update of attachment 163576 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163576&action=review
> 
> > Source/WebKit/efl/tests/UnitTestUtils/EWKTestBase.cpp:69
> > +void EWKTestBase::onLoadFinished(void* data, Evas_Object* webView, void* eventInfo)
> >  {
> > -    return createTest(Config::defaultTestPage, event_callback, event_name, event_data);
> > +    UNUSED_PARAM(data);
> > +    UNUSED_PARAM(webView);
> > +    UNUSED_PARAM(eventInfo);
> 
> Could you check whether compiler complains with void EWKTestBase::onLoadFinished(void*, Evas_Object*, void*) ?

I checked and this build configuration (it might be changed someday to be more restrictive) doesn't complain.
Ok, I will correct this.

> 
> > Source/WebKit/efl/tests/UnitTestUtils/EWKTestView.cpp:44
> > +bool EWKTestView::init()
> 
> Isn't it better to pass viewtype, width, height instead of keeping them ?
> 
> and initialize looks better.

You mean passing from command line ?
I proposed EWKTestConfig.h to keep constants in terms of readability.

> 
> > Source/WebKit/efl/tests/UnitTestUtils/EWKTestView.cpp:76
> > +    evas_object_focus_set(m_webView, EINA_TRUE);
> 
> true ?

You mean true as boolean value ?. Well I wanted to be coherent with function's declaration. It declares Eina_Bool.

> 
> > Source/WebKit/efl/tests/UnitTestUtils/EWKTestView.h:42
> > +    void viewTypeSet(EwkViewType testViewType) { m_viewType = testViewType; }
> 
> I think that it looks meaningless.

Let me explain, why I proposed this method.
This method gives possibility to test Tiled and Single backing store even in one test's translation unit.
By default Tiled backingstore is created. viewTypeSet gives possibility to change this to SingleView in other test case before runTest (btw. runTest is a bit ambiguous, rather should be loadTest) is calling.

[Krzysztof Czech]

(In reply to comment #11)
> (From update of attachment 163576 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163576&action=review
> 
> > Source/WebKit/efl/tests/test_ewk_view.cpp:33
> > +    runTest();
> 
> Could you explain why WK1 unit test should call runTest() unlike WK2 unit test ?

I think this method's name is a bit ambiguous. Should be rather loadUrl.
Generally, every time new test case is created, webkit should be properly initialized and test page should be loaded.

[Ryuan Choi]

Comment on attachment 163576 [details]
[EFL][UT]Refactoring an implementation of testing framework for wk1.

View in context: https://bugs.webkit.org/attachment.cgi?id=163576&action=review

>>> Source/WebKit/efl/tests/UnitTestUtils/EWKTestView.cpp:44
>>> +bool EWKTestView::init()
>> 
>> Isn't it better to pass viewtype, width, height instead of keeping them ?
>> 
>> and initialize looks better.
> 
> You mean passing from command line ?
> I proposed EWKTestConfig.h to keep constants in terms of readability.

I just mean passing them as argument of init() instead of argument of constructor.

>>> Source/WebKit/efl/tests/UnitTestUtils/EWKTestView.cpp:76
>>> +    evas_object_focus_set(m_webView, EINA_TRUE);
>> 
>> true ?
> 
> You mean true as boolean value ?. Well I wanted to be coherent with function's declaration. It declares Eina_Bool.

As following http://trac.webkit.org/wiki/EFLWebKitCodingStyle, we use true instead of EINA_TRUE in webkit internal.

>>> Source/WebKit/efl/tests/UnitTestUtils/EWKTestView.h:42
>>> +    void viewTypeSet(EwkViewType testViewType) { m_viewType = testViewType; }
>> 
>> I think that it looks meaningless.
> 
> Let me explain, why I proposed this method.
> This method gives possibility to test Tiled and Single backing store even in one test's translation unit.
> By default Tiled backingstore is created. viewTypeSet gives possibility to change this to SingleView in other test case before runTest (btw. runTest is a bit ambiguous, rather should be loadTest) is calling.

After called viewTypeSet, we should call init to really change the type of view.(and it is not used in other areas.
So, I think that we can just pass  type to init as a parameter.

> Source/WebKit/efl/tests/UnitTestUtils/EWKTestView.h:-66
> -    OwnPtr<Evas_Object> m_webView;

I will check which warning is generated.

But I believe that we can ensure destructor of members will be called in order.
We can just add comment like http://trac.webkit.org/browser/trunk/Source/WebCore/platform/efl/RenderThemeEfl.h#L251

[Krzysztof Czech]

(In reply to comment #14)
> (From update of attachment 163576 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163576&action=review
> 
> >>> Source/WebKit/efl/tests/UnitTestUtils/EWKTestView.cpp:44
> >>> +bool EWKTestView::init()
> >> 
> >> Isn't it better to pass viewtype, width, height instead of keeping them ?
> >> 
> >> and initialize looks better.
> > 
> > You mean passing from command line ?
> > I proposed EWKTestConfig.h to keep constants in terms of readability.
> 
> I just mean passing them as argument of init() instead of argument of constructor.
I agree with this idea. Corrected.
> 
> >>> Source/WebKit/efl/tests/UnitTestUtils/EWKTestView.cpp:76
> >>> +    evas_object_focus_set(m_webView, EINA_TRUE);
> >> 
> >> true ?
> > 
> > You mean true as boolean value ?. Well I wanted to be coherent with function's declaration. It declares Eina_Bool.
> 
> As following http://trac.webkit.org/wiki/EFLWebKitCodingStyle, we use true instead of EINA_TRUE in webkit internal.
Corrected.
> 
> >>> Source/WebKit/efl/tests/UnitTestUtils/EWKTestView.h:42
> >>> +    void viewTypeSet(EwkViewType testViewType) { m_viewType = testViewType; }
> >> 
> >> I think that it looks meaningless.
> > 
> > Let me explain, why I proposed this method.
> > This method gives possibility to test Tiled and Single backing store even in one test's translation unit.
> > By default Tiled backingstore is created. viewTypeSet gives possibility to change this to SingleView in other test case before runTest (btw. runTest is a bit ambiguous, rather should be loadTest) is calling.
> 
> After called viewTypeSet, we should call init to really change the type of view.(and it is not used in other areas.
> So, I think that we can just pass  type to init as a parameter.
Corrected.
> 
> > Source/WebKit/efl/tests/UnitTestUtils/EWKTestView.h:-66
> > -    OwnPtr<Evas_Object> m_webView;
> 
> I will check which warning is generated.
> 
> But I believe that we can ensure destructor of members will be called in order.
> We can just add comment like http://trac.webkit.org/browser/trunk/Source/WebCore/platform/efl/RenderThemeEfl.h#L251
Thanks for the tip. Indeed, ordering members helped. I used OwnPtrs.

[Krzysztof Czech]

Created attachment 163859 [details]
[EFL][UT] Refactoring an implementation of testing framework for wk1.

[Ryuan Choi]

Comment on attachment 163859 [details]
[EFL][UT] Refactoring an implementation of testing framework for wk1.

View in context: https://bugs.webkit.org/attachment.cgi?id=163859&action=review

Cool, quite better than before.
thank you.

> Source/WebKit/efl/tests/UnitTestUtils/EWKTestBase.cpp:43
> +Evas_Object* EWKTestBase::mainFrame()
>  {
> -    ecore_main_loop_quit();
> +    return ewk_view_frame_main_get(webView());
>  }

mainFrame looks not used now.

what do you think about adding this when it is really needed?

> Source/WebKit/efl/tests/UnitTestUtils/EWKTestView.cpp:32
> +bool EWKTestView::init(int useX11Window, EwkViewType testViewType, int width, int height)

gyuyoung asked why useX11Window is int.
Did you answer it?

And when this function is called, I think that we should set webview to 0 first.
If not, when we create new ecore_evas, previous ecore_evas will be removed before cleaning webview.

> Source/WebKit/efl/tests/UnitTestUtils/EWKTestView.cpp:39
> +    if (!m_ecoreEvas.get())

.get() is not required here.

> Source/WebKit/efl/tests/UnitTestUtils/EWKTestView.cpp:58
>          return false;

if (!m_webView)
    return false;

> Source/WebKit/efl/tests/test_ewk_view.cpp:34
> +    ewk_view_editable_set(webView(), EINA_FALSE);

s/EINA_FALSE/false

[Krzysztof Czech]

(In reply to comment #17)
> (From update of attachment 163859 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163859&action=review
> 
> Cool, quite better than before.
> thank you.
> 
> > Source/WebKit/efl/tests/UnitTestUtils/EWKTestBase.cpp:43
> > +Evas_Object* EWKTestBase::mainFrame()
> >  {
> > -    ecore_main_loop_quit();
> > +    return ewk_view_frame_main_get(webView());
> >  }
> 
> mainFrame looks not used now.
> 
> what do you think about adding this when it is really needed?
Alright.
> 
> > Source/WebKit/efl/tests/UnitTestUtils/EWKTestView.cpp:32
> > +bool EWKTestView::init(int useX11Window, EwkViewType testViewType, int width, int height)
> 
> gyuyoung asked why useX11Window is int.
> Did you answer it?
Yes, I did it. int is used to have compatibility with getopt_long.
> 
> And when this function is called, I think that we should set webview to 0 first.
> If not, when we create new ecore_evas, previous ecore_evas will be removed before cleaning webview.
Generally this function should be called once per test case, then destruction is handled properly. If test case requires another page loading instead of loadUrl, ewk_view_uri_set(...) and waintUntilLoadFinished() should be called.
Initialization (init()) should be done only once per test case.
I thought about to move init() from loadUrl() and call it separately as a first statement of each new test. It might more readable.
What do you think ?
>  
> > Source/WebKit/efl/tests/UnitTestUtils/EWKTestView.cpp:39
> > +    if (!m_ecoreEvas.get())
> 
> .get() is not required here.
Indeed.
> 
> > Source/WebKit/efl/tests/UnitTestUtils/EWKTestView.cpp:58
> >          return false;
> 
> if (!m_webView)
>     return false;
Indeed.
> 
> > Source/WebKit/efl/tests/test_ewk_view.cpp:34
> > +    ewk_view_editable_set(webView(), EINA_FALSE);
Ups :), I haven't noticed, thanks.
> 
> s/EINA_FALSE/false

[Krzysztof Czech]

> And when this function is called, I think that we should set webview to 0 first.
> If not, when we create new ecore_evas, previous ecore_evas will be removed before cleaning webview.

Yes, I think setting webview to 0 before ecore_evas is created sounds very good.
It resolves issue with clearing. Nice :). Ignore my last comment regarding this.

[Krzysztof Czech]

Created attachment 163885 [details]
[EFL][UT] Refactoring an implementation of testing framework for wk1.

[Krzysztof Czech]

Created attachment 163888 [details]
[EFL][UT] Refactoring an implementation of testing framework for wk1.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 163888 [details]
[EFL][UT] Refactoring an implementation of testing framework for wk1.

View in context: https://bugs.webkit.org/attachment.cgi?id=163888&action=review

> Source/WebKit/efl/tests/UnitTestUtils/EWKTestBase.cpp:49
> +    m_ewkTestView = new EWKTestView();

You could use an OwnPtr here so you do not need to delete this pointer manually later.

> Source/WebKit/efl/tests/UnitTestUtils/EWKTestView.h:46
>      OwnPtr<Evas_Object> m_webView;

Evas_Objects should be wrapped inside RefPtrs now.

> Source/WebKit/efl/tests/test_runner.cpp:18
>  */
> +#include "config.h"

Very minor nit: I would add an empty line before this #include.

[Krzysztof Czech]

(In reply to comment #22)
> (From update of attachment 163888 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163888&action=review
> 
> > Source/WebKit/efl/tests/UnitTestUtils/EWKTestBase.cpp:49
> > +    m_ewkTestView = new EWKTestView();
> 
> You could use an OwnPtr here so you do not need to delete this pointer manually later.
I wanna be sure m_ewkTestView is destroyed before ecore_evas_shutdown() and ewk_shutdown(), this is the reason I don't use OwnPtr in this particular case.
> 
> > Source/WebKit/efl/tests/UnitTestUtils/EWKTestView.h:46
> >      OwnPtr<Evas_Object> m_webView;
> 
> Evas_Objects should be wrapped inside RefPtrs now.
Thanks, I will correct.
> 
> > Source/WebKit/efl/tests/test_runner.cpp:18
> >  */
> > +#include "config.h"
> 
> Very minor nit: I would add an empty line before this #include.
Thanks, I will correct.

[Krzysztof Czech]

Created attachment 164556 [details]
[EFL][UT] Refactoring an implementation of testing framework for wk1.

[Ryuan Choi]

(In reply to comment #24)
> Created an attachment (id=164556) [details]
> [EFL][UT] Refactoring an implementation of testing framework for wk1.

Patch itself is good to me.
please rebase the patch.

[Gyuyoung Kim]

Comment on attachment 164556 [details]
[EFL][UT] Refactoring an implementation of testing framework for wk1.

View in context: https://bugs.webkit.org/attachment.cgi?id=164556&action=review

> Source/WebKit/efl/tests/UnitTestUtils/EWKTestView.cpp:66
> +    evas_object_focus_set(m_webView.get(), true);

In public API case, I think it is better to use EINA_FALSE | _TRUE to test for public API. Because, this API will be used by EFL application. So, awk2 test case also uses EINA_TRUE | FALSE.

> Source/WebKit/efl/tests/test_ewk_view.cpp:37
> +    ewk_view_editable_set(webView(), false);

ditto.

[Krzysztof Czech]

Created attachment 164685 [details]
[EFL][UT] Refactoring an implementation of testing framework for wk1.

[Krzysztof Czech]

(In reply to comment #26)
> (From update of attachment 164556 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164556&action=review
> 
> > Source/WebKit/efl/tests/UnitTestUtils/EWKTestView.cpp:66
> > +    evas_object_focus_set(m_webView.get(), true);
> 
> In public API case, I think it is better to use EINA_FALSE | _TRUE to test for public API. Because, this API will be used by EFL application. So, awk2 test case also uses EINA_TRUE | FALSE.
Done.
> 
> > Source/WebKit/efl/tests/test_ewk_view.cpp:37
> > +    ewk_view_editable_set(webView(), false);
> 
> ditto.
Done.

[Ryuan Choi]

Comment on attachment 164685 [details]
[EFL][UT] Refactoring an implementation of testing framework for wk1.

View in context: https://bugs.webkit.org/attachment.cgi?id=164685&action=review

LGTM.
nit below.

> Source/WebKit/efl/tests/test_ewk_view.cpp:26
> +#include <Ecore.h>
> +
> +#include <wtf/OwnPtr.h>

unnecessary empty line

[Gyuyoung Kim]

Comment on attachment 164685 [details]
[EFL][UT] Refactoring an implementation of testing framework for wk1.

Please land after fixing the nit.

[Krzysztof Czech]

Created attachment 164727 [details]
[EFL][UT] Refactoring an implementation of testing framework for wk1.

[Krzysztof Czech]

(In reply to comment #29)
> (From update of attachment 164685 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164685&action=review
> 
> LGTM.
> nit below.
> 
> > Source/WebKit/efl/tests/test_ewk_view.cpp:26
> > +#include <Ecore.h>
> > +
> > +#include <wtf/OwnPtr.h>
> 
> unnecessary empty line
Thanks, done.

[Ryuan Choi]

Comment on attachment 164727 [details]
[EFL][UT] Refactoring an implementation of testing framework for wk1.

thank you.

[WebKit Review Bot]

Comment on attachment 164727 [details]
[EFL][UT] Refactoring an implementation of testing framework for wk1.

Clearing flags on attachment: 164727

Committed r128995: <http://trac.webkit.org/changeset/128995>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

Made the bot red:
http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug/builds/6108

test_ewk_view is segfaulting.

[Chris Dumez]

[ RUN      ] EWKTestBase.ewk_view_editable_get
[       OK ] EWKTestBase.ewk_view_editable_get (64 ms)
[ RUN      ] EWKTestBase.ewk_view_uri_get
ASSERTION FAILED: platformStrategies != s_platformStrategies
/home/buildslave-1/webkit-buildslave/efl-linux-64-debug/build/Source/WebCore/platform/PlatformStrategies.cpp(52) : void WebCore::setPlatformStrategies(WebCore::PlatformStrategies*)
1   0x2b19780aae62 WebCore::setPlatformStrategies(WebCore::PlatformStrategies*)
2   0x2b19748a313d
3   0x2b19748baaad
4   0x2b19748ba8db ewk_init
5   0x4090d7 EWKUnitTests::EWKTestBase::SetUp()
6   0x2b1974b94f5f testing::Test::Run()
7   0x2b1974b955c1 testing::internal::TestInfoImpl::Run()
8   0x2b1974b95b32 testing::TestCase::Run()
9   0x2b1974b9a4fe testing::internal::UnitTestImpl::RunAllTests()
10  0x2b1974b9927c testing::UnitTest::Run()
11  0x407b9d main
12  0x2b197b66476d __libc_start_main
13  0x406c59

[Chris Dumez]

Comment on attachment 164727 [details]
[EFL][UT] Refactoring an implementation of testing framework for wk1.

View in context: https://bugs.webkit.org/attachment.cgi?id=164727&action=review

> Source/WebKit/efl/tests/UnitTestUtils/EWKTestBase.cpp:42
> +    ASSERT_GT(ecore_evas_init(), 0);

It is already called in ewk_init(), it is not needed here.

> Source/WebKit/efl/tests/UnitTestUtils/EWKTestBase.cpp:43
> +    ASSERT_GT(ewk_init(), 0);

We cannot let the ewk_main ref count go to 0 during the program execution or we will do WebCore initializations more than once, which will cause assertions. With your patch, the ref count goes down to 0 after each unit test and ewk_init() will therefore initialize WebCore again next time it is called. You probably need to call ewk_init() / ewk_shutdown() in the EWKTestBase constructor / destructor as well, so that ref count stays 1 between tests.

[Chris Dumez]

(In reply to comment #38)
> (From update of attachment 164727 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164727&action=review
> 
> > Source/WebKit/efl/tests/UnitTestUtils/EWKTestBase.cpp:42
> > +    ASSERT_GT(ecore_evas_init(), 0);
> 
> It is already called in ewk_init(), it is not needed here.
> 
> > Source/WebKit/efl/tests/UnitTestUtils/EWKTestBase.cpp:43
> > +    ASSERT_GT(ewk_init(), 0);
> 
> We cannot let the ewk_main ref count go to 0 during the program execution or we will do WebCore initializations more than once, which will cause assertions. With your patch, the ref count goes down to 0 after each unit test and ewk_init() will therefore initialize WebCore again next time it is called. You probably need to call ewk_init() / ewk_shutdown() in the EWKTestBase constructor / destructor as well, so that ref count stays 1 between tests.

I actually don't believe we need to call ewk_init() / ewk_shutdown() before and after each test. It is probably just enough to call them once in the ctr/dtor.

[WebKit Review Bot]

Re-opened since this is blocked by 97114

[Gyuyoung Kim]

(In reply to comment #36)
> Made the bot red:
> http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug/builds/6108
> 
> test_ewk_view is segfaulting.

Oops, I'm sorry. spank spank spank !! To avoid problem like this, I will check this further when patch touches unit test and APIs. Especially, in debug build.

[Krzysztof Czech]

Created attachment 166178 [details]
[EFL][UT] Refactoring an implementation of testing framework for wk1.

[Gyuyoung Kim]

Comment on attachment 166178 [details]
[EFL][UT] Refactoring an implementation of testing framework for wk1.

Attachment 166178 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14059241

[Krzysztof Czech]

Due to the problems of multiple initializations of ewk_main(), I've made some modifications to the patch. I added global SetUp and TearDown methods and respectively added ewk_init() and ewk_shutdown(). These methods are executed only once which means at the beginning of process execution and before it is finally closed. These two methods are part of EWKTestEnvironemnt class.

[Krzysztof Czech]

Created attachment 166185 [details]
[EFL][UT] Refactoring an implementation of testing framework for wk1.

[Gyuyoung Kim]

Comment on attachment 166185 [details]
[EFL][UT] Refactoring an implementation of testing framework for wk1.

View in context: https://bugs.webkit.org/attachment.cgi?id=166185&action=review

Please verify this patch won't influence on both release and debug build.

> Source/WebKit/PlatformEfl.cmake:324
> +    ${WEBKIT_DIR}/efl/tests/UnitTestUtils/EWKTestEnvironment.cpp

Wrong alphabetical order.

> Source/WebKit/efl/tests/UnitTestUtils/EWKTestEnvironment.h:27
> +    EWKTestEnvironment(bool useX11Window);

Missing explicit keyword

> Source/WebKit/efl/tests/test_ewk_view.cpp:36
> +    ewk_view_editable_set(webView(), EINA_FALSE);

Please use true instead of EINA_FALSE

[Gyuyoung Kim]

Comment on attachment 166185 [details]
[EFL][UT] Refactoring an implementation of testing framework for wk1.

View in context: https://bugs.webkit.org/attachment.cgi?id=166185&action=review

>> Source/WebKit/efl/tests/test_ewk_view.cpp:36
>> +    ewk_view_editable_set(webView(), EINA_FALSE);
> 
> Please use true instead of EINA_FALSE

true -> false

[Krzysztof Czech]

(In reply to comment #46)
> (From update of attachment 166185 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166185&action=review
> 
> Please verify this patch won't influence on both release and debug build.
>
I verified, tests are working properly on both release and debug
> > Source/WebKit/PlatformEfl.cmake:324
> > +    ${WEBKIT_DIR}/efl/tests/UnitTestUtils/EWKTestEnvironment.cpp
> 
> Wrong alphabetical order.
Done.
> 
> > Source/WebKit/efl/tests/UnitTestUtils/EWKTestEnvironment.h:27
> > +    EWKTestEnvironment(bool useX11Window);
> 
> Missing explicit keyword
Done.
> 
> > Source/WebKit/efl/tests/test_ewk_view.cpp:36
> > +    ewk_view_editable_set(webView(), EINA_FALSE);
> 
> Please use true instead of EINA_FALSE
Done.

[Krzysztof Czech]

Created attachment 166457 [details]
[EFL][UT] Refactoring an implementation of testing framework for wk1.

[Krzysztof Czech]

Created attachment 166677 [details]
[EFL][UT] Refactoring an implementation of testing framework for wk1.

[Gyuyoung Kim]

Comment on attachment 166677 [details]
[EFL][UT] Refactoring an implementation of testing framework for wk1.

View in context: https://bugs.webkit.org/attachment.cgi?id=166677&action=review

Please fix them I point out, then I really hope this patch won't roll out again. (Please check this before landing again. :-) )

> Source/WebKit/efl/tests/UnitTestUtils/EWKTestEnvironment.h:17
> +*/

new line ?

> Source/WebKit/efl/tests/UnitTestUtils/EWKTestEnvironment.h:27
> +    EWKTestEnvironment(bool useX11Window);

Missing explicit keyword.

[Krzysztof Czech]

Created attachment 166687 [details]
[EFL][UT] Refactoring an implementation of testing framework for wk1.

[Krzysztof Czech]

(In reply to comment #51)
> (From update of attachment 166677 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166677&action=review
> 
> Please fix them I point out, then I really hope this patch won't roll out again. (Please check this before landing again. :-) )
> 
Checked on both debug and release. Seems fine.
> > Source/WebKit/efl/tests/UnitTestUtils/EWKTestEnvironment.h:17
> > +*/
> 
> new line ?
Done.
> 
> > Source/WebKit/efl/tests/UnitTestUtils/EWKTestEnvironment.h:27
> > +    EWKTestEnvironment(bool useX11Window);
> 
> Missing explicit keyword.
Done

[WebKit Review Bot]

Comment on attachment 166687 [details]
[EFL][UT] Refactoring an implementation of testing framework for wk1.

Clearing flags on attachment: 166687

Committed r130175: <http://trac.webkit.org/changeset/130175>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.