Bug 61042

Summary: [Qt] Fix tst_QDeclarativeWebView::basicProperties() and historyNav() autotests
Product: WebKit Reporter: Csaba Osztrogonác <ossy>
Component: Tools / TestsAssignee: Rafael Brandao <rafael.lobo>
Status: RESOLVED FIXED    
Severity: Normal CC: ademar, benjamin, cmarcelo, commit-queue, menard, ossy, rafael.lobo, webkit.review.bot
Priority: P2 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 38654    
Attachments:
Description Flags
mark failing test cases as expected fail
none
Forces icon to be loaded on each test by setting a temporary path for them, and making test to wait for the iconChange signal.
menard: review-
Same as before, but without the unrelated patch and the annoying ::
benjamin: commit-queue-
remove mark as expected fail where XPASS and mark failing test cases as expected fail
none
Following kling's advices, and solving some issues around comparison of bitmaps in this test. none

Description Csaba Osztrogonác 2011-05-18 05:06:59 PDT
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)]
Comment 1 Csaba Osztrogonác 2011-05-20 06:11:43 PDT
*** Bug 61043 has been marked as a duplicate of this bug. ***
Comment 2 Csaba Osztrogonác 2011-05-20 06:15:31 PDT
Created attachment 94205 [details]
mark failing test cases as expected fail
Comment 3 WebKit Commit Bot 2011-05-20 07:27:59 PDT
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>
Comment 4 WebKit Commit Bot 2011-05-20 07:28:03 PDT
All reviewed patches have been landed.  Closing bug.
Comment 5 Csaba Osztrogonác 2011-05-20 07:28:41 PDT
Reopen to fix the bug.
Comment 6 Ademar Reis 2011-06-01 12:32:44 PDT
Revision r86952 cherry-picked into qtwebkit-2.2 with commit fbde563 <http://gitorious.org/webkit/qtwebkit/commit/fbde563>
Comment 7 Rafael Brandao 2011-06-09 13:16:19 PDT
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.
Comment 8 Alexis Menard (darktears) 2011-06-13 06:16:21 PDT
(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.
Comment 9 Rafael Brandao 2011-06-16 14:05:38 PDT
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.
Comment 10 Alexis Menard (darktears) 2011-06-16 14:13:05 PDT
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 ::?
Comment 11 Rafael Brandao 2011-06-16 14:51:07 PDT
(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. :/
Comment 12 Rafael Brandao 2011-06-16 16:26:14 PDT
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
Comment 13 Andreas Kling 2011-06-28 11:43:02 PDT
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? ;(
Comment 14 Benjamin Poulain 2011-07-09 06:20:57 PDT
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);" ???
Comment 15 Rafael Brandao 2011-07-22 07:12:21 PDT
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! :)
Comment 16 Kristóf Kosztyó 2011-08-02 06:25:48 PDT
Created attachment 102642 [details]
remove mark as expected fail where XPASS and mark failing test cases as expected fail
Comment 17 Csaba Osztrogonác 2011-08-02 06:50:19 PDT
Comment on attachment 102642 [details]
remove mark as expected fail where XPASS and mark failing test cases as expected fail

r=me
Comment 18 Csaba Osztrogonác 2011-08-02 06:59:31 PDT
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
Comment 19 Rafael Brandao 2011-08-06 05:06:24 PDT
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. :)
Comment 20 Simon Hausmann 2011-11-03 13:24:53 PDT
Comment on attachment 103147 [details]
Following kling's advices, and solving some issues around comparison of bitmaps in this test.

r=me
Comment 21 WebKit Review Bot 2011-11-03 13:37:35 PDT
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>
Comment 22 WebKit Review Bot 2011-11-03 13:37:45 PDT
All reviewed patches have been landed.  Closing bug.