Bug 90606 – [EFL] Port the test framework to WebKit 2

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Thiago Marcos P. Santos]

No excuses for API without tests.

[Thiago Marcos P. Santos]

Created attachment 150962 [details]
Patch

[Krzysztof Czech]

> void EWK2TestBase::shutdown()
> {
>    ecore_evas_shutdown();
> }

EWK2TestBase::shutdown is not used. I think it can be deleted as well as in wk1 tests

[Thiago Marcos P. Santos]

Created attachment 151034 [details]
Patch

Unused method removed. Thanks for reviewing.

[Krzysztof Czech]

LGTM, thanks

[Thiago Marcos P. Santos]

Created attachment 151049 [details]
Patch

Removed unused include path.

[Thiago Marcos P. Santos]

Created attachment 151065 [details]
Patch

Cleanup more unused include paths.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 151065 [details]
Patch

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

Do you need to add Intel's copyright to any header?

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2TestBase.cpp:27
> +int EWK2UnitTests::EWK2TestBase::useX11Window;

Shouldn't you add a default value here?

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2TestBase.cpp:40
> +    int count = 0;
> +
> +    while ((count = ecore_evas_shutdown()) > 0) { }

Isn't it better to call ecore_evas_shutdown() only once and ASSERT that itit returns so that faulty code gets evident more easily?

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2TestBase.h:63
> +    static int useX11Window;

Isn't it possible to use a bool here?

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2TestConfig.h:23
> +namespace EWK2UnitTests {
> +namespace Config {

Nit: we normally put an empty line after each namespace declaration.

[Thiago Marcos P. Santos]

(In reply to comment #7)
> (From update of attachment 151065 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151065&action=review
> 
> Do you need to add Intel's copyright to any header?
> 

This is basically a copy from WK1 test framework skeleton. Nothing was added, so I kept the original copyright holder since no significant changes (more than 10 lines per file) were made.  

> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2TestBase.cpp:27
> > +int EWK2UnitTests::EWK2TestBase::useX11Window;
> 
> Shouldn't you add a default value here?

I guess since it is static, it is zeroed by default.

> 
> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2TestBase.cpp:40
> > +    int count = 0;
> > +
> > +    while ((count = ecore_evas_shutdown()) > 0) { }
> 
> Isn't it better to call ecore_evas_shutdown() only once and ASSERT that itit returns so that faulty code gets evident more easily?

Yes, gonna change that.

> 
> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2TestBase.h:63
> > +    static int useX11Window;
> 
> Isn't it possible to use a bool here?

I guess it is int because of compatibility with getopt_long()

> 
> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2TestConfig.h:23
> > +namespace EWK2UnitTests {
> > +namespace Config {
> 
> Nit: we normally put an empty line after each namespace declaration.

Roger that.

[Thiago Marcos P. Santos]

> > 
> > > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2TestBase.cpp:40
> > > +    int count = 0;
> > > +
> > > +    while ((count = ecore_evas_shutdown()) > 0) { }
> > 
> > Isn't it better to call ecore_evas_shutdown() only once and ASSERT that itit returns so that faulty code gets evident more easily?
> 
> Yes, gonna change that.
> 

Actually I double checked this on the Ecore docs [1]. It actually makes sense since it returns > 0 when it is still in use. We have too many asynchronous events ongoing and on my tests here it rarely return 0 at the first attempt.

[1] http://docs.enlightenment.org/auto/ecore/group__Ecore__Evas__Group.html#gab8fa311077f749190c9b622f672b2214

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #9)
> Actually I double checked this on the Ecore docs [1]. It actually makes sense since it returns > 0 when it is still in use. We have too many asynchronous events ongoing and on my tests here it rarely return 0 at the first attempt.

It does what all _shutdown() EFL functions do. If the number of calls to _init() and _shutdown() are properly balanced, in the end the counters should all return 0.

[Thiago Marcos P. Santos]

Created attachment 151284 [details]
Patch

[Thiago Marcos P. Santos]

Created attachment 151289 [details]
Patch

[Thiago Marcos P. Santos]

Ok, after the feedback, the patch was completely reworked. Waiting for comments.

Please note that it is now more gtest-ish.

[Gyuyoung Kim]

Comment on attachment 151289 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:31
> +static void onLoadProgress(void *userData, Evas_Object *webView, void *eventInfo)

nit : Move * to variable type side.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 151289 [details]
Patch

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

> Source/WebKit2/PlatformEfl.cmake:215
> +    TARGET_LINK_LIBRARIES(${testName} ${EWK2UnitTests_LIBRARIES} ewk2UnitTestUtils gtest pthread)

Do you really need to pass pthread here? If it's really necessary, you should use find_package(Threads) and link to ${CMAKE_THREAD_LIBS_INIT}.

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:62
> +    m_webView = ewk_view_add(evas);

Isn't this leaking? We have an OwnPtr overload for Evas_Object* that's probably handy here.

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:70
> +    ecore_evas_free(m_ecoreEvas);

We have an OwnPtr overload for Ecore_Evas*. You can use it here and avoid having to manually keep track of the memory.

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:74
> +

Nit: extra empty line.

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:83
> +    while (m_loadProgress != 1.f)
> +        ecore_main_loop_iterate();

Where's the main loop being started and shut down? Do you really need to busy wait like this?

[Krzysztof Czech]

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

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.h:44
> +    Evas_Object* m_webView;

I think OwnPtr can be used instead.

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.h:45
> +    Ecore_Evas* m_ecoreEvas;

I think OwnPtr can be used instead.

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.h:48
> +    unsigned int m_height;

I see m_width, m_height are hardcoded, so maybe it can be changed to
static const int m_width = 800;
static const int m_height = 600;

[Thiago Marcos P. Santos]

(In reply to comment #15)
> (From update of attachment 151289 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151289&action=review
> 
> > Source/WebKit2/PlatformEfl.cmake:215
> > +    TARGET_LINK_LIBRARIES(${testName} ${EWK2UnitTests_LIBRARIES} ewk2UnitTestUtils gtest pthread)
> 
> Do you really need to pass pthread here? If it's really necessary, you should use find_package(Threads) and link to ${CMAKE_THREAD_LIBS_INIT}.
> 

It is need by gtest, so maybe it is in the wrong place. Gonna verify that.

> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:62
> > +    m_webView = ewk_view_add(evas);
> 
> Isn't this leaking? We have an OwnPtr overload for Evas_Object* that's probably handy here.
> 

I'm going to double check ecore_evas source code (since the docs doesn't say much), but I assumed that ecore_evas_free was in charge to clean up it's children. My tests using valgrind + system malloc doesn't show any leak though. Maybe I'm been coding Qt for too long. :)

> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:70
> > +    ecore_evas_free(m_ecoreEvas);
> 
> We have an OwnPtr overload for Ecore_Evas*. You can use it here and avoid having to manually keep track of the memory.
> 

The reason why I'm not using OwnPtr is because I want to ensure coherency. Not sure if it is a good idea to destruct them after ecore_evas_shutdown() (which would be the case when using auto pointers).

> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:74
> > +
> 
> Nit: extra empty line.
> 
> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:83
> > +    while (m_loadProgress != 1.f)
> > +        ecore_main_loop_iterate();
> 
> Where's the main loop being started and shut down? Do you really need to busy wait like this?

This is not a busy wait, it is totally event/timeout oriented (pretty much what the main loop does). Just a convenience function to load a page synchronously since I'm expecting to most of the tests are going to be "load a page, verify getters". This way you can write your tests inline.

Qt utests uses title change events to ensure a certain state inside the page, since even when the page is 100% loaded you might still have some JS executing. I should write something similar to wait for some title changes/value synchronously. This makes a lot easier and cleaner to write tests.

If needed, each test should start the main_loop and stop it when is most appropriated.

[Thiago Marcos P. Santos]

(In reply to comment #16)
> View in context: https://bugs.webkit.org/attachment.cgi?id=151289&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.h:44
> > +    Evas_Object* m_webView;
> 
> I think OwnPtr can be used instead.
> 
> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.h:45
> > +    Ecore_Evas* m_ecoreEvas;
> 
> I think OwnPtr can be used instead.
> 
> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.h:48
> > +    unsigned int m_height;
> 
> I see m_width, m_height are hardcoded, so maybe it can be changed to
> static const int m_width = 800;
> static const int m_height = 600;

Maybe I should put these globals at ::testing::Environment.

[Thiago Marcos P. Santos]

Created attachment 151532 [details]
Patch

The pthreads issue is going to be fixed in another bug. I'm going to make gtest a shared library now that we have many customers for it (TestWebKitAPI is also comming), and fix the link problem for wk1 and wk2 tests. I still not using OwnPtr because of destruction order coherence.

[Thiago Marcos P. Santos]

(In reply to comment #19)
> Created an attachment (id=151532) [details]
> Patch
> 
> The pthreads issue is going to be fixed in another bug. I'm going to make gtest a shared library now that we have many customers for it (TestWebKitAPI is also comming), and fix the link problem for wk1 and wk2 tests. I still not using OwnPtr because of destruction order coherence.

Here: bug 90973

[Raphael Kubo da Costa (:rakuco)]

Right now I only have some questions left:

 - You mention not being able to use OwnPtr with the init/shutdown stuff. I'm not very familiar with how gtest is supposed to work; wouldn't it be possible to init and shutdown in the constructor and the destructor, respectively?

 - On a side note, it's generally good practice to init and shutdown everything you use (evas, for example).

 - On loadUrlSync don't you actually need to init and shut down the main loop since you are using it?

[Thiago Marcos P. Santos]

(In reply to comment #21)
> Right now I only have some questions left:
> 
>  - You mention not being able to use OwnPtr with the init/shutdown stuff. I'm not very familiar with how gtest is supposed to work; wouldn't it be possible to init and shutdown in the constructor and the destructor, respectively?
> 

Yes, it is possible, but it's not a good idea since we use ASSERT_GT on the constructor and gtest team want to make assertions throw exceptions.

>  - On a side note, it's generally good practice to init and shutdown everything you use (evas, for example).
> 

Yes, the latest version of the patch deletes everything. I found no evidence of ecore_evas_free deleting it's children.

>  - On loadUrlSync don't you actually need to init and shut down the main loop since you are using it?

No. Actually that's the point of loadUrlSync.

[Thiago Marcos P. Santos]

> >  - You mention not being able to use OwnPtr with the init/shutdown stuff. I'm not very familiar with how gtest is supposed to work; wouldn't it be possible to init and shutdown in the constructor and the destructor, respectively?
> > 
> 
> Yes, it is possible, but it's not a good idea since we use ASSERT_GT on the constructor and gtest team want to make assertions throw exceptions.

s/constructor/SetUp

[Chris Dumez]

Comment on attachment 151532 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:43
> +    : m_loadProgress(0.f)

Use 0, not 0.f. This is part of the coding style.

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:49
> +void EWK2UnitTestBase::SetUp()

Why does this start with a capital letter?

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:67
> +    evas_object_focus_set(m_webView, EINA_TRUE);

Should be true, not EINA_TRUE.

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:70
> +void EWK2UnitTestBase::TearDown()

Why does this start with a capital letter?

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:79
> +    m_loadProgress = 0;

Should we have an EINA SAFETY for url?

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:84
> +    while (m_loadProgress != 1.f)

Use 1, not 1.f, as per coding style.

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.h:32
> +    Evas_Object* webView() { return m_webView; }

Should be const.

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.h:49
> +}

// namespace EWK2UnitTest

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.h:51
> +#endif

// EWK2UnitTestBase_h

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestEnvironment.cpp:37
> +}

// namespace EWK2UnitTest

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestEnvironment.h:45
> +#endif

// EWK2UnitTestEnvironment_h

[Thiago Marcos P. Santos]

(In reply to comment #24)
> (From update of attachment 151532 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151532&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:43
> > +    : m_loadProgress(0.f)
> 
> Use 0, not 0.f. This is part of the coding style.
> 
> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:49
> > +void EWK2UnitTestBase::SetUp()
> 
> Why does this start with a capital letter?
> 

This is enforced by gtest.

> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:67
> > +    evas_object_focus_set(m_webView, EINA_TRUE);
> 
> Should be true, not EINA_TRUE.
> 

Docs says Eina_Bool.

http://docs.enlightenment.org/auto/evas/group__Evas__Object__Group__Basic.html#ga114336ce1de94dbb13843cca2516e34b

> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:70
> > +void EWK2UnitTestBase::TearDown()
> 
> Why does this start with a capital letter?
> 

Enforced by gtest.

> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:79
> > +    m_loadProgress = 0;
> 
> Should we have an EINA SAFETY for url?
> 

I don't think so. I'm not accessing "url", just handling it directly to EWK. The check would be at the one that changes/reads it.

> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:84
> > +    while (m_loadProgress != 1.f)
> 
> Use 1, not 1.f, as per coding style.
> 
> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.h:32
> > +    Evas_Object* webView() { return m_webView; }
> 
> Should be const.
> 

Not really. What about const correctness? I've seen WebKit respecting this rule in many places:

Example -> CSSToStyleMap* styleMap() { return &m_styleMap; }

> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.h:49
> > +}
> 
> // namespace EWK2UnitTest
> 
> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.h:51
> > +#endif
> 
> // EWK2UnitTestBase_h
> 
> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestEnvironment.cpp:37
> > +}
> 
> // namespace EWK2UnitTest
> 
> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestEnvironment.h:45
> > +#endif
> 
> // EWK2UnitTestEnvironment_h

[Thiago Marcos P. Santos]

Created attachment 151925 [details]
Patch

Thanks for having a look.

[Kenneth Rohde Christiansen]

Comment on attachment 151925 [details]
Patch

Looks fine

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #22)
> >  - On a side note, it's generally good practice to init and shutdown everything you use (evas, for example).
> Yes, the latest version of the patch deletes everything. I found no evidence of ecore_evas_free deleting it's children.

I mean, you should probably call evas_init() and evas_shutdown().

> >  - On loadUrlSync don't you actually need to init and shut down the main loop since you are using it?
> No. Actually that's the point of loadUrlSync.

So you can/should iterate without starting the main loop?

(In reply to comment #25)
> (In reply to comment #24)
> > > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.h:32
> > > +    Evas_Object* webView() { return m_webView; }
> > 
> > Should be const.
> Not really. What about const correctness? I've seen WebKit respecting this rule in many places:
> 
> Example -> CSSToStyleMap* styleMap() { return &m_styleMap; }

It depends on whether callers are expected to call functions which take a non-const Evas_Object*. Right now only ewk_view_uri_get() is called, but I guess non-const functions will be called at some point.

[Chris Dumez]

(In reply to comment #28)
> (In reply to comment #22)
> > >  - On a side note, it's generally good practice to init and shutdown everything you use (evas, for example).
> > Yes, the latest version of the patch deletes everything. I found no evidence of ecore_evas_free deleting it's children.
> 
> I mean, you should probably call evas_init() and evas_shutdown().
> 
> > >  - On loadUrlSync don't you actually need to init and shut down the main loop since you are using it?
> > No. Actually that's the point of loadUrlSync.
> 
> So you can/should iterate without starting the main loop?
> 
> (In reply to comment #25)
> > (In reply to comment #24)
> > > > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.h:32
> > > > +    Evas_Object* webView() { return m_webView; }
> > > 
> > > Should be const.
> > Not really. What about const correctness? I've seen WebKit respecting this rule in many places:
> > 
> > Example -> CSSToStyleMap* styleMap() { return &m_styleMap; }
> 
> It depends on whether callers are expected to call functions which take a non-const Evas_Object*. Right now only ewk_view_uri_get() is called, but I guess non-const functions will be called at some point.

I meant that the getter should be const since it does not alter the object, not a const return value. For e.g.:

inline HTMLFrameOwnerElement* Frame::ownerElement() const
{
    return m_ownerElement;
}

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 151925 [details]
Patch

cq+'ing so Thiago can go to bed in peace :-)

I finally understood what Chris was trying to say, and I agree with him. Thiago seems to be OK with it as well, but he'll do that in another patch.

[Thiago Marcos P. Santos]

(In reply to comment #30)
> (From update of attachment 151925 [details])
> cq+'ing so Thiago can go to bed in peace :-)
> 
> I finally understood what Chris was trying to say, and I agree with him. Thiago seems to be OK with it as well, but he'll do that in another patch.

I will ignore what I learned from the Scott Meyers books in favor of consistence with the rest of EFL port. :-P

[WebKit Review Bot]

Comment on attachment 151925 [details]
Patch

Clearing flags on attachment: 151925

Committed r122512: <http://trac.webkit.org/changeset/122512>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Thiago Marcos P. Santos]

*** Bug 68510 has been marked as a duplicate of this bug. ***