Bug 61042 – [Qt] Fix tst_QDeclarativeWebView::basicProperties() and historyNav() autotests

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Csaba Osztrogonác]

Error message with r86748:

FAIL!  : tst_QDeclarativeWebView::basicProperties() Compared values are not the same
   Actual (qvariant_cast<QPixmap>(wv->property("icon")).width()): 16
   Expected (48): 48
   Loc: [/ramdisk/qt-linux-release/build/Source/WebKit/qt/tests/qdeclarativewebview/tst_qdeclarativewebview.cpp(104)]

[Csaba Osztrogonác]

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

[Csaba Osztrogonác]

Created attachment 94205 [details]
mark failing test cases as expected fail

[WebKit Commit Bot]

Comment on attachment 94205 [details]
mark failing test cases as expected fail

Clearing flags on attachment: 94205

Committed r86952: <http://trac.webkit.org/changeset/86952>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

Reopen to fix the bug.

[Ademar Reis]

Revision r86952 cherry-picked into qtwebkit-2.2 with commit fbde563 <http://gitorious.org/webkit/qtwebkit/commit/fbde563>

[Rafael Brandao]

I think I've almost solved this.

First, this test should expect the iconChanged signal before checking icon size. And then we also have a problem loading the icon for local files.

The problem lies on IconDatabase. It is defined there this static function called pageCanHaveIcon, and it only accepts pages whose protocol is part of http family (http or https), so if we load a local file, our page cannot have an icon, and then a default icon is loaded instead (whose size is 16x16), thus causing the test fail.

This function was added at r85056, but the patch does not describe very well why this decision was made, so I've just tried to contact the author and ask more about it. 

I've also added to the test QWebSettings::setIconDatabasePath, so I could clean the file where the icon is stored after the test, forcing the page to always load the icon.

[Alexis Menard (darktears)]

(In reply to comment #7)
> I think I've almost solved this.
> 
> First, this test should expect the iconChanged signal before checking icon size. And then we also have a problem loading the icon for local files.
> 
> The problem lies on IconDatabase. It is defined there this static function called pageCanHaveIcon, and it only accepts pages whose protocol is part of http family (http or https), so if we load a local file, our page cannot have an icon, and then a default icon is loaded instead (whose size is 16x16), thus causing the test fail.
> 
> This function was added at r85056, but the patch does not describe very well why this decision was made, so I've just tried to contact the author and ask more about it. 
> 
> I've also added to the test QWebSettings::setIconDatabasePath, so I could clean the file where the icon is stored after the test, forcing the page to always load the icon.

Send the patch for review :D.

[Rafael Brandao]

Created attachment 97491 [details]
Forces icon to be loaded on each test by setting a temporary path for them, and making test to wait for the iconChange signal.

The change on IconDatabase is still being discussed (https://bugs.webkit.org/show_bug.cgi?id=62459), but I'll have to add a layout test for it anyway, so I don't expect this to be landed. I would like it to be reviewed on my changes related to tst_qdeclarativewebview, because as soon as I get it working on webkit, this could be easily landed.

[Alexis Menard (darktears)]

Comment on attachment 97491 [details]
Forces icon to be loaded on each test by setting a temporary path for them, and making test to wait for the iconChange signal.

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

You posted an unrelated patch :D.

> Source/WebKit/qt/tests/qdeclarativewebview/tst_qdeclarativewebview.cpp:-165
> -    QWebSettings::enablePersistentStorage(tmpDir());

Why this move?

> Source/WebKit/qt/tests/qdeclarativewebview/tst_qdeclarativewebview.cpp:172
> +    ::waitForSignal(wv, SIGNAL(iconChanged()), 15000);

By curiosity why ::?

[Rafael Brandao]

(In reply to comment #10)
> (From update of attachment 97491 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97491&action=review
> 
> You posted an unrelated patch :D.
> 
> > Source/WebKit/qt/tests/qdeclarativewebview/tst_qdeclarativewebview.cpp:-165
> > -    QWebSettings::enablePersistentStorage(tmpDir());
> 
> Why this move?

When I added QWebSettings::setIconDatabasePath in the beginning of the test, it felt like the another one was a bit late already, because it is called just after creating the QDeclarativeComponent. Thus, it seems more logical to me that we should adjust settings before doing (or possibly doing) anything on the test.

> 
> > Source/WebKit/qt/tests/qdeclarativewebview/tst_qdeclarativewebview.cpp:172
> > +    ::waitForSignal(wv, SIGNAL(iconChanged()), 15000);
> 
> By curiosity why ::?

Nevermind... I've found that being used that way so many times on tst_qwebpage before, that it felt like it was the preferred way to use it. :/

[Rafael Brandao]

Created attachment 97518 [details]
Same as before, but without the unrelated patch and the annoying ::

If changing the order of QWebSettings is not a problem, I'm confident this will fix the test fail. But as I said in the previous comment, this is not supposed to land until the other bug is fixed.

I should add that I've modified the QTRY_COMPARE to COMPARE on those cases because I already wait 15 seconds for the icon to change, so if I get to the check and the icon is not already there, then it means something went wrong, and testing it more times may be pointless. If this is a problem I could put QTRY_COMPARE again.

Thanks for the feedback! :D

[Andreas Kling]

Comment on attachment 97518 [details]
Same as before, but without the unrelated patch and the annoying ::

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

> Source/WebKit/qt/tests/qdeclarativewebview/tst_qdeclarativewebview.cpp:96
> +    QWebSettings::setIconDatabasePath(tmpDir());
> +    QWebSettings::enablePersistentStorage(tmpDir());

Unless you actually need to redo this multiple times, these should go in initTestCase() instead.

> Source/WebKit/qt/tests/qdeclarativewebview/tst_qdeclarativewebview.cpp:103
> +    waitForSignal(wv, SIGNAL(iconChanged()), 15000);

Any way we can avoid the 15 second timeout? ;(

[Benjamin Poulain]

Comment on attachment 97518 [details]
Same as before, but without the unrelated patch and the annoying ::

The patch makes sense. Please fix the code from Andreas' comments
Wha about the others "QEXPECT_FAIL("", "'icon' property isn't working", Continue);" ???

[Rafael Brandao]

Thanks Benjamin, I really forgot to check the other XFAIL that was already there. I could fix the ones that complain about the 'icon' property. But it has been a bit of time that I didn't come back to this, and now I see that was other tests failing there as well. I've filed another bug report (https://bugs.webkit.org/show_bug.cgi?id=64989) and I wonder if I should wait to get it resolved first. If you're around, feel free to look into it as well! :)

[Kristóf Kosztyó]

Created attachment 102642 [details]
remove mark as expected fail where XPASS and mark failing test cases as expected fail

[Csaba Osztrogonác]

Comment on attachment 102642 [details]
remove mark as expected fail where XPASS and mark failing test cases as expected fail

r=me

[Csaba Osztrogonác]

Comment on attachment 102642 [details]
remove mark as expected fail where XPASS and mark failing test cases as expected fail

landed in https://trac.webkit.org/changeset/92186

[Rafael Brandao]

Created attachment 103147 [details]
Following kling's advices, and solving some issues around comparison of bitmaps in this test.

According to Qt doc, instead of "qrc://", for a QDir you must use ":/". As QPixmap was expecting a file path, it wouldn't resolve the file with the url format. As the icon and this not retrieved image were both empty QPixmaps, the test was presenting a false positive.

For the issues around QActions on historyNav() test case, there is a small patch pending for review on https://bugs.webkit.org/show_bug.cgi?id=64989 with a quick solution. If anyone else wants to investigate those errors, go ahead. :)

[Simon Hausmann]

Comment on attachment 103147 [details]
Following kling's advices, and solving some issues around comparison of bitmaps in this test.

r=me

[WebKit Review Bot]

Comment on attachment 103147 [details]
Following kling's advices, and solving some issues around comparison of bitmaps in this test.

Clearing flags on attachment: 103147

Committed r99235: <http://trac.webkit.org/changeset/99235>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.