Bug 91721 – [EFL][WK2] Use "load,finished" signal in EWK2UnitTestBase::loadUrlSync() instead of "load,progress"

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Chris Dumez]

The purpose of EWK2UnitTestBase::loadUrlSync() is to load a URL in the view and wait synchronously for the load to finish.
The current implementation uses the "load,progress" signal to detect when the load is finished, which is inefficient because it gets emitted several times.

It is better to wait for the "load,finished" signal which gets emitted only once when the load is complete.

[Chris Dumez]

Created attachment 153197 [details]
Patch

[Thiago Marcos P. Santos]

Comment on attachment 153197 [details]
Patch

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

Good to know that we finally have "load,finished" for EFL WebKit2. :)

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:-34
> -    UNUSED_PARAM(webView);

It's unclear to me the policy regarding unused parameters at the EFL port. Some reviewers asked me for UNUSED_PARAM a couple of times,. If the policy is not to use, you should also remove <wtf/UnusedParam.h> and, if the policy is to use, you should add for webView and eventInfo.

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.cpp:-87
> -    while (m_loadProgress != 1)
> -        ecore_main_loop_iterate();

I think we still need to use ecore_main_loop_iterate() with some exit condition (maybe s/m_loadProgress/m_loadDone). The reason is we might call the loadUrlSync from inside a callback. If you do that, you will nest two ecore_main_loop_begin() (that first one will probably starve, but the EFL docs doesn't say anything about it).

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestBase.h:-31
> -    void setLoadProgress(float progress) { m_loadProgress = progress; }

You might need a setLoadDone() for the reasons I mentioned at the comment about ecore_main_loop_iterate().

[Chris Dumez]

Comment on attachment 153197 [details]
Patch

Will reupload a patch taking Thiago's feedback into consideration soon. Clearing flags until then.

[Chris Dumez]

Created attachment 153442 [details]
Patch

[Thiago Marcos P. Santos]

Comment on attachment 153442 [details]
Patch

LGTM. Thanks.

[WebKit Review Bot]

Comment on attachment 153442 [details]
Patch

Clearing flags on attachment: 153442

Committed r123199: <http://trac.webkit.org/changeset/123199>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.