| Summary: | [EFL] Spelling unit tests should use ecore_main_loop_iterate() | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Lukasz Bialek <l.bialek> | ||||||
| Component: | WebKit EFL | Assignee: | Lukasz Bialek <l.bialek> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | bunhere, cdumez, commit-queue, gyuyoung.kim, lucas.de.marchi, sergio | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Lukasz Bialek
2014-01-22 07:37:00 PST
Created attachment 222105 [details]
Proposed patch
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 on attachment 222105 [details] Proposed patch You're right. EFL's spelchecking has stopped using idlers since r148670. Definitely +1 from my side. Thanks. > > 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.
(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 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. Created attachment 223689 [details]
Patch
Switched " = 0" for " = nullptr"
Comment on attachment 223689 [details]
Patch
Reviewer only can set r+. In this case, you need to request cq? only.
Comment on attachment 223689 [details] Patch Clearing flags on attachment: 223689 Committed r163779: <http://trac.webkit.org/changeset/163779> All reviewed patches have been landed. Closing bug. |