[webkit-reviews] review granted: [Bug 66016] [Qt] Add test for correct order of load signals in QWebPage : [Attachment 103661] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Aug 11 13:45:06 PDT 2011


Benjamin Poulain <benjamin at webkit.org> has granted Caio Marcelo de Oliveira
Filho <cmarcelo at webkit.org>'s request for review:
Bug 66016: [Qt] Add test for correct order of load signals in QWebPage
https://bugs.webkit.org/show_bug.cgi?id=66016

Attachment 103661: Patch
https://bugs.webkit.org/attachment.cgi?id=103661&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
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().


More information about the webkit-reviews mailing list