[GTK] Notifications API does not expose or respect the "tag" attribute

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Adrian Perez]

According to https://webkitgtk.org/reference/webkit2gtk/stable/WebKitNotification.html
the WebKitNotification objects expose the following attributes:

 * body
 * title
 * id

Meanwhile, the HTML5 specification for WebNotifications includes a “tag” attribute and
a few others which are not exposed by WebKitGTK+. Documentation:

 * https://developer.mozilla.org/en-US/docs/Web/API/notification

In particular, the “tag” attribute can be used to indicate that one notification may
replace a previous one which has the same value for “tag”, instead of letting many
related notifications pile up in systems where replacing notifications is supported.
For example, when using “g_application_send_notification()” to display notifications
from a GTK+ application, it is possible to replace previous notifications, so it would
be desirable that WebKitGTK+ exposes the “tag” attribute in order to know when an
application running in an embedded WebKitWebView wants to replace a notification.

For an example usage of “tag” in the WebNotification API, see:

 * https://developer.mozilla.org/en-US/docs/Web/API/Notifications_API/Using_the_Notifications_API#Tag_example

[Michael Catanzaro]

Test page:

https://mdn.github.io/emogotchi/

With Firefox, as your emo becomes well-adjusted, Firefox creates a new notification and withdraws the previous one. With Epiphany (using our default WebKitGTK+ notification implementation) we just get a notification spam. So just exposing the property is insufficient, we also have to actually respect it in our default implementation, so applications don't have to manage this themselves.

We are also missing the icon that's supposed to be displayed. We should expose that too. Bug #164986.

[Michael Catanzaro]

(In reply to comment #1)
> With Firefox, as your emo becomes well-adjusted, Firefox creates a new
> notification and withdraws the previous one. With Epiphany (using our
> default WebKitGTK+ notification implementation) we just get a notification
> spam.

Hm, actually I must have tested this wrong. Seems it withdraws notifications itself, just after a short delay.

[Michael Catanzaro]

(In reply to comment #1)
> Test page:
> 
> https://mdn.github.io/emogotchi/

Maybe I should actually examine the test before posting it here. It doesn't use a tag at all. :p

[Michael Catanzaro]

*Temporarily* rehosted it, modified to use tags: https://people.gnome.org/~mcatanzaro/emogotchi-gh-pages/

Unfortunately our notifications do not work at all using file:// URIs, unlike Firefox. :(

[Michael Catanzaro]

Created attachment 295268 [details]
Patch

[Michael Catanzaro]

Created attachment 295271 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 295271 [details]
Patch

This looks good to me =)

[WebKit Commit Bot]

Comment on attachment 295271 [details]
Patch

Clearing flags on attachment: 295271

Committed r208974: <http://trac.webkit.org/changeset/208974>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Carlos Garcia Campos]

Comment on attachment 295271 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=295271&action=review

Please, remember that patches adding new API require the approval of two reviewers.

> Source/WebKit2/UIProcess/API/gtk/WebKitNotification.cpp:275
> +    const gchar* tag = notification->priv->tag.data();
> +    return strlen(tag) > 0 ? tag : nullptr;

You don't need to do strlen again, you have a CString with a length already:

return notification->priv->tag.length() ? notification->priv->tag.data() : nullptr;

> Source/WebKit2/UIProcess/API/gtk/WebKitNotificationProvider.cpp:111
> +        if (tag == webkit_notification_get_tag(notification.get())) {

Here you are comparing strings with different encondings, the public API method returns a UTF-8 string that here is implicitly converted to String for the comparison. You should either use String::fromUT8() or even better pass a UTF8 string to this function so that you only convert once.

> Source/WebKit2/UIProcess/API/gtk/WebKitNotificationProvider.cpp:128
> +        withdrawAnyPreviousNotificationMatchingTag(webNotification.tag());

Here you could pass webNotification.tag().utf8() instead, and use the CString.

> Source/WebKit2/UIProcess/API/gtk/WebKitNotificationProvider.cpp:129
>          notification = adoptGRef(webkitNotificationCreate(WEBKIT_WEB_VIEW(page->viewWidget()), webNotification));

Or you could create the new notification first, and pass it to withdrawAnyPreviousNotificationMatchingTag and then you would be comparing two WebKitNotification:tag properties.

> Source/WebKit2/UIProcess/API/gtk/WebKitNotificationProvider.h:51
> +    void withdrawAnyPreviousNotificationMatchingTag(const String&);

Why isn't this static instead? It's only called from WebKitNotificationProvider.cpp, no?

[Michael Catanzaro]

(In reply to comment #10)
> Comment on attachment 295271 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=295271&action=review
> 
> Please, remember that patches adding new API require the approval of two
> reviewers.

Gustavo and me, you had the rule that one reviewer could be the submitter. Remember I did ask you to look at it first.

> > Source/WebKit2/UIProcess/API/gtk/WebKitNotification.cpp:275
> > +    const gchar* tag = notification->priv->tag.data();
> > +    return strlen(tag) > 0 ? tag : nullptr;
> 
> You don't need to do strlen again, you have a CString with a length already:
> 
> return notification->priv->tag.length() ? notification->priv->tag.data() :
> nullptr;

OK
 
> > Source/WebKit2/UIProcess/API/gtk/WebKitNotificationProvider.cpp:111
> > +        if (tag == webkit_notification_get_tag(notification.get())) {
> 
> Here you are comparing strings with different encondings, the public API
> method returns a UTF-8 string that here is implicitly converted to String
> for the comparison. You should either use String::fromUT8() or even better
> pass a UTF8 string to this function so that you only convert once.

Ow, thanks for catching this.

> > Source/WebKit2/UIProcess/API/gtk/WebKitNotificationProvider.h:51
> > +    void withdrawAnyPreviousNotificationMatchingTag(const String&);
> 
> Why isn't this static instead? It's only called from
> WebKitNotificationProvider.cpp, no?

That can't be the rule to decide whether to make a function file-static as then we would never have any private member functions at all, and that's not the code style we follow. But it could indeed be easily made static if we pass the m_notificationPermissions member. I chose to make it private to avoid doing that because we don't have any guidance on which style is preferred.

[Michael Catanzaro]

Committed r208977: <http://trac.webkit.org/changeset/208977>

[Carlos Garcia Campos]

(In reply to comment #11)
> (In reply to comment #10)
> > Comment on attachment 295271 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=295271&action=review
> > 
> > Please, remember that patches adding new API require the approval of two
> > reviewers.
> 
> Gustavo and me, you had the rule that one reviewer could be the submitter.

Oh, right, I invented that rule for me :-P

> Remember I did ask you to look at it first.

And I told you, ask Gustavo, who wrote the notifications API, and I'll just check the API after his review.

> > > Source/WebKit2/UIProcess/API/gtk/WebKitNotification.cpp:275
> > > +    const gchar* tag = notification->priv->tag.data();
> > > +    return strlen(tag) > 0 ? tag : nullptr;
> > 
> > You don't need to do strlen again, you have a CString with a length already:
> > 
> > return notification->priv->tag.length() ? notification->priv->tag.data() :
> > nullptr;
> 
> OK
>  
> > > Source/WebKit2/UIProcess/API/gtk/WebKitNotificationProvider.cpp:111
> > > +        if (tag == webkit_notification_get_tag(notification.get())) {
> > 
> > Here you are comparing strings with different encondings, the public API
> > method returns a UTF-8 string that here is implicitly converted to String
> > for the comparison. You should either use String::fromUT8() or even better
> > pass a UTF8 string to this function so that you only convert once.
> 
> Ow, thanks for catching this.
> 
> > > Source/WebKit2/UIProcess/API/gtk/WebKitNotificationProvider.h:51
> > > +    void withdrawAnyPreviousNotificationMatchingTag(const String&);
> > 
> > Why isn't this static instead? It's only called from
> > WebKitNotificationProvider.cpp, no?
> 
> That can't be the rule to decide whether to make a function file-static as
> then we would never have any private member functions at all, and that's not
> the code style we follow. But it could indeed be easily made static if we
> pass the m_notificationPermissions member. I chose to make it private to
> avoid doing that because we don't have any guidance on which style is
> preferred.

I'm sorry, I thought that was Private.h header for a GObject implementation.

[Michael Catanzaro]

(In reply to comment #13)
> Oh, right, I invented that rule for me :-P

I thought so. ;)
 
> And I told you, ask Gustavo, who wrote the notifications API, and I'll just
> check the API after his review.

I must have missed that, sorry.