Bug 63945 - [WK2] IconDatabase: Add a way to notify when icon data is available
Summary: [WK2] IconDatabase: Add a way to notify when icon data is available
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 96476
  Show dependency treegraph
 
Reported: 2011-07-05 10:04 PDT by Carlos Garcia Campos
Modified: 2012-09-27 12:24 PDT (History)
5 users (show)

See Also:


Attachments
Patch proposal (11.45 KB, patch)
2012-09-10 07:52 PDT, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
Patch proposal (11.46 KB, patch)
2012-09-12 05:25 PDT, Mario Sanchez Prada
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>