Bug 32334 – [GTK] Should provide an API to control the IconDatabase

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Gustavo Noronha (kov)]

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.

[Gustavo Noronha (kov)]

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.

[WebKit Review Bot]

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

[Gustavo Noronha (kov)]

Created attachment 44550 [details]
Epiphany patch using this API

As usual =).

[Gustavo Noronha (kov)]

(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.

[Eric Seidel (no email)]

(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.

[Adam Barth]

Please file the bug.  We don't want the bot spamming suggestions we don't expect to follow.

[Xan Lopez]

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?

[Xan Lopez]

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.

[Julian de Navascues]

(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.

[Gustavo Noronha (kov)]

(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? =)

[Gustavo Noronha (kov)]

Created attachment 44608 [details]
second try

[Gustavo Noronha (kov)]

Created attachment 44610 [details]
real second try

Oops, forgot an important part of the test! Here it is.

[Xan Lopez]

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.

[Gustavo Noronha (kov)]

Landed as 51948, with the problems noticed by Xan fixed. I am going to report a bug on the style checker later today.

[Julian de Navascues]

(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.

[Gustavo Noronha (kov)]

(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.

[Julian de Navascues]

I have opened a new bug to discuss the API to control the Icon Database: https://bugs.webkit.org/show_bug.cgi?id=3251

[Julian de Navascues]

(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!