[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