Bug 85882

Created attachment 140707 [details] Patch

This assertion hits if you use MiniBrowser with my webcore imagedecoder patch from bug #80400. QtTestBrowser works well, since WK1 doesn't have this assertion.

Comment on attachment 140707 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140707&action=review > Source/WebKit2/UIProcess/API/qt/qwebiconimageprovider.cpp:-60 > - ASSERT(!icon.isNull()); Actually the reason you're hitting this assert is not because the page doesn't have an icon but a known issue on conversion between QUrl and QString. In some cases we can't revert it back to the original value, so even after changing the icon url of a web view confirming that its icon is there, you cannot request it back at image provider level. :-/ How frequent are you hitting this assert? You should get into it only on some tricky urls with special characters. Right now, I don't think removing is a good idea. Maybe with Thiago recent QUrl changes we will have this fixed (or detect when it still need to be worked on).

(In reply to comment #3) > (From update of attachment 140707 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=140707&action=review > > > Source/WebKit2/UIProcess/API/qt/qwebiconimageprovider.cpp:-60 > > - ASSERT(!icon.isNull()); > > Actually the reason you're hitting this assert is not because the page doesn't have an icon but a known issue on conversion between QUrl and QString. In some cases we can't revert it back to the original value, so even after changing the icon url of a web view confirming that its icon is there, you cannot request it back at image provider level. :-/ Thus you don't have a valid icon at this point (and you don't always need). What is the reason for the assert? Using a null icon is a valid state here, then why do we demand for not null icon? > How frequent are you hitting this assert? You should get into it only on some tricky urls with special characters. Right now, I don't think removing is a good idea. Maybe with Thiago recent QUrl changes we will have this fixed (or detect when it still need to be worked on). It's hitting by simple URLs frequently also, just clicking on the menus of google.com.

This should be fixed now that we base64 the page-url of the image-provider url, since r118762

Comment on attachment 140707 [details] Patch Bug is marked as resolved/fixed, so clearing review on this patch to take it out of the review queue.

(In reply to comment #6) > (From update of attachment 140707 [details]) > Bug is marked as resolved/fixed, so clearing review on this patch to take it out of the review queue. Would be nice if the review queue was a bit smarter than that :)