Bug 127427 – [EFL] Spelling unit tests should use ecore_main_loop_iterate()

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

[Lukasz Bialek]

Spelling unit tests used timers for waiting for function results as a result of implementation of spelling languages loading. After refactoring of this code, timers in unit tests were replaced with ecore_main_loop_iterate().

[Lukasz Bialek]

Created attachment 222105 [details]
Proposed patch

[Sergio Correia (qrwteyrutiyoup)]

Comment on attachment 222105 [details]
Proposed patch

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

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:243
> +    Eina_List* loadedLanguages = 0;
> +    void* actual = 0;

nullptr instead of 0.

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

nullptr again.

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:255
> +        ecore_main_loop_iterate();

The documentation seems to suggest this function to be avoided:
http://docs.enlightenment.org/auto/ecore/group__Ecore__Main__Loop__Group.html#ga7f5463c1d4f3f020968ed06d6e5816cb

Do you really need to use it here?

[Grzegorz Czajkowski]

Comment on attachment 222105 [details]
Proposed patch

You're right. EFL's spelchecking has stopped using idlers since r148670. Definitely +1 from my side. Thanks.

[Lukasz Bialek]

> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:255
> > +        ecore_main_loop_iterate();
> 
> The documentation seems to suggest this function to be avoided:
> http://docs.enlightenment.org/auto/ecore/group__Ecore__Main__Loop__Group.html#ga7f5463c1d4f3f020968ed06d6e5816cb
> 
> Do you really need to use it here?

This function is commonly used in spell-checking unit tests. It has been decided to use it here, just like in other places. That is why I think this specific usage of this function is valid.

It is quite important to mention, that previous solution was based on waiting specific time (always the same). New concept allows to quit waiting as soon as all need data is collected.

[Sergio Correia (qrwteyrutiyoup)]

(In reply to comment #4)
> > > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:255
> > > +        ecore_main_loop_iterate();
> > 
> > The documentation seems to suggest this function to be avoided:
> > http://docs.enlightenment.org/auto/ecore/group__Ecore__Main__Loop__Group.html#ga7f5463c1d4f3f020968ed06d6e5816cb
> > 
> > Do you really need to use it here?
> 
> This function is commonly used in spell-checking unit tests. It has been decided to use it here, just like in other places. That is why I think this specific usage of this function is valid.
> 
> It is quite important to mention, that previous solution was based on waiting specific time (always the same). New concept allows to quit waiting as soon as all need data is collected.

Go ahead, then :)
It's just the documentation kinda tells one to avoid it, but if it fits well the design here, no problem, I guess.

[Gyuyoung Kim]

Comment on attachment 222105 [details]
Proposed patch

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

rs=me

>> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:243
>> +    void* actual = 0;
> 
> nullptr instead of 0.

Please fix this one before landing.

>> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:249
>> +            actual = 0;
> 
> nullptr again.

ditto.

[Lukasz Bialek]

Created attachment 223689 [details]
Patch

Switched " = 0"  for " = nullptr"

[Gyuyoung Kim]

Comment on attachment 223689 [details]
Patch

Reviewer only can set r+. In this case, you need to request cq? only.

[WebKit Commit Bot]

Comment on attachment 223689 [details]
Patch

Clearing flags on attachment: 223689

Committed r163779: <http://trac.webkit.org/changeset/163779>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.