Bug 94925

Summary: [EFL][UT]Refactoring an implementation of testing framework for wk1.
Product: WebKit Reporter: Krzysztof Czech <k.czech>
Component: WebKit EFLAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, gyuyoung.kim, lucas.de.marchi, rakuco, ryuan.choi, tmpsantos, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Linux   
Bug Depends on: 97114    
Bug Blocks: 86091, 95984    
Attachments:
Description Flags
Updating ut framework for wk1
none
Updating ut framework for wk1
gyuyoung.kim: review-
[EFL][UT]Refactoring an implementation of testing framework for wk1.
none
[EFL][UT]Refactoring an implementation of testing framework for wk1.
none
[EFL][UT] Refactoring an implementation of testing framework for wk1.
none
[EFL][UT] Refactoring an implementation of testing framework for wk1.
none
[EFL][UT] Refactoring an implementation of testing framework for wk1.
none
[EFL][UT] Refactoring an implementation of testing framework for wk1.
none
[EFL][UT] Refactoring an implementation of testing framework for wk1.
gyuyoung.kim: review+
[EFL][UT] Refactoring an implementation of testing framework for wk1.
none
[EFL][UT] Refactoring an implementation of testing framework for wk1.
gyuyoung.kim: commit-queue-
[EFL][UT] Refactoring an implementation of testing framework for wk1.
none
[EFL][UT] Refactoring an implementation of testing framework for wk1.
none
[EFL][UT] Refactoring an implementation of testing framework for wk1.
gyuyoung.kim: review+
[EFL][UT] Refactoring an implementation of testing framework for wk1. none

Description Krzysztof Czech 2012-08-24 05:18:03 PDT
Refactoring an implementation of testing framework for wk1
Comment 1 Krzysztof Czech 2012-08-24 05:20:11 PDT
Created attachment 160401 [details]
Updating ut framework for wk1
Comment 2 WebKit Review Bot 2012-08-24 05:23:01 PDT
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.
Comment 3 Krzysztof Czech 2012-08-24 09:00:01 PDT
Created attachment 160433 [details]
Updating ut framework for wk1
Comment 4 Ryuan Choi 2012-08-25 10:43:36 PDT
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?
Comment 5 Gyuyoung Kim 2012-09-02 22:54:06 PDT
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 ?
Comment 6 Krzysztof Czech 2012-09-12 03:12:47 PDT
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
Comment 7 Krzysztof Czech 2012-09-12 03:39:49 PDT
Created attachment 163572 [details]
[EFL][UT]Refactoring an implementation of testing framework for wk1.
Comment 8 WebKit Review Bot 2012-09-12 03:42:32 PDT
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.
Comment 9 Krzysztof Czech 2012-09-12 03:52:11 PDT
Created attachment 163576 [details]
[EFL][UT]Refactoring an implementation of testing framework for wk1.
Comment 10 Ryuan Choi 2012-09-12 05:27:31 PDT
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.
Comment 11 Gyuyoung Kim 2012-09-12 05:43:29 PDT
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 ?
Comment 12 Krzysztof Czech 2012-09-12 08:40:13 PDT
(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.
Comment 13 Krzysztof Czech 2012-09-12 08:41:02 PDT
(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.
Comment 14 Ryuan Choi 2012-09-12 09:28:17 PDT
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
Comment 15 Krzysztof Czech 2012-09-13 06:33:50 PDT
(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.
Comment 16 Krzysztof Czech 2012-09-13 06:34:17 PDT
Created attachment 163859 [details]
[EFL][UT] Refactoring an implementation of testing framework for wk1.
Comment 17 Ryuan Choi 2012-09-13 07:04:36 PDT
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
Comment 18 Krzysztof Czech 2012-09-13 08:22:59 PDT
(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
Comment 19 Krzysztof Czech 2012-09-13 08:38:09 PDT
> 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.
Comment 20 Krzysztof Czech 2012-09-13 08:50:02 PDT
Created attachment 163885 [details]
[EFL][UT] Refactoring an implementation of testing framework for wk1.
Comment 21 Krzysztof Czech 2012-09-13 09:17:21 PDT
Created attachment 163888 [details]
[EFL][UT] Refactoring an implementation of testing framework for wk1.
Comment 22 Raphael Kubo da Costa (:rakuco) 2012-09-14 07:35:07 PDT
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.
Comment 23 Krzysztof Czech 2012-09-18 06:50:25 PDT
(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.
Comment 24 Krzysztof Czech 2012-09-18 07:24:13 PDT
Created attachment 164556 [details]
[EFL][UT] Refactoring an implementation of testing framework for wk1.
Comment 25 Ryuan Choi 2012-09-18 16:00:09 PDT
(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.
Comment 26 Gyuyoung Kim 2012-09-18 18:43:34 PDT
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.
Comment 27 Krzysztof Czech 2012-09-19 01:12:54 PDT
Created attachment 164685 [details]
[EFL][UT] Refactoring an implementation of testing framework for wk1.
Comment 28 Krzysztof Czech 2012-09-19 02:24:18 PDT
(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.
Comment 29 Ryuan Choi 2012-09-19 05:57:21 PDT
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
Comment 30 Gyuyoung Kim 2012-09-19 06:12:41 PDT
Comment on attachment 164685 [details]
[EFL][UT] Refactoring an implementation of testing framework for wk1.

Please land after fixing the nit.
Comment 31 Krzysztof Czech 2012-09-19 06:28:56 PDT
Created attachment 164727 [details]
[EFL][UT] Refactoring an implementation of testing framework for wk1.
Comment 32 Krzysztof Czech 2012-09-19 06:29:23 PDT
(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.
Comment 33 Ryuan Choi 2012-09-19 06:32:35 PDT
Comment on attachment 164727 [details]
[EFL][UT] Refactoring an implementation of testing framework for wk1.

thank you.
Comment 34 WebKit Review Bot 2012-09-19 06:53:23 PDT
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>
Comment 35 WebKit Review Bot 2012-09-19 06:53:29 PDT
All reviewed patches have been landed.  Closing bug.
Comment 36 Chris Dumez 2012-09-19 08:26:02 PDT
Made the bot red:
http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug/builds/6108

test_ewk_view is segfaulting.
Comment 37 Chris Dumez 2012-09-19 08:27:20 PDT
[ 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
Comment 38 Chris Dumez 2012-09-19 09:00:07 PDT
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.
Comment 39 Chris Dumez 2012-09-19 09:02:58 PDT
(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.
Comment 40 WebKit Review Bot 2012-09-19 09:41:43 PDT
Re-opened since this is blocked by 97114
Comment 41 Gyuyoung Kim 2012-09-19 18:37:45 PDT
(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.
Comment 42 Krzysztof Czech 2012-09-28 02:12:43 PDT
Created attachment 166178 [details]
[EFL][UT] Refactoring an implementation of testing framework for wk1.
Comment 43 Gyuyoung Kim 2012-09-28 02:20:08 PDT
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
Comment 44 Krzysztof Czech 2012-09-28 02:25:02 PDT
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.
Comment 45 Krzysztof Czech 2012-09-28 02:33:12 PDT
Created attachment 166185 [details]
[EFL][UT] Refactoring an implementation of testing framework for wk1.
Comment 46 Gyuyoung Kim 2012-09-28 03:33:05 PDT
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
Comment 47 Gyuyoung Kim 2012-09-28 03:38:23 PDT
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
Comment 48 Krzysztof Czech 2012-10-01 05:40:36 PDT
(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.
Comment 49 Krzysztof Czech 2012-10-01 05:41:12 PDT
Created attachment 166457 [details]
[EFL][UT] Refactoring an implementation of testing framework for wk1.
Comment 50 Krzysztof Czech 2012-10-02 05:41:14 PDT
Created attachment 166677 [details]
[EFL][UT] Refactoring an implementation of testing framework for wk1.
Comment 51 Gyuyoung Kim 2012-10-02 06:12:52 PDT
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.
Comment 52 Krzysztof Czech 2012-10-02 07:35:03 PDT
Created attachment 166687 [details]
[EFL][UT] Refactoring an implementation of testing framework for wk1.
Comment 53 Krzysztof Czech 2012-10-02 07:36:14 PDT
(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
Comment 54 WebKit Review Bot 2012-10-02 08:59:40 PDT
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>
Comment 55 WebKit Review Bot 2012-10-02 08:59:46 PDT
All reviewed patches have been landed.  Closing bug.