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.
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.
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
Created attachment 44550 [details] Epiphany patch using this API As usual =).
(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.
(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.
Please file the bug. We don't want the bot spamming suggestions we don't expect to follow.
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?
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.
(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.
(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? =)
Created attachment 44608 [details] second try
Created attachment 44610 [details] real second try Oops, forgot an important part of the test! Here it is.
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.
Landed as 51948, with the problems noticed by Xan fixed. I am going to report a bug on the style checker later today.
(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.
(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.
I have opened a new bug to discuss the API to control the Icon Database: https://bugs.webkit.org/show_bug.cgi?id=3251
(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!