Bug 66016

Summary: [Qt] Add test for correct order of load signals in QWebPage
Product: WebKit Reporter: Caio Marcelo de Oliveira Filho <cmarcelo>
Component: New BugsAssignee: Caio Marcelo de Oliveira Filho <cmarcelo>
Severity: Normal CC: ademar, benjamin
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Description Flags
Patch benjamin: review+

Description Caio Marcelo de Oliveira Filho 2011-08-10 15:25:53 PDT
[Qt] Add test for correct order of loadProgress() and loadFinished() signals
Comment 1 Caio Marcelo de Oliveira Filho 2011-08-10 15:27:10 PDT
Created attachment 103541 [details]
Comment 2 Luiz Agostini 2011-08-10 15:31:51 PDT
Comment on attachment 103541 [details]

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?
Comment 3 Caio Marcelo de Oliveira Filho 2011-08-10 15:41:16 PDT
Created attachment 103543 [details]
Comment 4 Benjamin Poulain 2011-08-11 10:21:13 PDT
Comment on attachment 103543 [details]

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.
Comment 5 Caio Marcelo de Oliveira Filho 2011-08-11 13:25:29 PDT
Created attachment 103661 [details]
Comment 6 Benjamin Poulain 2011-08-11 13:45:07 PDT
Comment on attachment 103661 [details]

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
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().
Comment 7 Caio Marcelo de Oliveira Filho 2011-08-12 08:16:14 PDT
Committed r92963: <http://trac.webkit.org/changeset/92963>
Comment 8 Ademar Reis 2011-09-05 09:41:40 PDT
Revision r92963 cherry-picked into qtwebkit-2.2 with commit 85204c9 <http://gitorious.org/webkit/qtwebkit/commit/85204c9>