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 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 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
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.
(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.
(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 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
(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 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
(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
> 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 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.
(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.
(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 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.
(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 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.
(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.
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 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.
(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
2012-08-24 05:20 PDT, Krzysztof Czech
2012-08-24 09:00 PDT, Krzysztof Czech
2012-09-12 03:39 PDT, Krzysztof Czech
2012-09-12 03:52 PDT, Krzysztof Czech
2012-09-13 06:34 PDT, Krzysztof Czech
2012-09-13 08:50 PDT, Krzysztof Czech
2012-09-13 09:17 PDT, Krzysztof Czech
2012-09-18 07:24 PDT, Krzysztof Czech
2012-09-19 01:12 PDT, Krzysztof Czech
2012-09-19 06:28 PDT, Krzysztof Czech
2012-09-28 02:12 PDT, Krzysztof Czech
2012-09-28 02:33 PDT, Krzysztof Czech
2012-10-01 05:41 PDT, Krzysztof Czech
2012-10-02 05:41 PDT, Krzysztof Czech
2012-10-02 07:35 PDT, Krzysztof Czech