Bug 63945

Summary: [WK2] IconDatabase: Add a way to notify when icon data is available
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, beidson, mario, svillar, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on:    
Bug Blocks: 96476    
Attachments:
Description Flags
Patch proposal
none
Patch proposal andersca: review+

Description Carlos Garcia Campos 2011-07-05 10:04:33 PDT
Currently WebIconDatabaseClient uses a single callback, didChangeIconForPageURL(), called when the icon has changed in the database. WebIconDatabase uses that callback when the icon data is available too. The problem is that didImportIconDataForPageURL() is never called in WebKit2 when the icon is already in the database, because when imported the list of pages interested in icons is empty. In Webkit1 we use FrameLoaderClient::dispatchDidReceiveIcon(), called when the icon has just been loaded or when it was in the database and the data was already available, and FrameLoaderClient::registerForIconNotification() called when the icon was in the database but the data wasn't available yet. Both methods are currently unimplemented in WebKit2, but I'm not sure the same approach of WebKit1 is the right one for WebKit2.
Comment 1 Carlos Garcia Campos 2012-08-16 08:21:51 PDT
A possible solution would be to add a client callback to notify when the icon data is available (and imageForPageURL() will return a valid image) called from both didImportIconDataForPageURL() and setIconDataForIconURL()
Comment 2 Mario Sanchez Prada 2012-09-10 07:52:20 PDT
Created attachment 163129 [details]
Patch proposal

Attaching a patch proposal for this issue.

The current patch is what I'm using now in a local branch to implement the favicons API for WebKit2GTK+, and has been apparently working fine (at least as good as I would expect) for the last week.
Comment 3 WebKit Review Bot 2012-09-10 07:54:49 PDT
Attachment 163129 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:44:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Mario Sanchez Prada 2012-09-10 08:01:57 PDT
(In reply to comment #3)
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:44:  Alphabetical sorting problem.  [build/include_order] [4]
> Total errors found: 1 in 8 files

Oops! Sorry about this. I will fix it in a follow-up patch, but first I'd say it would be better to concentrate on reviewing the actual code :-)
Comment 5 Mario Sanchez Prada 2012-09-12 05:25:51 PDT
Created attachment 163598 [details]
Patch proposal

Uploading a new patch here fixing the code style issue.
Comment 6 Mario Sanchez Prada 2012-09-21 01:19:31 PDT
Ping reviewers?
Comment 7 Mario Sanchez Prada 2012-09-27 02:24:45 PDT
Committed r129742: <http://trac.webkit.org/changeset/129742>
Comment 8 Alexey Proskuryakov 2012-09-27 09:55:39 PDT
This broke nightly builds, bug 97784.
Comment 9 Mario Sanchez Prada 2012-09-27 12:24:05 PDT
Committed r129789: <http://trac.webkit.org/changeset/129789>