Bug 65692 - REGRESSION (r91540): Favicons are not loaded
Summary: REGRESSION (r91540): Favicons are not loaded
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.7
: P1 Critical
Assignee: Rafael Brandao
URL: All webpages
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-08-04 09:16 PDT by Daniel Bramhall
Modified: 2011-08-06 09:52 PDT (History)
5 users (show)

See Also:


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 Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bramhall 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...
Comment 1 Daniel Bramhall 2011-08-04 09:17:05 PDT
Created attachment 102928 [details]
Bookmarks Menu View
Comment 2 Daniel Bramhall 2011-08-04 09:17:24 PDT
Created attachment 102929 [details]
Bookmarks Window
Comment 3 Alexey Proskuryakov 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.
Comment 4 Alexey Proskuryakov 2011-08-05 13:27:19 PDT
<rdar://problem/9906147>
Comment 5 Adam Barth 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.
Comment 6 Rafael Brandao 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.
Comment 7 Rafael Brandao 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.
Comment 8 WebKit Review Bot 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>
Comment 9 WebKit Review Bot 2011-08-06 08:57:27 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Adam Barth 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.  :(