WebKit Gtk port should provide an API to control Icon Database (as suggested in bug https://bugs.webkit.org/show_bug.cgi?id=32334 ).
Im preparing a patch to add a IconDatabaseClient in WebCoreSupport as Win port does. In the Win port the client implements the virtual methods but also some extra methods for things such the start up, retrieve icon from url and a lot that doesnt seem needed by know (as far as I know). So I think all database operations should be done through the client, any thoughts about this issue? (In reply to comment #0) > WebKit Gtk port should provide an API to control Icon Database (as suggested in > bug https://bugs.webkit.org/show_bug.cgi?id=32334 ).
(In reply to comment #1) > Im preparing a patch to add a IconDatabaseClient in WebCoreSupport as Win port > does. In the Win port the client implements the virtual methods but also some > extra methods for things such the start up, retrieve icon from url and a lot > that doesnt seem needed by know (as far as I know). > > So I think all database operations should be done through the client, any > thoughts about this issue? It sounds right to me. The thing to keep in mind is you should use the client to mediate the communication between applications and WebCore, so you will probably want to create a new GObject to represent the icon database, and expose all functionality through it. It will hook into the client to know what to do/perform stuff.
Do you have an idea of what features you want yet? I think the following is what I would want to use in an application: set expiration time clear cache icon url for page url icon for page url -- a pixbuf, so it can be from memory signal icon-updated -- needed for bookmark menus and buttons make sure private mode is respected set/ get the database path can icons work without writing the database to disk? would be very good for private browsing
(In reply to comment #3) > Do you have an idea of what features you want yet? I think the following is > what I would want to use in an application: > > set expiration time > clear cache > icon url for page url > icon for page url -- a pixbuf, so it can be from memory > signal icon-updated -- needed for bookmark menus and buttons > make sure private mode is respected > set/ get the database path > can icons work without writing the database to disk? would be very good for > private browsing I'm creating a new GObject as Gustavo suggests, thanks. I think all features you have proposed can be easily done using directly the IconDatabase API. But the expiration time is hardcoded in WebCore (set to 4 days). About privacy issues, we are lucky because IconDatabase has a private browsing mode that avoids writtings in disk.
(In reply to comment #4) > I'm creating a new GObject as Gustavo suggests, thanks. I think all features > you have proposed can be easily done using directly the IconDatabase API. But > the expiration time is hardcoded in WebCore (set to 4 days). About privacy > issues, we are lucky because IconDatabase has a private browsing mode that > avoids writtings in disk. Yeah, an object sounds good. Maybe expriation time can be changed in WebCore so that it can be changed - but it can be in a different patch, if the other features work it's relatively low priority.
Created attachment 83166 [details] Implement WebKitIconDatabase API So I implemented WebKitIconDatabase now. With methods set_path() to set the folder on disk, or NULL to unset it. get_icon_uri() to get the icon URI for a page URI get_icon_pixbuf to get a GdkPixbuf for a page URI clear() to clear all icons "icon-loaded" signal with the page URI as the argument, similar to the same signal on WebKitWebView but emitted for any icon including sub frames. This makes it possible to handle icon updates isolated from a particular view (tab). I also added get_icon_pixbuf() to WebKitWebView because I figure this is a very common use case and I know that a lot of people ask for this in #webkit-gtk, and it is so much easier and also more efficient than having to load the icon with WebKitDownload. No retaining API as of yet, I'm a bit unsure if/ how this should be exposed.
Attachment 83166 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1 Source/WebKit/gtk/webkit/webkiticondatabase.cpp:32: Alphabetical sorting problem. [build/include_order] [4] Source/WebKit/gtk/webkit/webkiticondatabase.cpp:191: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/WebKit/gtk/webkit/webkiticondatabase.cpp:198: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/WebKit/gtk/webkit/webkiticondatabase.cpp:253: Missing space after , [whitespace/comma] [3] Source/WebKit/gtk/webkit/webkitglobals.cpp:319: An else should appear on the same line as the preceding } [whitespace/newline] [4] Source/WebKit/gtk/webkit/webkiticondatabase.h:26: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 6 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 83169 [details] Implement WebKitIconDatabase API #2 So I implemented WebKitIconDatabase now. With methods set_path() to set the folder on disk, or NULL to unset it. get_icon_uri() to get the icon URI for a page URI get_icon_pixbuf to get a GdkPixbuf for a page URI clear() to clear all icons "icon-loaded" signal with the page URI as the argument, similar to the same signal on WebKitWebView but emitted for any icon including sub frames. This makes it possible to handle icon updates isolated from a particular view (tab). I also added get_icon_pixbuf() to WebKitWebView because I figure this is a very common use case and I know that a lot of people ask for this in #webkit-gtk, and it is so much easier and also more efficient than having to load the icon with WebKitDownload. No retaining API as of yet, I'm a bit unsure if/ how this should be exposed.
Comment on attachment 83169 [details] Implement WebKitIconDatabase API #2 (sorry the view in context link won't work here since, I made this against your old patch. :() Great stuff! I'd really like to see this update the old DumpRenderTree code and the removal of the old DumpRenderTreeSupport code in this patch. Please fix all the style issues as well as the stuff I mentioned below. > Source/WebKit/gtk/webkit/webkitglobals.cpp:248 > static void closeIconDatabaseOnExit() > { > iconDatabase()->close(); This code should probably go in webkiticondatabase.c now along with the code that actuall does the initialization. > Source/WebKit/gtk/webkit/webkitglobals.cpp:-301 > atexit(closeIconDatabaseOnExit); > } > > + WebKitIconDatabase* database = webkit_get_icon_database(); > if (enabled) { > - iconDatabase()->setEnabled(true); > GOwnPtr<gchar> iconDatabasePath(g_build_filename(g_get_user_data_dir(), "webkit", "icondatabase", NULL)); > - iconDatabase()->open(iconDatabasePath.get()); > - return; > + webkit_icon_database_set_path(database, iconDatabasePath.get()); > + } > + else { > + webkit_icon_database_set_path(database, 0); > } > - > - iconDatabase()->setEnabled(false); > - iconDatabase()->close(); I'd rather you removed this entirely along with where we call it in DumpRenderTreeSupport. If it's possible for DumpRenderTree to enable and diable the icon database with the API it should use that. > Source/WebKit/gtk/webkit/webkiticondatabase.cpp:40 > + * @short_description: A WebKit web application database > + * > + * #WebKitIconDatabase provides access to website icons, as shown > + * in tab labels, window captions or bookmarks. > + */ If it's possible to annotate these with a @since you should do that. You should expand this greatly. Explain that when the icon database is enabled WebKit will request so and so with every page. Mention that you can disable it by setting an null path. > Source/WebKit/gtk/webkit/webkiticondatabase.cpp:69 > + priv->path.set(0); Instead of explicitly clearing this, you should explicitly "new" and destruct the private area. Take a look at what happens in the webkit_web_view_finalize and webkit_web_view_init for one way to do that. > Source/WebKit/gtk/webkit/webkiticondatabase.cpp:120 > + * Since: 1.3.4 I'm pretty sure all of the 1.3.4's should be 1.3.11's. > Source/WebKit/gtk/webkit/webkiticondatabase.cpp:182 > + * Passing %NULL disables the icon database. Both null and "" should probably disable the database. > Source/WebKit/gtk/webkit/webkiticondatabase.cpp:187 > +void webkit_icon_database_set_path(WebKitIconDatabase* database, > + const gchar* path) Please use WebKit style here and put this all on one line. > Source/WebKit/gtk/webkit/webkiticondatabase.cpp:205 > + WebCore::iconDatabase()->open(database->priv->path.get()); Casting a gchar* directly to a string here is incorrect. You need to use fileSystemRepresentation from FileSystemGtk.cpp. > Source/WebKit/gtk/webkit/webkiticondatabase.cpp:222 > +gchar* webkit_icon_database_get_icon_uri(WebKitIconDatabase* database, > + const gchar* pageURI) > +{ Ditto. > Source/WebKit/gtk/webkit/webkiticondatabase.cpp:226 > + String pageURL(pageURI); Use String::fromUTF8 here. > Source/WebKit/gtk/webkit/webkiticondatabase.cpp:238 > + * The pixbuf can have a variable size and should be resized > + * before it is displayed. Does this return the icon in the largest size possible? If so, I'd mention that. If not, that's probably the behavior we want to get somehow. The documentation for this webkit_web_view_get_icon_pixbuf and actually very different. They should be in sync. > Source/WebKit/gtk/webkit/webkiticondatabase.cpp:241 > + * Returns: the a new reference to a #GdkPixbuf This should also mention that null is returned when there is a problem. What if the page has no favicon? > Source/WebKit/gtk/webkit/webkiticondatabase.cpp:252 > + // The size we pass is irrelevant as long as it is > 0,0 I think we should expand this comment a big. Why is the size irrelevant? > Source/WebKit/gtk/webkit/webkiticondatabase.cpp:256 > + GdkPixbuf* pixbuf = icon->getGdkPixbuf (); Extra space here. > Source/WebKit/gtk/webkit/webkitwebview.cpp:5076 > + * Obtains a #GdkPixbuf for the favicon for the given #WebKitWebView, or Probably should be Obtains a #GdkPixbuf for the favicon of the given #WebKitWebView > Source/WebKit/gtk/webkit/webkitwebview.cpp:5077 > + * a default icon. Use webkit_web_view_get_icon_uri() if you need to Should say when a default icon is returned. > Source/WebKit/gtk/webkit/webkitwebview.cpp:5080 > + * Usually you want to connect to WebKitWebView::icon-loaded and load the > + * pixbuf in the callback. Please fix the flow here, so that this abuts the end of the last sentence. You might want to mention that this is a just a shortcut for getting the icon database and then getting the pixbuf from there.
(In reply to comment #9) > (From update of attachment 83169 [details]) > (sorry the view in context link won't work here since, I made this against your old patch. :() Actually, it does. Just navigate to the formatted diff.
Created attachment 83333 [details] Implement WebKitIconDatabase API #3 Changed DumpRenderTree to use the new API. Moved code fully into new class except for the atexit handler. Elaborated in the introduction of the new class. Fixed all Since tags. Used placement new syntax, hadn't known that one before. Allow "" to unset just like %NULL. Used String::fromUTF8. Synced and clarifying _get_icon_pixbuf methods. Clarified comments on icon sizing. Pending: use of fileSystemRepresentation, I didn't find a way to pass that because it uses CString and I was unable to find a convertion function. Will have to check that again.
Attachment 83333 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1 Source/WebKit/gtk/webkit/webkiticondatabase.cpp:28: Alphabetical sorting problem. [build/include_order] [4] Source/WebKit/gtk/webkit/webkiticondatabase.cpp:224: Should have a space between // and comment [whitespace/comments] [4] Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:110: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 3 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 83333 [details] Implement WebKitIconDatabase API #3 View in context: https://bugs.webkit.org/attachment.cgi?id=83333&action=review Great stuff. Watch the style issues. I'd really like to see some GIR annotations in all the new documentation. > Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:111 > void DumpRenderTreeSupportGtk::setIconDatabaseEnabled(bool enabled) > { > - WebKit::setIconDatabaseEnabled(enabled); > + WebKitIconDatabase* database = webkit_get_icon_database(); > + if (enabled) { > + GOwnPtr<gchar> iconDatabasePath(g_build_filename(g_get_user_data_dir(), "webkit", "icondatabase", NULL)); > + webkit_icon_database_set_path(database, iconDatabasePath.get()); > + } else { > + webkit_icon_database_set_path(database, 0); > + } > } I would remove this method entirely and just do this in DumpRenderTree itself. DumpRenderTreeSupport really exists just to expose stuff that isn't in the API yet. When we remove things from here its a big win. : ) Perhaps DRT should have it's own icon directory too? > Source/WebKit/gtk/webkit/webkitglobals.cpp:253 > + if (!database) { > + database = WEBKIT_ICON_DATABASE(g_object_new(WEBKIT_TYPE_ICON_DATABASE, NULL)); > + atexit(closeIconDatabaseOnExit); > + } > + It makes sense to move this to where you first open the WebCore icon database actually. I'd move both the atexit call and closeIconDatabaseOnExit to webkitwebicondatabase.cpp. > Source/WebKit/gtk/webkit/webkiticondatabase.cpp:43 > + * SECTION:webkitwebdatabase > + * @short_description: A WebKit web application database > + * > + * #WebKitIconDatabase provides access to website icons, as shown > + * in tab labels, window captions or bookmarks. > + * > + * The icon database is enabled by default and stored in > + * ~/.local/share/webkit/icondatabase, depending on XDG_DATA_HOME. It's worth mentioning that all views share the same icon database. > Source/WebKit/gtk/webkit/webkiticondatabase.cpp:146 > + * @page_uri: the URI of the page containing the icon page_uri -> frame_uri. > Source/WebKit/gtk/webkit/webkiticondatabase.cpp:151 > + * This signal is emitted when a favicon is available for a page, > + * including child frames. > + * See WebKitWebView::icon-loaded if you simply need the favicon > + * for a particular #WebKitWebView. Might want to clarify this a bit. WebKitWebView::icon-loaded only fires for the main frame icon. It's probably worth repeating that in the documentation for WebKitWebView::icon-loaded and pointing to this signal for child frames. > Source/WebKit/gtk/webkit/webkiticondatabase.cpp:213 > + if (database->priv->path.get()) { > + WebCore::iconDatabase()->setEnabled(false); > + WebCore::iconDatabase()->close(); > + } Why do you do you disable the database here? >> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:224 >> + //WebCore::iconDatabase()->open(String(WebCore::fileSystemRepresentation(database->priv->path.get()))); > > Should have a space between // and comment [whitespace/comments] [4] You want WebCore::iconDatabase()->open(WebCore::filenameToString(database->priv->path.get())); > Source/WebKit/gtk/webkit/webkiticondatabase.cpp:236 > + * Returns: the URI for the favicon, or %NULL You need to mention whether it's newly allocated or not. It's probably possible to avoid it using a static CString to just cache it locally in the method. > Source/WebKit/gtk/webkit/webkiticondatabase.cpp:246 > + String pageURL = String::fromUTF8(pageURI); > + return g_strdup(WebCore::iconDatabase()->iconURLForPageURL(pageURL).utf8().data()); You might be able to do something like this: static CString iconURL; iconURL = WebCore::iconDatabase()->iconURLForPageURL(String::fromUTF8(pageURI)).utf8(); return iconURL.data(); If you did that the user does not have to free the data. > Source/WebKit/gtk/webkit/webkiticondatabase.cpp:282 > + return static_cast<GdkPixbuf*>(g_object_ref (pixbuf)); No space after g_object_ref
Created attachment 83469 [details] Implement WebKitIconDatabase API #4 (In reply to comment #13) > (From update of attachment 83333 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=83333&action=review > > Great stuff. Watch the style issues. I'd really like to see some GIR annotations in all the new documentation. Oddly enough, I did run check-webkit-style on the patch locally but it didn't show any issues. I don't know what kind of annotations you have in mind. The pixbufs are newly referenced and strings don't need annotations as far as I'm aware, and webkit_get_icon_database() does have an annotation already. > > Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:111 > > void DumpRenderTreeSupportGtk::setIconDatabaseEnabled(bool enabled) > > { > > - WebKit::setIconDatabaseEnabled(enabled); > > + WebKitIconDatabase* database = webkit_get_icon_database(); > > + if (enabled) { > > + GOwnPtr<gchar> iconDatabasePath(g_build_filename(g_get_user_data_dir(), "webkit", "icondatabase", NULL)); > > + webkit_icon_database_set_path(database, iconDatabasePath.get()); > > + } else { > > + webkit_icon_database_set_path(database, 0); > > + } > > } > > I would remove this method entirely and just do this in DumpRenderTree itself. DumpRenderTreeSupport really exists just to expose stuff that isn't in the API yet. When we remove things from here its a big win. : ) > > Perhaps DRT should have it's own icon directory too? Moved the code inside DRT and made it use /tmp/webkit now. This actually seems like a good idea to avoid unexplicable test failures. > > Source/WebKit/gtk/webkit/webkitglobals.cpp:253 > > + if (!database) { > > + database = WEBKIT_ICON_DATABASE(g_object_new(WEBKIT_TYPE_ICON_DATABASE, NULL)); > > + atexit(closeIconDatabaseOnExit); > > + } > > + > > It makes sense to move this to where you first open the WebCore icon database actually. I'd move both the atexit call and closeIconDatabaseOnExit to webkitwebicondatabase.cpp. Moved. > > Source/WebKit/gtk/webkit/webkiticondatabase.cpp:213 > > + if (database->priv->path.get()) { > > + WebCore::iconDatabase()->setEnabled(false); > > + WebCore::iconDatabase()->close(); > > + } > > Why do you do you disable the database here? Trying to keep the code flow simple. I changed it now. > > Source/WebKit/gtk/webkit/webkiticondatabase.cpp:236 > > + * Returns: the URI for the favicon, or %NULL > > You need to mention whether it's newly allocated or not. It's probably possible to avoid it using a static CString to just cache it locally in the method. I think in this case it is fully expected to get a newly allocated string. In particular because there is no single string that is always returned, unlike the icon of a page loaded at a point in time. And this could actually lead to bugs when loading more than one icon at the same time. I also added a comment that icons are cleaned up after 4 days and mentioned that private enable-private-browsing prevents changes of the database on disk.
Comment on attachment 83469 [details] Implement WebKitIconDatabase API #4 View in context: https://bugs.webkit.org/attachment.cgi?id=83469&action=review Thanks for the changes. We just need to have another GTK+ reviewer look this over to okay the API additions. > Tools/DumpRenderTree/gtk/LayoutTestControllerGtk.cpp:497 > + GOwnPtr<gchar> iconDatabasePath(g_build_filename(g_get_tmp_dir(), "webkit", "icondatabase", NULL)); Probably want to change "webkit" to "DumpRenderTree"
Some comments from a fellow hacker: (09:48:27 AM) csaavedra: mrobinson: I remember gir complaining when I had not explicitly annotated the methods returning pixbuf objects (09:49:21 AM) csaavedra: (unless it is a _new() methods I believe) (09:49:53 AM) csaavedra: but hey, just generating the gir file and seeing the output should suffice to find out (09:50:24 AM) csaavedra: there is a typo here btw "* Returns: the a new reference to a #GdkPixbuf, or %NULL"
Created attachment 83810 [details] Implement WebKitIconDatabase API #5 Updated filename, corrected typo and added transfer: full annoation.
Comment on attachment 83810 [details] Implement WebKitIconDatabase API #5 View in context: https://bugs.webkit.org/attachment.cgi?id=83810&action=review Looks good to me! With the fixes for the @since lines and the small documentation change below, this just needs the approval of another GTK+ reviewer. Thanks! > Source/WebKit/gtk/webkit/webkitglobals.cpp:233 > + * Since: 1.3.4 Needs to be 1.3.13. > Source/WebKit/gtk/webkit/webkitwebview.cpp:5081 > + * Usually you want to connect to WebKitWebView::icon-loaded and load the Probably should change 'load' to 'get' or 'call this method'. > Source/WebKit/gtk/webkit/webkitwebview.cpp:5090 > + * Since: 1.3.11 All of these lines need to be 1.3.13, I think.
Comment on attachment 83810 [details] Implement WebKitIconDatabase API #5 View in context: https://bugs.webkit.org/attachment.cgi?id=83810&action=review > Source/WebKit/gtk/webkit/webkiticondatabase.cpp:153 > + * @webView: the object on which the signal is emitted > + * @frame_uri: the URI of the frame containing the icon > + * It feels weird to have @webView here, is this actually meant to be @iconDatabase? Also, would it make sense to have the frame as one of the arguments? Everything else looks good to me.
Created attachment 84745 [details] Implement WebKitIconDatabase API #6 Fixed all Since tags. Changed 'load the pixbuf' to 'call this method'. webView → database added a frame argument to icon-loaded
Attachment 84745 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1 Source/WebKit/gtk/webkit/webkiticondatabase.cpp:28: Alphabetical sorting problem. [build/include_order] [4] Tools/DumpRenderTree/gtk/LayoutTestControllerGtk.cpp:500: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 2 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 84745 [details] Implement WebKitIconDatabase API #6 View in context: https://bugs.webkit.org/attachment.cgi?id=84745&action=review > Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.h:-4 > -/* > - * Copyright (C) Research In Motion Limited 2010. All rights reserved. > - * > - * This library is free software; you can redistribute it and/or Did you mean to remove this file?
Created attachment 84760 [details] Implement WebKitIconDatabase API #7 > Did you mean to remove this file? No, accident during conflict resolving. I also had to update the patch because apparently IconDatabase in WebCore changed after the commit I had written it for. So updated patch, against latest TOT, and style issues resolved.
Comment on attachment 84760 [details] Implement WebKitIconDatabase API #7 View in context: https://bugs.webkit.org/attachment.cgi?id=84760&action=review Great stuff. Thanks for implementing this. > Source/WebKit/gtk/webkit/webkitglobals.cpp:242 > + static WebKitIconDatabase* database = 0; > + > + webkitInit(); > + > + if (!database) Small nit here. Just move the declaration of database right above where it's first used. > Source/WebKit/gtk/webkit/webkiticondatabase.cpp:50 > + * found images into the memory cache if possible. The signal "icon-loaded" found images -> images found or images it finds > Source/WebKit/gtk/webkit/webkiticondatabase.cpp:279 > + * The pixbuf will be the largest possible size and should be > + * resized before it is displayed. I guess it might be clearer here to say "will have the same size as the original image from the server" or something similar. > Source/WebKit/gtk/webkit/webkiticondatabase.cpp:282 > + * Returns: (transfer full): a new reference to a #GdkPixbuf, or %NULL No comma necessary here, I think. > Source/WebKit/gtk/webkit/webkitwebview.cpp:5080 > + * The pixbuf will be the largest possible size and should be > + * resized before it is displayed. Ditto.
> > Source/WebKit/gtk/webkit/webkiticondatabase.cpp:282 > > + * Returns: (transfer full): a new reference to a #GdkPixbuf, or %NULL > > No comma necessary here, I think. The comma is always used in "it could also be NULL" cases, look at gtk_image_get_ or gtk_notebook_get_ functions which can return NULL. Committed as r80561.