Bug 65692

Summary:

REGRESSION (r91540): Favicons are not loaded

Product:

WebKit

Reporter:

Daniel Bramhall <dbramhall>

Component:

WebCore Misc.

Assignee:

Rafael Brandao <rafael.lobo>

Status:

RESOLVED FIXED

Severity:

Critical

CC:

abarth, ap, darin, rafael.lobo, webkit.review.bot

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

Mac

OS:

OS X 10.7

URL:

All webpages

Attachments:

Description	Flags
Address Bar Favicon	none
Bookmarks Menu View	none
Bookmarks Window	none
Move documentCanHaveIcon to WebCore's scope, so it won't need to be reimplemented.	none

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