Bug 32334 - [GTK] Should provide an API to control the IconDatabase
Summary: [GTK] Should provide an API to control the IconDatabase
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2009-12-09 11:30 PST by Gustavo Noronha (kov)
Modified: 2009-12-14 05:27 PST (History)
5 users (show)

See Also:


Attachments
first step (8.51 KB, patch)
2009-12-09 11:34 PST, Gustavo Noronha (kov)
gns: commit-queue-
Details | Formatted Diff | Diff
Epiphany patch using this API (7.32 KB, patch)
2009-12-09 11:38 PST, Gustavo Noronha (kov)
no flags Details | Formatted Diff | Diff
second try (16.18 KB, patch)
2009-12-10 05:57 PST, Gustavo Noronha (kov)
gns: commit-queue-
Details | Formatted Diff | Diff
real second try (16.50 KB, patch)
2009-12-10 06:01 PST, Gustavo Noronha (kov)
xan.lopez: review+
gns: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gustavo Noronha (kov) 2009-12-09 11:30:12 PST
WebKit has a full-blown icon database. We should probably take advantage of it, and implement a nice API on top of it. For now, I would like to at least allow applications to be notified of icon URLs.
Comment 1 Gustavo Noronha (kov) 2009-12-09 11:34:30 PST
Created attachment 44549 [details]
first step

There are some caveats (the full blown icon database is enabled, but only a tiny bit is being used), but this allows us to move forward with implementing favicons, and is a first step towards adopting the internal icon database.
Comment 2 WebKit Review Bot 2009-12-09 11:37:40 PST
Attachment 44549 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/gtk/webkit/webkitwebview.cpp:2398:  Use 0 instead of NULL.  [readability/null] [5]
WebKit/gtk/webkit/webkitwebview.cpp:4003:  Use 0 instead of NULL.  [readability/null] [5]
WebKit/gtk/webkit/webkitprivate.cpp:262:  Use 0 instead of NULL.  [readability/null] [5]
WebKit/gtk/webkit/webkitwebview.h:371:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 4
Comment 3 Gustavo Noronha (kov) 2009-12-09 11:38:00 PST
Created attachment 44550 [details]
Epiphany patch using this API

As usual =).
Comment 4 Gustavo Noronha (kov) 2009-12-09 12:01:17 PST
(In reply to comment #2)
> Attachment 44549 [details] did not pass style-queue:
> 
> Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
> WebKit/gtk/webkit/webkitwebview.cpp:2398:  Use 0 instead of NULL. 
> [readability/null] [5]
> WebKit/gtk/webkit/webkitwebview.cpp:4003:  Use 0 instead of NULL. 
> [readability/null] [5]
> WebKit/gtk/webkit/webkitprivate.cpp:262:  Use 0 instead of NULL. 
> [readability/null] [5]

This is caused by our convention that NULL should be used for any calls gcc checks for the sentinel, and for code inside WebKit/gtk/.

> WebKit/gtk/webkit/webkitwebview.h:371:  Extra space before ( in function call 
> [whitespace/parens] [4]
> Total errors found: 4

This is caused by the GTK+ API convention of using vertical alignment for parameters. Any .h inside WebKit/gtk/webkit/ will have that.
Comment 5 Eric Seidel (no email) 2009-12-09 13:34:07 PST
(In reply to comment #4)
> (In reply to comment #2)
> > Attachment 44549 [details] [details] did not pass style-queue:
> > 
> > Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
> > WebKit/gtk/webkit/webkitwebview.cpp:2398:  Use 0 instead of NULL. 
> > [readability/null] [5]
> > WebKit/gtk/webkit/webkitwebview.cpp:4003:  Use 0 instead of NULL. 
> > [readability/null] [5]
> > WebKit/gtk/webkit/webkitprivate.cpp:262:  Use 0 instead of NULL. 
> > [readability/null] [5]
> 
> This is caused by our convention that NULL should be used for any calls gcc
> checks for the sentinel, and for code inside WebKit/gtk/.
> 
> > WebKit/gtk/webkit/webkitwebview.h:371:  Extra space before ( in function call 
> > [whitespace/parens] [4]
> > Total errors found: 4
> 
> This is caused by the GTK+ API convention of using vertical alignment for
> parameters. Any .h inside WebKit/gtk/webkit/ will have that.

If you believe check-webkit-style is wrong here, please file a bug an CC Dave Levin and Adam Barth.
Comment 6 Adam Barth 2009-12-09 13:38:12 PST
Please file the bug.  We don't want the bot spamming suggestions we don't expect to follow.
Comment 7 Xan Lopez 2009-12-09 23:00:17 PST
Comment on attachment 44549 [details]
first step

I realize it would be an API break, but seems to me it would be a lot more convenient to have 'icon-loaded' carry the favicon URI, right? Any anyway you are already kind of changing the API to ack the fact that the signal was carrying a frame.

What do you think?
Comment 8 Xan Lopez 2009-12-10 02:21:17 PST
Also, per comments on IRC, seems you need to close the icon database somehow if you are opening it. The other ports seem to detect when the process is being terminated to do that.
Comment 9 Julian de Navascues 2009-12-10 02:39:32 PST
(In reply to comment #0)
> WebKit has a full-blown icon database. We should probably take advantage of it,
> and implement a nice API on top of it. 

Hi! I'm working in this subject.
Comment 10 Gustavo Noronha (kov) 2009-12-10 04:26:18 PST
(In reply to comment #9)
> (In reply to comment #0)
> > WebKit has a full-blown icon database. We should probably take advantage of it,
> > and implement a nice API on top of it. 
> 
> Hi! I'm working in this subject.

Good =) thoughts on the patch I posted? =)
Comment 11 Gustavo Noronha (kov) 2009-12-10 05:57:40 PST
Created attachment 44608 [details]
second try
Comment 12 Gustavo Noronha (kov) 2009-12-10 06:01:44 PST
Created attachment 44610 [details]
real second try

Oops, forgot an important part of the test! Here it is.
Comment 13 Xan Lopez 2009-12-10 06:12:49 PST
Comment on attachment 44610 [details]
real second try

Looks good to me. The test has some style issues wrt pointer types and the '*', try to go through it once before landing.
Comment 14 Gustavo Noronha (kov) 2009-12-10 06:36:05 PST
Landed as 51948, with the problems noticed by Xan fixed. I am going to report a bug on the style checker later today.
Comment 15 Julian de Navascues 2009-12-13 05:06:34 PST
(In reply to comment #11)
> Created an attachment (id=44608) [details]
> second try

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #0)
> > > WebKit has a full-blown icon database. We should probably take advantage of it,
> > > and implement a nice API on top of it. 
> > 
> > Hi! I'm working in this subject.
> 
> Good =) thoughts on the patch I posted? =)

I think we should use #if ENABLE (ICONDATABASE) directive in future patchs like WebCore does. In the case of Epiphany I guess icons will be cached twice, but that's not a big problem. Closing icondatabse with atexit seems a great idea to me.
Comment 16 Gustavo Noronha (kov) 2009-12-13 07:09:47 PST
(In reply to comment #15)
> > Good =) thoughts on the patch I posted? =)
> 
> I think we should use #if ENABLE (ICONDATABASE) directive in future patchs like
> WebCore does. In the case of Epiphany I guess icons will be cached twice, but
> that's not a big problem. Closing icondatabse with atexit seems a great idea to
> me.

The reason I did not do this is because we already enable the icon database unconditionally for the gtk+ port, so it seemed unnecessary. This has to of course be revisited if we intend to support building with it disabled.
Comment 17 Julian de Navascues 2009-12-14 05:25:15 PST
I have opened a new bug to discuss the API to control the Icon Database: https://bugs.webkit.org/show_bug.cgi?id=3251
Comment 18 Julian de Navascues 2009-12-14 05:27:51 PST
(In reply to comment #17)
> I have opened a new bug to discuss the API to control the Icon Database:
> https://bugs.webkit.org/show_bug.cgi?id=3251

The right link is: https://bugs.webkit.org/show_bug.cgi?id=32510

Sorry!