[webkit-reviews] review denied: [Bug 50222] [Qt] QML WebView inside a Flickable shows checkers pattern at startup : [Attachment 83470] Updated patch based on suggestion from reviewer

[bugzilla-daemon at webkit.org]

Alexis Menard <alexis.menard at openbossa.org> has denied Gopal Raghavan
<gopal.1.raghavan at nokia.com>'s request for review:
Bug 50222: [Qt] QML WebView inside a Flickable shows checkers pattern at
startup
https://bugs.webkit.org/show_bug.cgi?id=50222

Attachment 83470: Updated patch based on suggestion from reviewer
https://bugs.webkit.org/attachment.cgi?id=83470&action=review

------- Additional Comments from Alexis Menard <alexis.menard at openbossa.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=83470&action=review

Thanks for the patch. Modify it to take into account the comments :).

> Source/WebKit/qt/tests/qdeclarativewebview/tst_qdeclarativewebview.cpp:6
> +#include <QtDeclarative/qdeclarativeengine.h>

Why mixing #include <QDeclarativeView> and the other types of includes #include
<QtDeclarative/qdeclarativecomponent.h>

> Source/WebKit/qt/tests/qdeclarativewebview/tst_qdeclarativewebview.cpp:7
> +#include <qtest.h>

Why not used <QtTest/QTest>

> Source/WebKit/qt/tests/qdeclarativewebview/tst_qdeclarativewebview.cpp:22
> +    void qTryVerify(bool);

No need. Use the one in QTestLib like other tests.

> Source/WebKit/qt/tests/qdeclarativewebview/tst_qdeclarativewebview.cpp:46
> +    QDeclarativeComponent component(&engine,
QUrl::fromLocalFile(TESTS_SOURCE_DIR"/qdeclarativewebview/resources/webviewtest
.qml"));

In resources as suggested on IRC.

> Source/WebKit/qt/tests/qdeclarativewebview/tst_qdeclarativewebview.cpp:57
> +	   QSKIP(QString("This test requires access to resources found in
'%1'").arg(TESTS_SOURCE_DIR).toLatin1().constData(), SkipAll);

Don't forget to remove that if you use the resources.

> Source/WebKit/qt/tests/qdeclarativewebview/tst_qdeclarativewebview.cpp:71
> +	   QSKIP(QString("This test requires access to resources found in
'%1'").arg(TESTS_SOURCE_DIR).toLatin1().constData(), SkipAll);

Same as 57.

> Source/WebKit/qt/tests/qdeclarativewebview/tst_qdeclarativewebview.cpp:105
> +	       qWarning() << errorStr;

Why? We don't need to print in the terminal the warnings. It fails or pass like
the QVERIFY below.

> Source/WebKit/qt/tests/qdeclarativewebview/resources/webviewtest.qml:17
> +	url: "http://qt.nokia.com"

Tab issue.

> Source/WebKit/qt/tests/qdeclarativewebview/resources/webviewtest.qml:18
> +	   smooth: false

Check the tab or spaces. It has to be spaces.

>
Source/WebKit/qt/tests/qdeclarativewebview/resources/webviewtestdefault.qml:17
> +	url: "http://qt.nokia.com"

Same tab issue.