[webkit-reviews] review denied: [Bug 61304] [Qt] QML WebView API tests need improvements : [Attachment 94697] Modified patch

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has denied Venkat Penukonda
<venkat.2.penukonda at nokia.com>'s request for review:
Bug 61304: [Qt] QML WebView API tests need improvements
https://bugs.webkit.org/show_bug.cgi?id=61304

Attachment 94697: Modified patch
https://bugs.webkit.org/attachment.cgi?id=94697&action=review

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=94697&action=review

I think the general direction is correct, but I'm saying r- because of the
nitpicks and because the patch should add only one entry to the ChangeLog, not
two.

> Source/WebKit/qt/tests/qdeclarativewebview/tst_qdeclarativewebview.cpp:29
> +protected:
> +    QDeclarativeEngine* m_engine;
> +    QDeclarativeComponent* m_component;
> +

I suppose those should be private instead of protected?

> Source/WebKit/qt/tests/qdeclarativewebview/tst_qdeclarativewebview.cpp:75
>  tst_QDeclarativeWebView::tst_QDeclarativeWebView()
>  {
> -    Q_UNUSED(waitForSignal)
> +	m_engine = new QDeclarativeEngine();
> +	m_component= new QDeclarativeComponent(m_engine);
> +	Q_UNUSED(waitForSignal)

If you want to minimize the risk of side-effects between the test cases, then
you could create and destroy the engine/component in init() and cleanup().

> Source/WebKit/qt/tests/qdeclarativewebview/tst_qdeclarativewebview.cpp:134
> +    QCOMPARE(wv->property("title").toString(), QLatin1String("")); // empty
string by default

I suggest to use QVERIFY and QString::isEmpty() instead of comparing with an
empty QLatin1String.

> Source/WebKit/qt/tests/qdeclarativewebview/tst_qdeclarativewebview.cpp:551
> +void tst_QDeclarativeWebView::settings_defaultFontSize()
> +{
> +    m_component->loadUrl(QUrl("qrc:///resources/basic.qml"));
> +    QObject* wv = m_component->create();
> +    QVERIFY(wv);
> +    QTRY_COMPARE(wv->property("progress").toDouble(), 1.0);
> +
> +    QObject* s = QDeclarativeProperty(wv, "settings").object();
> +    QVERIFY(s);
> +    const QString& name = QString::fromAscii("defaultFontSize");
> +    int value = QWebSettings::DefaultFontSize;
> +    s->setProperty(name.toAscii().data(), value);
> +    QCOMPARE(s->property(name.toAscii().data()).toInt(), value);
> +}
> +void tst_QDeclarativeWebView::settings_standardFontFamily()
> +{
> +    m_component->loadUrl(QUrl("qrc:///resources/basic.qml"));
> +    QObject* wv = m_component->create();
> +    QVERIFY(wv);
> +    QTRY_COMPARE(wv->property("progress").toDouble(), 1.0);
> +
> +    QObject* s = QDeclarativeProperty(wv, "settings").object();
> +    QVERIFY(s);
> +    const QString& name = QString::fromAscii("standardFontFamily");
> +    const QString& value = "Arial";
> +    s->setProperty(name.toAscii().data(), value);
> +    QCOMPARE(s->property(name.toAscii().data()).toString(), value);
> +}
> +

Why not use QTestLib's data driven testing for testing the symmetry of all the
properties instead of duplicating all the code?

Then you get the best of both: No duplicated code but if one property test
fails the remaining tests are still executed.