Bug 296927 – [WPE] Favicon database support

• Component: New Bugs
• Status: ASSIGNED

[Adrian Perez]

WebKitFaviconDatabase was removed from the public API of the WPE port in 259675@main, because it did not have a way of retrieving the actual favicon pixel data in order to avoid exposing Cairo types in the public API -- so it seemed moot to have it at all.

We could provide the pixel data in a fixed format (RGBA8888 should suffice for any kind of favicon) with a few additional metadata bits about the favicon: width, height, and stride. There is precedent in WPEPlatform, where we have a WPEView.set_cursor_from_bytes vfunc that does exactly that. This way we would not need to expose any kind of internal type used for graphics handling.

For example, something like the following could work well, adding a boxed type that can be queried to get the favicon metadata and the pixel data:

  typedef struct _WebKitFavicon WebKitFavicon;  // Boxed type

  guint webkit_favicon_get_width(WebKitFavicon*);
  guint webkit_favicon_get_height(WebKitFavicon*);
  GBytes* webkit_favicon_get_data(WebKitFavicon*);

The webkit_favicon_database_get_favicon() function would be the same as for the GTK port, the only difference would be that the corresponding _finish() function would return a WebKitFavicon instance (instead of e.g. a GdkTexture or a cairo_surface_t):

  WebKitImage* webkit_favicon_database_get_favicon_finish(WebKitFaviconDatabase* database, GAsyncResult* result, GError** error);

[Adrian Perez]

Pull request: https://github.com/WebKit/WebKit/pull/49452

[Adrian Perez]

(In reply to Adrian Perez from comment #1)
> Pull request: https://github.com/WebKit/WebKit/pull/49452

The  suggested patch works, but in the end it has a number of issues that we would rather solve with *new*, improved API, then deprecate the existing API and make it work in terms of the new one. 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)

Opinions?

[Simon Pena]

The above sounds good!

To add to it, and based on https://html.spec.whatwg.org/multipage/links.html#rel-icon:

> Icons could be auditory icons, visual icons, or other kinds of icons. If multiple icons are provided, the user agent must select the most appropriate icon according to the type, media, and sizes attributes. If there are multiple equally appropriate icons, user agents must use the last one declared in tree order at the time that the user agent collected the list of icons. If the user agent tries to use an icon but that icon is determined, upon closer examination, to in fact be inappropriate (e.g. because it uses an unsupported format), then the user agent must try the next-most-appropriate icon as determined by the attributes.

I feel the API should allow certain level of filtering (by each of the attributes mentioned above)

I didn't know about the "media" property, but found an example at https://favicon.im/blog/favicon-for-light-dark-modes: 

    <head>
      <!-- Default favicon (fallback for unsupported browsers) -->
      <link rel="icon" href="/favicon-light.ico" type="image/x-icon">
  
      <!-- Light mode favicon -->
      <link rel="icon" href="/favicon-light.ico" type="image/x-icon" media="(prefers-color-scheme: light)">
  
      <!-- Dark mode favicon -->
      <link rel="icon" href="/favicon-dark.ico" type="image/x-icon" media="(prefers-color-scheme: dark)">
    </head>

We could expose all the available types, media and sizes, so our getter could be then filtered by them. As for returning a list, I understand we should respect the "tree order" when we return them.

[Adrian Perez]

(In reply to Simon Pena from comment #3)
> The above sounds good!
> 
> To add to it, and based on
> https://html.spec.whatwg.org/multipage/links.html#rel-icon:
> 
> > Icons could be auditory icons, visual icons, or other kinds of icons. If multiple icons are provided, the user agent must select the most appropriate icon according to the type, media, and sizes attributes. If there are multiple equally appropriate icons, user agents must use the last one declared in tree order at the time that the user agent collected the list of icons. If the user agent tries to use an icon but that icon is determined, upon closer examination, to in fact be inappropriate (e.g. because it uses an unsupported format), then the user agent must try the next-most-appropriate icon as determined by the attributes.
> 
> I feel the API should allow certain level of filtering (by each of the
> attributes mentioned above)
> 
> I didn't know about the "media" property, but found an example at
> https://favicon.im/blog/favicon-for-light-dark-modes: 
> 
>     <head>
>       <!-- Default favicon (fallback for unsupported browsers) -->
>       <link rel="icon" href="/favicon-light.ico" type="image/x-icon">
>   
>       <!-- Light mode favicon -->
>       <link rel="icon" href="/favicon-light.ico" type="image/x-icon"
> media="(prefers-color-scheme: light)">
>   
>       <!-- Dark mode favicon -->
>       <link rel="icon" href="/favicon-dark.ico" type="image/x-icon"
> media="(prefers-color-scheme: dark)">
>     </head>
> 
> We could expose all the available types, media and sizes, so our getter
> could be then filtered by them. As for returning a list, I understand we
> should respect the "tree order" when we return them.

While this looks like a good idea, I think it's better to add first
the smallest amount of new public functions possible, and then add
to it later: additions are always backwards-compatible, both API- and
ABI-wise.

With the proposed approach, we could add later new functions like for
example webkit_favicon_database_find_icon() that somehow would accept
a filter (maybe a callback?) to pick a single result. Thing is, then
we would *really* want a WebKitFavicon type (subclass of WebKitImage?)
that can provide additional information. I think this may be actually
good, and then we could also add webkit_favicon_get_url() to expose
the URL of the favicon image: I've been checking and Epiphany actually
uses that in the code that implements the Firefox Sync support (file
ephy-open-tabs-record.c to be precise).

Also, I think this could be a good reason to return a WebKitImageList:
it would allow later on to add convenience functions to search and
filter the list.

[Simon Pena]

> While this looks like a good idea, I think it's better to add first
the smallest amount of new public functions possible, and then add
to it later: additions are always backwards-compatible, both API- and
ABI-wise.

That makes sense, yes. Maybe we can a meta-bug or something similar?

[Jamie Murphy]

I'd like to add that WebKitWebExtension would absolutely benefit from a `WebKitImage` type that works on both Gtk and WPE.

[Adrian Perez]

(In reply to Jamie Murphy from comment #6)
> I'd like to add that WebKitWebExtension would absolutely benefit from a
> `WebKitImage` type that works on both Gtk and WPE.

This is valuable information, thanks for commenting! I have taken that into
account in https://bugs.webkit.org/show_bug.cgi?id=301086, which is a new
meta-bug to track the addition of the improved favicons API.