Bug 301086 – [GTK][WPE] New improved API for page favicons

• Status: NEW
• Component: WebKitGTK

[Adrian Perez]

As outlined in https://bugs.webkit.org/show_bug.cgi?id=296927#c2 we would like to have a better API for page favicons, and for WebKitFaviconDatabase. I am going to reproduce the content of that comment here:

In particular, we would like to address the following:

- WebKit currently supports reporting more than one icon for a page. Currently this causes e.g. the WebKitWebView:favicon property to be set multiple times, one per icon loaded icon. The order in which icons are set to the property is undefined and may vary from one page load to the next.
- WebKitFaviconDatabase (and in particular the internal IconDatabase) do not handle multiple icons per page URL. This means that the icon that gets stored in the database for an URL happens to be the one loaded last, which again may be a different one on each page load. This needs to be improved to handle multiple icons per page, both storing them and allowing their retrieval.
- Even if all icons for a page can be retrieved by an application by handling the WebKitWebView:favicon::notify signal, there is no way to know how many icons WebKit will report for a page. Internally WebKit *knows* how many are there, but it serially queries the loading client one-by-one. This could be changed to use a single message per page load that provides an array. This way we can also notify the upper layers the exact list of icons in a single shot operation.

Given the above, an outline of what I'd like to do regarding an improved API would be:

- Add a WebKitImage type, which would implement GIcon, and maybe GLoadableIcon. It would have the minimum needed for WPE (something like WebKitFavicon in the draft PR from earlier), and in the GTK port it would include methods to get the contents as a GdkTexture and/or a cairo_surface_t (and it may wrap them, in fact). This type could be reused in the future to implement webkit_web_view_get_snapshot() for the WPE port, too!
- Add a new WebKitWebView:icons, holding a GList of WebKitImage instances, or a helper WebKitImageList boxed type used as helper; the latter might be slightly better for introspection bindings, the same way we have WebKitFeatureList.
- Conversely, add webkit_favicon_database_get_icons() and the accompanying webkit_favicon_database_get_icons_finish(), returning the list of icons (GList or WebKitImageList) instead of a single icon.
- Add a new WebKitFaviconDatabase:icons-changed signal, the callbacks get the page URL and the list of icons. I highly doubt that any application has interest in knowing the URL of the favicon image itself: what applications want is to display the icons.

For compatibility, the GTK port would get the following (the WPE port would get only the new functionality outlined above):

- When WebKitWebView:icons changes, the new list of icons is traversed and WebKitWebView:favicon is set once for each of the icons in the list.
- When the WebKitFaviconDatabase:icons-changed is emitted, WebKitFaviconDatabase:favicon-changed also is emitted, with the information for the last saved icon (as it already does now)

Simon Pena suggested adding ways to filter the icons, but we should focus first on having a base API that can replace the old one first, but plan for possible further additions in the future. Also, Jamie Murphy pointed out that having a WebKitImage that would work for both ports would be desirable for the WebExtensions work. Therefore, the plan is:

1.) Change IconDatabase (the internal implementation) to store and manage multiple icons per page.

2.) Add a new WebKitImage class.
    - It least it needs to allow querying width, height, stride, and provide a method to obtain a GBytes wrapping a buffer with 32bpp RGBA/BGRA pixels.
    - Implementing the GIcon and GLoadableIcon interfaces would be desirable.
    - For the GTK port, it may have a method to obtain a GdkTexture out of it, for the sake of convenience (optional?).
    - Provide a nullable “url” property which may be filled with the URL from which the image was loaded (doubtful: would it better belong in a WebKitFavicon subclass?)

3.) Add webkit_favicon_database_get_icons[_finish](), returning a WebKitFaviconList.
    - This would allow future extension by adding webkit_favicon_list_find_foo() methods, or similar.
    - It can wrap a GPtrArray of WebKitImage instances, similar to WebKitFeatureList.
    - This can probably be a boxed type.

4.) Add a WebKitFaviconDatabase::icons-changed signal.
     - For compatibility, after this is emitted, WebKitFaviconDatabase::icon-changed gets emitted for each icon.

5.) Add a new WebKitWebView::icons property holding a WebKitImageList.
     - For compatibility, after this gets set, WebKitFaviconDatabase::favicon gets set for each icon.

[Michael Catanzaro]

> - WebKit currently supports reporting more than one icon for a page. Currently this causes e.g. the WebKitWebView:favicon property to be set multiple times, one per icon loaded icon. The order in which icons are set to the property is undefined and may vary from one page load to the next.

Well, this explains why favicons sometimes seem so buggy....

> - Add a new WebKitWebView:icons, holding a GList of WebKitImage instances, or a helper WebKitImageList boxed type used as helper; the latter might be slightly better for introspection bindings, the same way we have WebKitFeatureList.

WebKitImageList is probably better than using GList, which is indeed awkward for bindings.

> Provide a nullable “url” property which may be filled with the URL from which the image was loaded (doubtful: would it better belong in a WebKitFavicon subclass?)

All properties are nullable. I would just let it be NULL if it's not a favicon. I wouldn't introduce a subclass unless you have a strong reason to.

[Jamie Murphy]

> WebKitImageList is probably better than using GList, which is indeed awkward for bindings.

This is a bit off-topic, but the fact that we have WebKitFeatureList and possibly WebKitImageList makes me wonder if there's any value in a generic WebKitList type.