Bug 66016 – [Qt] Add test for correct order of load signals in QWebPage

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Caio Marcelo de Oliveira Filho]

[Qt] Add test for correct order of loadProgress() and loadFinished() signals

[Caio Marcelo de Oliveira Filho]

Created attachment 103541 [details]
Patch

[Luiz Agostini]

Comment on attachment 103541 [details]
Patch

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

> Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:3178
> +    QSignalSpy(&page, SIGNAL(loadFinished(bool)));

Is it needed?

[Caio Marcelo de Oliveira Filho]

Created attachment 103543 [details]
Patch

[Benjamin Poulain]

Comment on attachment 103543 [details]
Patch

For the sake of code coverage, could you ensure loadStarted() is also send before loadProgress()?

+page.mainFrame()->load(QUrl("data:text/html,This is first page")); 
This is a special case. I would like another load() with a local URL.

[Caio Marcelo de Oliveira Filho]

Created attachment 103661 [details]
Patch

[Benjamin Poulain]

Comment on attachment 103661 [details]
Patch

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

Great patch.

> Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:3151
> +    SpyForLoadSignalsOrder(QWebPage* page)
> +        : QStateMachine(page)

This is setting the parent of the QStateMachine to page. I think they should be two separate things:
SpyForLoadSignalsOrder(QWebPage* page, QObject* parent= 0);

This is because I am not a fan of:
SpyForLoadSignalsOrder loadSpy(&page);
(parenting something on the stack by a QObject on the stack)

> Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:3195
> +    if (loadSpy.isRunning())
> +        QVERIFY(waitForSignal(&loadSpy, SIGNAL(finished())));

May I suggest to add
QVERIFY(loadSpy.isComplete());
after the if() {} ?
The isComplete() would just return !isRunning();

This is totally superfluous, your test is already correct. But I think that would make it easier for people to read the test.
If someone read the test without knowing anything about how SpyForLoadSignalsOrder works, she will think: the QVERIFY is in a branch, it is probably wrong because loadSpy could have finished before the if().

[Caio Marcelo de Oliveira Filho]

Committed r92963: <http://trac.webkit.org/changeset/92963>

[Ademar Reis]

Revision r92963 cherry-picked into qtwebkit-2.2 with commit 85204c9 <http://gitorious.org/webkit/qtwebkit/commit/85204c9>