Bug 16296

Summary: -[WebFrameLoadDelegate didReceiveIcon:forFrame:] never called?
Product: WebKit Reporter: Todd Ditchendorf <itod>
Component: WebKit APIAssignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Major CC: itod, mrowe
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
URL: http://ditchnet.org/WebKitFaviconTest.zip
Attachments:
Description Flags
test case
none
patch beidson: review+

Description Todd Ditchendorf 2007-12-04 13:23:48 PST
System: 10.5.1, MacBook 2.2GHz Intel Core 2 Duo

I have a simple (and also a not-so-simple) cocoa app with a WebView and a controller object that is set as the WebFrameLoadDelegate for the WebView. All of the WebFrameLoadDelegate methods seem to be called just fine except for one:

-[WebFrameLoadDelegate didReceiveIcon:forFrame:]

This is not called even when I'm visiting a page that I know has a favicon like google.com, msn.com or apple.com.
I see no mention of this issue in WebKit Bugzilla. Google, does turn up a few mentions of this issue over the years on the web, but there's no apparent resolution.

I did see a suggestion to fix the issue along these lines:

	NSString *dbDir = [@"~/Library/Icons" stringByExpandingTildeInPath];
	[[NSUserDefaults standardUserDefaults] setObject:dbDir forKey:WebIconDatabaseDirectoryDefaultsKey];

but that does not have any effect on my simple test case.

Also, I have tried downloading the WebKit nightly and linking my test case against the WebKit.framework found therein. this does not fix the issue for me either.

My very simple test case/Xcode project that displays the problem is located here:

http://ditchnet.org/WebKitFaviconTest.zip
Comment 1 Brady Eidson 2007-12-04 13:53:19 PST
I tried to run your favicon test, but it doesn't seem to be working:
2007-12-04 13:45:56.517 WebKitFaviconTest[43824:813] init
2007-12-04 13:45:56.521 WebKitFaviconTest[43824:813] *** -[NSKeyedUnarchiver decodeObjectForKey:]: cannot decode object of class (WebView)

I checked a few bugs in radar and found this issue had been reported awhile ago but closed as works, fixed in September by a related change.

Using a debug build of WebKit, I just set a breakpoint in _dispatchDidReceiveIconFromWebFrame in WebView.mm  (which is where the delegate call is sent from) and visited a site I knew I didn't have the icon for, and the breakpoint was hit immediately.
Comment 2 Todd Ditchendorf 2007-12-04 18:11:33 PST
Hi Brady, thanks for trying, however, I still see the issue.

First, I have updated the test case to try to fix the apparent linking problems you were experiencing. This test runs on both of my macbooks, so pretty sure it's cool now. I've also attached the test case to this issue directly.

What issue from september are you referring to?

thanks!
Comment 3 Todd Ditchendorf 2007-12-04 18:12:16 PST
Created attachment 17711 [details]
test case
Comment 4 Brady Eidson 2007-12-04 22:45:01 PST
Okay, I see what the problem is.  The icon database - and therefore all icon loading - is disabled by default.

Since icon loading is disabled, you'll never get this notification.

Calling [WebIconDatabase sharedIconDatabase] is enough to instantiate the global icon database, enable it, and result in icon loads.  Slapping that one-liner into your - (void)setupIconDatabase; method made it work.

It really sucks that the only way to enable part of the API is to use SPI (granted, very stable SPI that's been around forever) but the alternative is to have the overhead of icon loads and multithreading for *all* WebKit apps, which is entirely unacceptable.

I'm at a loss as to how to resolve this without changes to the API (or at least the documentation of the API).
Comment 5 Darin Adler 2007-12-07 10:33:22 PST
As we discussed, this will be simple to fix. If you implement the icon delegate method, then WebKit should call [WebIconDatabase sharedIconDatabase] for you when you attach that delegate to a WebView.
Comment 6 Mark Rowe (bdash) 2007-12-07 10:33:49 PST
<rdar://problem/5635641>
Comment 7 Darin Adler 2007-12-11 10:36:35 PST
Created attachment 17850 [details]
patch
Comment 8 Brady Eidson 2007-12-11 15:53:31 PST
Comment on attachment 17850 [details]
patch

r=me
Comment 9 Darin Adler 2007-12-14 12:30:29 PST
r28718