Bug 90606

Created attachment 150962 [details] Patch

> void EWK2TestBase::shutdown() > { > ecore_evas_shutdown(); > } EWK2TestBase::shutdown is not used. I think it can be deleted as well as in wk1 tests

Created attachment 151034 [details] Patch Unused method removed. Thanks for reviewing.

LGTM, thanks

Created attachment 151049 [details] Patch Removed unused include path.

Created attachment 151065 [details] Patch Cleanup more unused include paths.

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.

(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.

> > > > > 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

(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.

Created attachment 151284 [details] Patch

Created attachment 151289 [details] Patch

Ok, after the feedback, the patch was completely reworked. Waiting for comments. Please note that it is now more gtest-ish.

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.

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?

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;

(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.

(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.

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.

(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

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?

(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.

> > - 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

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

(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

Created attachment 151925 [details] Patch Thanks for having a look.

Comment on attachment 151925 [details] Patch Looks fine

(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.

(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; }

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.

(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

Comment on attachment 151925 [details] Patch Clearing flags on attachment: 151925 Committed r122512: <http://trac.webkit.org/changeset/122512>

All reviewed patches have been landed. Closing bug.

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