Bug 50222 – [Qt] QML WebView inside a Flickable shows checkers pattern at startup

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Henry Haverinen]

This code snippet shows only checkers pattern until you move the view:

import Qt 4.7
import QtWebKit 1.0
Flickable {
    id: flick; width: 640; height: 400
    clip: true
    contentWidth: web.width; contentHeight: web.height
    WebView {
        id: web
        url: "http://qt.nokia.com" 
    }
}

If you add this workaround in the WebView, then it will render the page correctly:

        onLoadFinished: {
            flick.contentY += 1; 
        }

[Sriram]

COuld you please provide steps to reproduce this issue.

[Henry Haverinen]

Here's how you should be able to reproduce this:

1. Install Qt 4.7 on Windows or Mac
2. Save the code snippet above as testcase.qml
3. run qmlviewer testcase.qml

I've reproduced the problem on windows and Mac but it occurs on devices too.

An alternative is to create a little C++ application that embeds a QDeclarativeView for showing this problem. You could use the Qt Quick application wizard present in Qt Creator 2.1 beta and paste the code snipped as the main.qml.

[Sriram]

I am looking into this error right now. I am not able to reproduce the error locally on linux qmlviewer. What I see is that a checker pattern appears for a split second and then the actual page is loaded(all of this without onloadfinished()).

Checking to see if there is some refresh happening on linux that is not happening on other platforms.

[Sriram]

I discovered that this error does not happen if there is no http proxy settings and content is accessed via file:/// scheme.

With http proxy settings, I can see the error even for file:/// scheme.

Looking to see why having proxy settings is causing the issue.

[Gopal Raghavan]

Henry,

WebView needs preferredHeight and preferredWidth to be set. Once you set it you will not see checkers pattern.
for example:

Flickable {
    id: flick; width: 640; height: 400
    clip: true
    contentWidth: web.width; contentHeight: web.height
    WebView {
        id: web
        url: "http://qt.nokia.com" 
        preferredHeight: 400
        preferredWidth: 640
    }
}

Ideally, in the absense of preferredHeight/preferredWidth it would be nice to take parent dimensions.

What is the product requirement? 
To do fall back behavior or use mandatory preferred property.

Br,
--
Gopal

[Gopal Raghavan]

solution :
http://gitorious.org/~gopal/qt/gopal-qt

[Henry Haverinen]

According to the documentation (http://doc.qt.nokia.com/4.7-snapshot/qml-webview.html#details), the QML developer can do one of the following things

- set the width, in which case content will be clipped if it doesn't fit
- set the preferredWidth, in which case Qt will try to fit content in preferredWidth but if that doesn't work then the width of the WebView will be more
- not set either, in which case the size will be adjusted to a size appropriate for the content

So it wouldn't work to have the WebView's preferredWidth to default to the parent's width. After all, we're using Flickable in order to show content that is larger than the Flickable area on the screen.

[Gopal Raghavan]

Sure, agreed with all your three cases.
I guess, we are now concerned about the last case. 
With the solution I have proposed, you should not see any checker during initial load. Then when you flick, it will auto restreach to show all the content.

[Gopal Raghavan]

Created attachment 76210 [details]
patch to fix the checker box issue

Please find attached a patch for eliminating checkerbox in absense of preferredHeight and preferredWidth on declarative WebView.

[Benjamin Poulain]

Sorry, I was not on the computer when you asked on IRC.

If there are not autotest for the declarative web view, you should create the folder and make the test file.

I think this need autotest. You are enforcing certains behaviors in certains conditions. Nothing is currently tested, this is bad.

Not your fault obvisouly if tests are missing, but you can solve the issue by making some tests :)

[Suresh Voruganti]

Fix required for Qtwebkit 2.1

[Henry Haverinen]

Gopal, I don't think you can set the preferredWidth to the parent's width if it is not given. If width and preferredWidth are not set, then the WebView should choose a width that is appropriate for the _content_, which is not necessarily the width of the parent.

[Gopal Raghavan]

Created attachment 76908 [details]
Patch attached along with test case

I have attached couple of test cases. The patch resolves the issue.
These test cannot go in the autotest. We probably need to add a framework to test layout rendered through qmlviewer.
Currently, the test can be manually checked through qmlviewer.
I request you to review and move this patch forward.
Thanks,
--
Gopal

[Ademar Reis]

As discussed (in pvt) with Suresh, this should not block qtwebkit-2.1. Marking as a nice-to-have fix for qtwebkit-2.2 (later it can be triaged and promoted to blocker if necessary)

[Gopal Raghavan]

Can someone please review the patch?
Thanks,
--
Gopal

[Suresh Voruganti]

(In reply to comment #15)
> Can someone please review the patch?
> Thanks,
> --
> Gopal

Can someone pls review the patch?

[Gopal Raghavan]

could also solve: https://bugs.webkit.org/show_bug.cgi?id=48413

[Alexis Menard (darktears)]

Comment on attachment 76908 [details]
Patch attached along with test case

The patch is ok to me. For the auto-test why not using a private header (#include "qdeclarative_p.h") and create a test case in C++ like tst_qwebpage.cpp? You instanciate the qdeclarativewebview class and using QTestLib you test the preferredwidth and preferredHeight.

[Laszlo Gombos]

Comment on attachment 76908 [details]
Patch attached along with test case

Change the the plugin code looks ok to me.

r- as tests are not automated. I like Alexis proposal on how to write auto tests for this.

[Gopal Raghavan]

Created attachment 82986 [details]
Added auto test cases along with patch as per suggestion

[WebKit Review Bot]

Attachment 82986 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/qt/ChangeLog', u'Source/WebK..." exit_code: 1

Source/WebKit/qt/tests/qdeclarativewebview/tst_qdeclarativewebview.cpp:1:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gopal Raghavan]

Created attachment 83233 [details]
Update checker.py to fix the style-checker issue

Added auto test cases along with patch as per suggestion.
Updated checker.py to take care of style-checker issue.

[Early Warning System Bot]

Attachment 83233 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7938978

[Gopal Raghavan]

Created attachment 83323 [details]
Fixed build issues

Added auto test cases.
Updated style checker.
Fixed build issues.

[Kenneth Rohde Christiansen]

Comment on attachment 83323 [details]
Fixed build issues

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.

[Gopal Raghavan]

Created attachment 83470 [details]
Updated patch based on suggestion from reviewer

[Alexis Menard (darktears)]

*** Bug 48413 has been marked as a duplicate of this bug. ***

[Alexis Menard (darktears)]

Comment on attachment 83470 [details]
Updated patch based on suggestion from reviewer

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.

[Gopal Raghavan]

Created attachment 83691 [details]
Took care of comments from reviewer

Thanks Alexis for your input. I have fixed as per your suggestions.

[Alexis Menard (darktears)]

Comment on attachment 83691 [details]
Took care of comments from reviewer

View in context: https://bugs.webkit.org/attachment.cgi?id=83691&action=review

> Source/WebKit/qt/ChangeLog:10
> +

This patch fixes the checkerboard visible at startup even if preferredWidth and preferredHeight are not set.

seems a better changelog. I'm nitpicking :).

> Source/WebKit/qt/tests/util.h:37
> +

Why this extra line?

> Source/WebKit/qt/tests/qdeclarativewebview/tst_qdeclarativewebview.cpp:41
> +    delete wv;

You don't need those deletes. component.create(); return a pointer and indeed allocate memory BUT everything is deleted anyway when QDeclarativeComponent dies (at the end of the scope) and when the QDeclarativeEngine dies because the root element is parented to them. I'm sure it can cause problem of double deletion (valgrind can confirm) but I guess probably that use case is catched so that's why it works.

[Gopal Raghavan]

Created attachment 83699 [details]
Update as per suggestions

[Alexis Menard (darktears)]

Comment on attachment 83699 [details]
Update as per suggestions

Looks good to me. Need someone to land it :)

[Andreas Kling]

Comment on attachment 83699 [details]
Update as per suggestions

Clearing flags on attachment: 83699

Committed r79672: <http://trac.webkit.org/changeset/79672>

[Andreas Kling]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #33)
> (From update of attachment 83699 [details])
> Clearing flags on attachment: 83699
> 
> Committed r79672: <http://trac.webkit.org/changeset/79672>

It broke Qt Linux Release minimal, because it hasn't QDeclarative.

We shouldn't build tst_qdeclarativewebview if CONFIG contains qt_minimal.

[Alexis Menard (darktears)]

On my way to make the fix.

[Csaba Osztrogonác]

Fix landed in http://trac.webkit.org/changeset/79678

[Ademar Reis]

Revision r79672 cherry-picked into qtwebkit-2.1.x with commit c31f2f4 <http://gitorious.org/webkit/qtwebkit/commit/c31f2f4>
Revision r79678 cherry-picked into qtwebkit-2.1.x with commit 0a95892 <http://gitorious.org/webkit/qtwebkit/commit/0a95892>

[Simon Hausmann]

(In reply to comment #38)
> Revision r79672 cherry-picked into qtwebkit-2.1.x with commit c31f2f4 <http://gitorious.org/webkit/qtwebkit/commit/c31f2f4>
> Revision r79678 cherry-picked into qtwebkit-2.1.x with commit 0a95892 <http://gitorious.org/webkit/qtwebkit/commit/0a95892>

I think something went wrong with this cherry-pick, as c31f2f4 created Source/WebKit/qt/declarative/qdeclarativewebview.cpp instead of merging the changes into WebKit/qt/declarative/qdeclarativewebview.cpp

[Caio Marcelo de Oliveira Filho]

Thanks for catching that, Simon.

The change from the original patch is correctly applied in WebKit/qt/declarative/qdeclarativewebview.cpp. What happened is that the equivalent file in "Source/" was also mistakenly added to the commit.

I've just pushed 23a7339771ad724427cfce724430b0f898177b2e in the qtwebkit-2.1.x branch to remove that file. But since it was not used by the build system in that branch, it didn't cause any harm.