Patch coming to skip this test for now.
Created attachment 34298 [details] Add to Skipped list
Comment on attachment 34298 [details] Add to Skipped list Landed skip update in r46902.
There seems to be a problem with the handling of ico files with multiple entries. I'm working on a solution for this.
Here is a patch for qt which corrects the layout test, but the imlementation should be changed higher up in QIcon and QPixmap. This is only an expected behaviour change, which is more compilant to the ICO file format description (http://www.daubnet.com/en/file-format-ico) which says: "For general purpose ICOs there should be at least one 32x32 image using the 16 Windows colors." diff --git a/src/plugins/imageformats/ico/qicohandler.cpp b/src/plugins/imageformats/ico/qicohandler.cpp index c9e04a4..77c68b7 100644 --- a/src/plugins/imageformats/ico/qicohandler.cpp +++ b/src/plugins/imageformats/ico/qicohandler.cpp @@ -802,7 +802,25 @@ bool QtIcoHandler::canRead(QIODevice *device) bool QtIcoHandler::read(QImage *image) { bool bSuccess = false; - QImage img = m_pICOReader->iconAt(m_currentIconIndex); + + QIODevice *d = device(); + + QList<QImage> imgs = m_pICOReader->read(d); + QImage img = imgs[m_currentIconIndex]; + QSize requestedSize(32,32); + + if ( imgs.size() > 1 ){ + + // if we have multiple icons in the file we return the first with 32x32 resolution because it is the reference size + // FIXME: there should be a proper way of handling all the files in an ICO file higher up in the abstraction + // ie. to fill the caller QIcon with the icons in the ICO file automatically, and let the user decide which to use + for ( int i=0; i<imgs.size(); ++i ) + if ( imgs[i].size() == requestedSize ){ + img = imgs[i]; + break; + } + + } // Make sure we only write to \a image when we succeed. if (!img.isNull()) {
(In reply to comment #4) > Here is a patch for qt which corrects the layout test, but the imlementation > should be changed higher up in QIcon and QPixmap. This is only an expected > behaviour change, which is more compilant to the ICO file format description > (http://www.daubnet.com/en/file-format-ico) which says: > > "For general purpose ICOs there should be at least one 32x32 image using the 16 > Windows colors." That sentence is advisory, to note that if you only include smaller sizes, Windows has to scale them up, and they look bad. However, 32x32 is not a "reference" size, and the first such entry found is not necessarily the best. The layout test is testing that the decoder selects the icon entries in order of largest first, then highest bit depth first after that. This is similar to how Windows itself behaves, especially on newer versions like Vista and 7, where the "typical" icon size is significantly larger than 32x32. Note that for some applications (e.g. displaying favicons in a tab or session history list), you probably want to be able to select a 16x16 icon.
> The layout test is testing that the decoder selects the icon entries in order > of largest first, then highest bit depth first after that. This is similar to > how Windows itself behaves, especially on newer versions like Vista and 7, > where the "typical" icon size is significantly larger than 32x32. > > Note that for some applications (e.g. displaying favicons in a tab or session > history list), you probably want to be able to select a 16x16 icon. Thank you very much for the information! I din't know which is the preferred order of selecting the icons, how ico exactly has to be handled. I will look into that and correct my patch. According to that the behaviour of the Qt infrastructure is totally wrong, because only the 1st element in the ico file is handed upward to create a QIcon or QPixmap or QImage, and no feature is considered. The only way to handle ICO files correctly is to use QImageLoader an specify how to select the icon. In my opinion at least QIcon sould automatically fill himself with all the icons contained in the ico file, and then select which to retunrt on QIcon::pixmap(...) calls, according to the features and the given arguments. Do you agree? WebKit converts the images to NativeImagePtr, which is QPixmap* in case of QtWebKit if I am not mistaken, so the information that it was an ico and had more entries is lost. I could write a patch to work around the test in QtWebKit, and to specifically handle ico files, but I think this is not a QtWebKit bug.
(In reply to comment #6) > According to that the behaviour of the Qt infrastructure is totally wrong, > because only the 1st element in the ico file is handed upward to create a QIcon > or QPixmap or QImage, and no feature is considered. If you're only going to select one image, it should probably be the largest (and highest bit depth). > In my > opinion at least QIcon sould automatically fill himself with all the icons > contained in the ico file, and then select which to retunrt on > QIcon::pixmap(...) calls, according to the features and the given arguments. Do > you agree? I am not familiar with the Qt API, so I can't comment usefully. In the abstract, that sounds correct :) > WebKit converts the images to NativeImagePtr, which is QPixmap* in case of > QtWebKit if I am not mistaken, so the information that it was an ico and had > more entries is lost. Things are only converted to a NativeImagePtr at the time when a client requests a frame at a particular index. But an outside client can query ImageSource::frameCount() for the number of frames in an image, and then use ImageSource::frameSizeAtIndex() to determine which frame it wants before requesting the actual frame data. Making this work depends on having your ImageDecoder return the right values for these calls. If your icon decoder sorts the directory entries by size, and bit depth within size, with the highest quality image first, then callers who blindly grab the first frame will get the highest-quality icon (which is what this layout test is testing), and callers who want to pick an icon can use the functions described above.
I have changed my patch according to pkasting's explanation. The patch can be found here: http://pastebin.com/f293c5c82
(In reply to comment #8) > I have changed my patch according to pkasting's explanation. The patch can be > found here: http://pastebin.com/f293c5c82 Could you please submit this patch as a merge request at http://qt.gitorious.org if you haven't already? Additionally you could create a report at http://bugreports.qt.nokia.com and link to it from here, so the dependency becomes explicit. Thanks.
(In reply to comment #9) > Could you please submit this patch as a merge request at http://qt.gitorious.org if you haven't already? > Additionally you could create a report at http://bugreports.qt.nokia.com and link to it from here, so the dependency becomes explicit. > Thanks. Was this done?
(In reply to comment #10) > (In reply to comment #9) > > Could you please submit this patch as a merge request at http://qt.gitorious.org if you haven't already? > > Additionally you could create a report at http://bugreports.qt.nokia.com and link to it from here, so the dependency becomes explicit. > > Thanks. > > Was this done? I had a merge request for this on gitorious but there was ongoing work by Milan Burda on Qt image loader and icon handling already, apparently he wanted this done differently so I didn't create a bugreport. Seems that the issue wasn't resolved, I'll recheck.
=== Bulk closing of Qt bugs === If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary. If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.