The tests in qdeclarativewebview.cpp needs improvements (at path Source/WebKit/qt/tests/qdeclarativewebview/ ). Currently the basicproperties() test case tries to test too many properties in one single test case. They need to be split into simpler test cases to test each property, so that when one test case fails, it is easy to narrow down which property failed the test. Same thing with historyNav() test case.
Created attachment 94486 [details] Proposed patch
Comment on attachment 94486 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=94486&action=review > Source/WebKit/qt/tests/qdeclarativewebview/tst_qdeclarativewebview.cpp:568 > + checkNoErrors(component); This pattern is repetitive? Could you refactor that and put the component and engine common for all (and even perhaps all tests)?
Created attachment 94697 [details] Modified patch
Updated the patch as per above suggestion from Alexis.
Comment on attachment 94697 [details] Modified patch 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.
Created attachment 95019 [details] Updated patch Simon's suggestions above are incorporated into this updated patch.
Comment on attachment 95019 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=95019&action=review > ChangeLog:7 > + You need to explain what you are doing. > Source/WebKit/qt/tests/qdeclarativewebview/tst_qdeclarativewebview.cpp:77 > + Q_UNUSED(waitForSignal) Indentation is wrong.
Created attachment 95203 [details] Patch updated and resubmitted incorporating the corrections suggested by the reviewer above.
Comment on attachment 95203 [details] Patch updated and resubmitted incorporating the corrections suggested by the reviewer above. View in context: https://bugs.webkit.org/attachment.cgi?id=95203&action=review > Source/WebKit/qt/tests/qdeclarativewebview/tst_qdeclarativewebview.cpp:113 > + m_component= new QDeclarativeComponent(m_engine); Indentation issue. 4 blank spaces. > Source/WebKit/qt/tests/qdeclarativewebview/tst_qdeclarativewebview.cpp:117 > + delete m_component; Ditto.
Created attachment 96103 [details] Updated Patch Patch updated to correct the indentation issue. Please review.
Ping Qt reviewers?
As far as I can tell the project does not want this patch. Would a Qt person please r- and close?
Comment on attachment 96103 [details] Updated Patch Venkat, are you still interested in getting this change in?
No, not interested as really long time has been passed since I submitted this bug and webkit has gone through many changes since then. This patch is not that important now. I am planning to review and submit qml tests for webkit2 webview api soon anyway in a separate patch.