Bug 164771

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.

(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.

(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

*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. :(

Created attachment 295268 [details] Patch

Created attachment 295271 [details] Patch

Comment on attachment 295271 [details] Patch This looks good to me =)

Comment on attachment 295271 [details] Patch Clearing flags on attachment: 295271 Committed r208974: <http://trac.webkit.org/changeset/208974>

All reviewed patches have been landed. Closing bug.

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?

(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.

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

(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.

(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.