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+

Carlos Garcia Campos
Reported 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.
Attachments
Patch proposal (11.45 KB, patch)
2012-09-10 07:52 PDT, Mario Sanchez Prada 		no flags
DetailsFormatted DiffDiff
Patch proposal (11.46 KB, patch)
2012-09-12 05:25 PDT, Mario Sanchez Prada 		andersca: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Carlos Garcia Campos
Comment 1 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()
Mario Sanchez Prada
Comment 2 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.
WebKit Review Bot
Comment 3 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.
Mario Sanchez Prada
Comment 4 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 :-)
Mario Sanchez Prada
Comment 5 2012-09-12 05:25:51 PDT
Created attachment 163598 [details] Patch proposal Uploading a new patch here fixing the code style issue.
Mario Sanchez Prada
Comment 6 2012-09-21 01:19:31 PDT
Ping reviewers?
Mario Sanchez Prada
Comment 7 2012-09-27 02:24:45 PDT
Committed r129742: <http://trac.webkit.org/changeset/129742>
Alexey Proskuryakov
Comment 8 2012-09-27 09:55:39 PDT
This broke nightly builds, bug 97784.
Mario Sanchez Prada
Comment 9 2012-09-27 12:24:05 PDT
Committed r129789: <http://trac.webkit.org/changeset/129789>
Note You need to log in before you can comment on or make changes to this bug.