Bug 85882 – [Qt] Remove ASSERT from QWebIconImageProvider::requestImage

• Component: New Bugs
• Status: RESOLVED FIXED

[Zoltan Horvath]

[Qt] Remove ASSERT from QWebIconImageProvider::requestImage

[Zoltan Horvath]

Created attachment 140707 [details]
Patch

[Zoltan Horvath]

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.

[Rafael Brandao]

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

[Zoltan Horvath]

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

[Tor Arne Vestbø]

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

[Simon Hausmann]

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.

[Tor Arne Vestbø]

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