RESOLVED FIXED 65692
REGRESSION (r91540): Favicons are not loaded 
https://bugs.webkit.org/show_bug.cgi?id=65692
Summary REGRESSION (r91540): Favicons are not loaded
Daniel Bramhall
Reported 2011-08-04 09:16:34 PDT
Created attachment 102927 [details] Address Bar Favicon No favicons render or appear in either the address bar or the bookmarks menu (or in any part of the application for that matter). This did happen in an early beta preview of Safari and since then has been corrected although seems to have appeared in WebKit...
Attachments
Address Bar Favicon (11.67 KB, image/png)
2011-08-04 09:16 PDT, Daniel Bramhall 		no flags
Details
Bookmarks Menu View (19.45 KB, image/png)
2011-08-04 09:17 PDT, Daniel Bramhall 		no flags
Details
Bookmarks Window (31.54 KB, image/png)
2011-08-04 09:17 PDT, Daniel Bramhall 		no flags
Details
Move documentCanHaveIcon to WebCore's scope, so it won't need to be reimplemented. (3.93 KB, patch)
2011-08-06 04:00 PDT, Rafael Brandao 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Daniel Bramhall
Comment 1 2011-08-04 09:17:05 PDT
Created attachment 102928 [details] Bookmarks Menu View
Daniel Bramhall
Comment 2 2011-08-04 09:17:24 PDT
Created attachment 102929 [details] Bookmarks Window
Alexey Proskuryakov
Comment 3 2011-08-05 13:24:59 PDT
What I'm seeing is that favicons that Safari has already stored are shown successfully, but new ones won't load. Regression range: r91538 - r91541.
Alexey Proskuryakov
Comment 4 2011-08-05 13:27:19 PDT
<rdar://problem/9906147>
Adam Barth
Comment 5 2011-08-05 13:44:32 PDT
http://trac.webkit.org/changeset/91540 is pretty tiny. It should be easy to diagnose what's going wrong.
Rafael Brandao
Comment 6 2011-08-06 02:02:55 PDT
Finally figured out. On Webkit2, there's another implementation of IconDatabaseBase known as WebIconDatabaseProxy, but it doesn't reimplement documentCanHaveIcon, so by default it will reject all favicon loads. Working on a patch to fix this.
Rafael Brandao
Comment 7 2011-08-06 04:00:57 PDT
Created attachment 103146 [details] Move documentCanHaveIcon to WebCore's scope, so it won't need to be reimplemented. As explained before, this is just moving the function documentCanHaveIcon to somewhere else, visible to both IconController and IconDatabase, now without the need to add the same code for every IconDatabaseBase reimplementation.
WebKit Review Bot
Comment 8 2011-08-06 08:57:22 PDT
Comment on attachment 103146 [details] Move documentCanHaveIcon to WebCore's scope, so it won't need to be reimplemented. Clearing flags on attachment: 103146 Committed r92551: <http://trac.webkit.org/changeset/92551>
WebKit Review Bot
Comment 9 2011-08-06 08:57:27 PDT
All reviewed patches have been landed. Closing bug.
Adam Barth
Comment 10 2011-08-06 09:52:06 PDT
Comment on attachment 103146 [details] Move documentCanHaveIcon to WebCore's scope, so it won't need to be reimplemented. No test? If we can't test icon loading, we're going to keep breaking it. :(
Note You need to log in before you can comment on or make changes to this bug.