Bug 91721 - [EFL][WK2] Use "load,finished" signal in EWK2UnitTestBase::loadUrlSync() instead of "load,progress"
Summary: [EFL][WK2] Use "load,finished" signal in EWK2UnitTestBase::loadUrlSync() inst...
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
Depends on:
Reported: 2012-07-19 00:40 PDT by Chris Dumez
Modified: 2012-07-20 03:17 PDT (History)
5 users (show)

See Also:

Patch (3.68 KB, patch)
2012-07-19 00:43 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (3.78 KB, patch)
2012-07-20 01:15 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2012-07-19 00:40:54 PDT
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.
Comment 1 Chris Dumez 2012-07-19 00:43:09 PDT
Created attachment 153197 [details]
Comment 2 Thiago Marcos P. Santos 2012-07-19 01:44:02 PDT
Comment on attachment 153197 [details]

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().
Comment 3 Chris Dumez 2012-07-20 00:54:32 PDT
Comment on attachment 153197 [details]

Will reupload a patch taking Thiago's feedback into consideration soon. Clearing flags until then.
Comment 4 Chris Dumez 2012-07-20 01:15:48 PDT
Created attachment 153442 [details]
Comment 5 Thiago Marcos P. Santos 2012-07-20 01:38:55 PDT
Comment on attachment 153442 [details]

LGTM. Thanks.
Comment 6 WebKit Review Bot 2012-07-20 03:17:20 PDT
Comment on attachment 153442 [details]

Clearing flags on attachment: 153442

Committed r123199: <http://trac.webkit.org/changeset/123199>
Comment 7 WebKit Review Bot 2012-07-20 03:17:26 PDT
All reviewed patches have been landed.  Closing bug.