Bug 50222

Summary: [Qt] QML WebView inside a Flickable shows checkers pattern at startup
Product: WebKit Reporter: Henry Haverinen <henry.haverinen>
Component: Layout and RenderingAssignee: Gopal Raghavan <gopal.1.raghavan>
Severity: Normal CC: ademar, benjamin, cmarcelo, gopal.1.raghavan, hausmann, henrik.hartz, kevin.simons, kling, laszlo.gombos, menard, ossy, sriram.seetharam, suresh.voruganti, webkit-ews, webkit.review.bot
Priority: P2 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Description Flags
patch to fix the checker box issue
Patch attached along with test case
laszlo.gombos: review-
Added auto test cases along with patch as per suggestion
Update checker.py to fix the style-checker issue
Fixed build issues
kenneth: review-
Updated patch based on suggestion from reviewer
menard: review-, menard: commit-queue-
Took care of comments from reviewer
menard: review-, menard: commit-queue-
Update as per suggestions none

Description Henry Haverinen 2010-11-30 03:45:13 PST
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; 
Comment 1 Sriram 2010-12-01 07:28:57 PST
COuld you please provide steps to reproduce this issue.
Comment 2 Henry Haverinen 2010-12-02 03:36:51 PST
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.
Comment 3 Sriram 2010-12-08 07:03:01 PST
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.
Comment 4 Sriram 2010-12-08 10:36:01 PST
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.
Comment 5 Gopal Raghavan 2010-12-09 12:31:19 PST

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.

Comment 6 Gopal Raghavan 2010-12-09 16:05:43 PST
solution :
Comment 7 Henry Haverinen 2010-12-10 01:30:10 PST
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.
Comment 8 Gopal Raghavan 2010-12-10 05:42:03 PST
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.
Comment 9 Gopal Raghavan 2010-12-10 09:03:42 PST
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.
Comment 10 Benjamin Poulain 2010-12-10 10:51:02 PST
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 :)
Comment 11 Suresh Voruganti 2010-12-10 12:00:36 PST
Fix required for Qtwebkit 2.1
Comment 12 Henry Haverinen 2010-12-13 00:22:30 PST
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.
Comment 13 Gopal Raghavan 2010-12-17 14:19:10 PST
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.
Comment 14 Ademar Reis 2010-12-28 13:38:07 PST
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)
Comment 15 Gopal Raghavan 2011-01-04 08:28:20 PST
Can someone please review the patch?
Comment 16 Suresh Voruganti 2011-01-10 08:06:16 PST
(In reply to comment #15)
> Can someone please review the patch?
> Thanks,
> --
> Gopal

Can someone pls review the patch?
Comment 17 Gopal Raghavan 2011-01-20 13:21:52 PST
could also solve: https://bugs.webkit.org/show_bug.cgi?id=48413
Comment 18 Alexis Menard (darktears) 2011-02-09 09:07:22 PST
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.
Comment 19 Laszlo Gombos 2011-02-09 09:20:59 PST
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.
Comment 20 Gopal Raghavan 2011-02-18 11:34:10 PST
Created attachment 82986 [details]
Added auto test cases along with patch as per suggestion
Comment 21 WebKit Review Bot 2011-02-18 11:37:53 PST
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.
Comment 22 Gopal Raghavan 2011-02-21 16:03:05 PST
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.
Comment 23 Early Warning System Bot 2011-02-21 18:28:18 PST
Attachment 83233 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7938978
Comment 24 Gopal Raghavan 2011-02-22 08:47:52 PST
Created attachment 83323 [details]
Fixed build issues

Added auto test cases.
Updated style checker.
Fixed build issues.
Comment 25 Kenneth Rohde Christiansen 2011-02-22 10:24:15 PST
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.
Comment 26 Gopal Raghavan 2011-02-23 04:18:01 PST
Created attachment 83470 [details]
Updated patch based on suggestion from reviewer
Comment 27 Alexis Menard (darktears) 2011-02-24 08:53:35 PST
*** Bug 48413 has been marked as a duplicate of this bug. ***
Comment 28 Alexis Menard (darktears) 2011-02-24 09:02:49 PST
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.
Comment 29 Gopal Raghavan 2011-02-24 11:44:26 PST
Created attachment 83691 [details]
Took care of comments from reviewer

Thanks Alexis for your input. I have fixed as per your suggestions.
Comment 30 Alexis Menard (darktears) 2011-02-24 11:58:48 PST
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.
Comment 31 Gopal Raghavan 2011-02-24 12:28:15 PST
Created attachment 83699 [details]
Update as per suggestions
Comment 32 Alexis Menard (darktears) 2011-02-24 12:32:39 PST
Comment on attachment 83699 [details]
Update as per suggestions

Looks good to me. Need someone to land it :)
Comment 33 Andreas Kling 2011-02-25 04:48:19 PST
Comment on attachment 83699 [details]
Update as per suggestions

Clearing flags on attachment: 83699

Committed r79672: <http://trac.webkit.org/changeset/79672>
Comment 34 Andreas Kling 2011-02-25 04:48:34 PST
All reviewed patches have been landed.  Closing bug.
Comment 35 Csaba Osztrogon√°c 2011-02-25 05:22:44 PST
(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.
Comment 36 Alexis Menard (darktears) 2011-02-25 05:32:16 PST
On my way to make the fix.
Comment 37 Csaba Osztrogon√°c 2011-02-25 05:54:56 PST
Fix landed in http://trac.webkit.org/changeset/79678
Comment 38 Ademar Reis 2011-03-03 11:08:38 PST
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>
Comment 39 Simon Hausmann 2011-03-17 15:48:44 PDT
(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
Comment 40 Caio Marcelo de Oliveira Filho 2011-03-21 12:16:41 PDT
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.