Bug 127427 - [EFL] Spelling unit tests should use ecore_main_loop_iterate()
Summary: [EFL] Spelling unit tests should use ecore_main_loop_iterate()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Lukasz Bialek
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-01-22 07:37 PST by Lukasz Bialek
Modified: 2014-02-10 05:00 PST (History)
6 users (show)

See Also:


Attachments
Proposed patch (8.13 KB, patch)
2014-01-24 06:23 PST, Lukasz Bialek
gyuyoung.kim: review+
Details | Formatted Diff | Diff
Patch (8.15 KB, patch)
2014-02-10 03:37 PST, Lukasz Bialek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Lukasz Bialek 2014-01-22 07:37:00 PST
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().
Comment 1 Lukasz Bialek 2014-01-24 06:23:06 PST
Created attachment 222105 [details]
Proposed patch
Comment 2 Sergio Correia (qrwteyrutiyoup) 2014-01-24 06:53:50 PST
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?
Comment 3 Grzegorz Czajkowski 2014-01-24 06:57:33 PST
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.
Comment 4 Lukasz Bialek 2014-01-24 07:13:22 PST
> > 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.
Comment 5 Sergio Correia (qrwteyrutiyoup) 2014-01-28 02:33:32 PST
(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.
Comment 6 Gyuyoung Kim 2014-02-07 03:48:42 PST
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.
Comment 7 Lukasz Bialek 2014-02-10 03:37:40 PST
Created attachment 223689 [details]
Patch

Switched " = 0"  for " = nullptr"
Comment 8 Gyuyoung Kim 2014-02-10 03:39:27 PST
Comment on attachment 223689 [details]
Patch

Reviewer only can set r+. In this case, you need to request cq? only.
Comment 9 WebKit Commit Bot 2014-02-10 05:00:01 PST
Comment on attachment 223689 [details]
Patch

Clearing flags on attachment: 223689

Committed r163779: <http://trac.webkit.org/changeset/163779>
Comment 10 WebKit Commit Bot 2014-02-10 05:00:05 PST
All reviewed patches have been landed.  Closing bug.