[webkit-reviews] review denied: [Bug 50222] [Qt] QML WebView inside a Flickable shows checkers pattern at startup : [Attachment 83323] Fixed build issues

[bugzilla-daemon at webkit.org]

Kenneth Rohde Christiansen <kenneth at webkit.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 83323: Fixed build issues
https://bugs.webkit.org/attachment.cgi?id=83323&action=review

------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=83323&action=review

> Source/WebKit/qt/ChangeLog:7
> +	   https://bugs.webkit.org/show_bug.cgi?id=50222. With this patch you
will
> +	   not see checkerboard in webview even if preferredWidth and 

Please split lines here

> Source/WebKit/qt/declarative/qdeclarativewebview.cpp:281
> +    if (!preferredWidth())
> +	   setPreferredWidth(d->view->preferredWidth());
> +    if (!preferredHeight())
> +	   setPreferredHeight(d->view->preferredHeight());

Probably OK, but would it make more sense for preferredWidth to return
d->view->preferredWidth() if setPreferredWidth was never set? The changelog
also does not explain why this solution is correct.

> Source/WebKit/qt/tests/qdeclarativewebview/tst_qdeclarativewebview.cpp:31
> +#define QTRY_VERIFY(__expr) \

I do not think that using this one place is enough for making it a macro. This
just obscures the code.

> Source/WebKit/qt/tests/qdeclarativewebview/tst_qdeclarativewebview.cpp:64
> +void tst_QDeclarativeWebView::initTestCase()
> +{
> +}
> +
> +// This will be called after the last test function is executed.
> +// It is only called once.
> +void tst_QDeclarativeWebView::cleanupTestCase()
> +{
> +}
> +
> +// This will be called before each test function is executed.
> +void tst_QDeclarativeWebView::init()
> +{
> +}
> +
> +// This will be called after every test function.
> +void tst_QDeclarativeWebView::cleanup()
> +{
> +}

Why add these if they do not test anything?

> Tools/Scripts/webkitpy/style/checker.py:127
> -	 "Tools/TestWebKitAPI/"],
> +	 "Tools/TestWebKitAPI/",
> +	 "Source/WebKit/qt/tests/qdeclarativewebview"],

This could be a separate patch.