Bug 61042

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

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

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>

All reviewed patches have been landed. Closing bug.

Reopen to fix the bug.

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

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.

(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.

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 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 ::?

(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. :/

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 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 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);" ???

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! :)

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

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

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

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 on attachment 103147 [details] Following kling's advices, and solving some issues around comparison of bitmaps in this test. r=me

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>

All reviewed patches have been landed. Closing bug.