1438051 - GNOME doesn't know that Firefox is sending the notification

Status: RESOLVED FIXED

(Toolkit Graveyard :: Notifications and Alerts, defect, P3)

Description

[Corentin Noël]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20180208173149

Steps to reproduce:

Get a notification from a website


Actual results:

The notification is shown on the desktop (good)
But there is no way to manage the notification preferences for firefox nor is there a way to have it listed in the notification center.
GNOME has no (real) way to know that it's coming from firefox


Expected results:

I should be able to mute firefox notifications from the settings app

Comment 1

[Corentin Noël]

Attachment: gnome-notification-desktop-entry.diff

Comment 2

[Corentin Noël]

Here is a patch that should fix this issue
Note: I haven't tested this patch

Comment 3

[Lina Butler [:lina]]

Oh, nifty! Thank you very much for the patch, I can test on my Ubuntu VM tomorrow. I see Nautilus did something similar in https://bugzilla.gnome.org/show_bug.cgi?id=690995.

Does this require Firefox to ship with a `firefox.desktop` file? I don't think we do that yet (bug 296568).

Comment 4

[Corentin Noël]

Wow, I wasn't even aware that firefox had no built-in .desktop file, at least all the distros are using firefox.desktop as a name so this shouldn't be an issue, but bug 296568 should really be resolved one day.

All the users of this variable should handle the missing .desktop file gracefully anyway if it's the case somewhere.

For reference, this is where GNOME Shell is taking this into account https://gitlab.gnome.org/GNOME/gnome-shell/blob/master/js/ui/notificationDaemon.js#L189
We are doing something similar on elementary OS.

Thanks for your commitment on this amazing browser :)

Comment 5

[Shubham Arora]

(In reply to Corentin Noël from comment #4)
> Wow, I wasn't even aware that firefox had no built-in .desktop file, at
> least all the distros are using firefox.desktop as a name so this shouldn't
> be an issue, but bug 296568 should really be resolved one day.
> 
> All the users of this variable should handle the missing .desktop file
> gracefully anyway if it's the case somewhere.
> 
> For reference, this is where GNOME Shell is taking this into account
> https://gitlab.gnome.org/GNOME/gnome-shell/blob/master/js/ui/
> notificationDaemon.js#L189
> We are doing something similar on elementary OS.
> 
> Thanks for your commitment on this amazing browser :)

So for clarification, the desktop file should be named "firefox.desktop" for this fix to work? I usually use "nightly.desktop" on elementary.

Comment 6

[Corentin Noël]

@(In reply to Shubham Arora from comment #5)
> So for clarification, the desktop file should be named "firefox.desktop" for
> this fix to work? I usually use "nightly.desktop" on elementary.

Yes, until there is proper support of the desktop file in firefox…
But right now there is no other choice than doing this as we can't reliably get the desktop name from anywhere in the source code.

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 8950802 [details] [diff] [review]
gnome-notification-desktop-entry.diff

Martin, are you someone who would be able to review this?

Comment 8

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8950802 [details] [diff] [review]
gnome-notification-desktop-entry.diff

Thanks for the patch.

+    notify_notification_set_hint_string = (notify_notification_set_hint_string_t)dlsym(libNotifyHandle, "notify_notification_set_hint_string");

notify_notification_set_hint_string is deprecated, please use notify_notification_set_hint() instead.

+  // Send the desktop name to identify the application
+  // The desktop-entry is the part before the .desktop
+  notify_notification_set_hint_string(mNotification, "desktop-entry", "firefox");

you should read the actual name run-time as it can be "thunderbird" for instance.  see https://dxr.mozilla.org/mozilla-central/source/ipc/glue/WindowsMessageLoop.cpp#509

Comment 9

[Corentin Noël]

(In reply to Martin Stránský [:stransky] from comment #8)
> Comment on attachment 8950802 [details] [diff] [review]
> gnome-notification-desktop-entry.diff
> 
> Thanks for the patch.
> 
> +    notify_notification_set_hint_string =
> (notify_notification_set_hint_string_t)dlsym(libNotifyHandle,
> "notify_notification_set_hint_string");
> 
> notify_notification_set_hint_string is deprecated, please use
> notify_notification_set_hint() instead.
> 
> +  // Send the desktop name to identify the application
> +  // The desktop-entry is the part before the .desktop
> +  notify_notification_set_hint_string(mNotification, "desktop-entry",
> "firefox");
> 
> you should read the actual name run-time as it can be "thunderbird" for
> instance.  see
> https://dxr.mozilla.org/mozilla-central/source/ipc/glue/WindowsMessageLoop.
> cpp#509

Thanks for the review,
unfortunately, this approach won't work here as there is no field to get the desktop entry name. The name of the application is likely to be different from the desktop file name.
(if looking at https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIXULAppInfo is the right place)
There is maybe another class that I'm missing here?

Comment 10

[Martin Stránský [:stransky] (ni? me)]

(In reply to Corentin Noël from comment #9)
> (In reply to Martin Stránský [:stransky] from comment #8)
> > Comment on attachment 8950802 [details] [diff] [review]
> > gnome-notification-desktop-entry.diff
> > 
> > Thanks for the patch.
> > 
> > +    notify_notification_set_hint_string =
> > (notify_notification_set_hint_string_t)dlsym(libNotifyHandle,
> > "notify_notification_set_hint_string");
> > 
> > notify_notification_set_hint_string is deprecated, please use
> > notify_notification_set_hint() instead.
> > 
> > +  // Send the desktop name to identify the application
> > +  // The desktop-entry is the part before the .desktop
> > +  notify_notification_set_hint_string(mNotification, "desktop-entry",
> > "firefox");
> > 
> > you should read the actual name run-time as it can be "thunderbird" for
> > instance.  see
> > https://dxr.mozilla.org/mozilla-central/source/ipc/glue/WindowsMessageLoop.
> > cpp#509
> 
> Thanks for the review,
> unfortunately, this approach won't work here as there is no field to get the
> desktop entry name. The name of the application is likely to be different
> from the desktop file name.
> (if looking at
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/
> Interface/nsIXULAppInfo is the right place)
> There is maybe another class that I'm missing here?

I mean read the name attribute ("firefox" in this case) from nsIXULAppInfo - doesn't it work? You may need to lowercase it but it should work...or do you mean you get "nightly" for instance?

Comment 11

[Corentin Noël]

So the patch didn't took into account this issue at all, but I don't think that taking the name and lower-casing it will do the trick as there might be various different users that might have more complicated names. So this might just work for firefox and thunderbird for now if I do that.
In top of that the new recommendation for desktop files is to use a reverse domain name notation (meaning that firefox.desktop might become org.mozilla.Firefox.desktop in the future)

So this will probably require some changes someday to have this working reliably for every use case…

Comment 12

[Martin Stránský [:stransky] (ni? me)]

(In reply to Corentin Noël from comment #11)
> So the patch didn't took into account this issue at all, but I don't think
> that taking the name and lower-casing it will do the trick as there might be
> various different users that might have more complicated names. So this
> might just work for firefox and thunderbird for now if I do that.

I'd say it's still better than nothing which we have now. But feel free to improve it any way.

Comment 13

[Martin Stránský [:stransky] (ni? me)]

btw. May we use a env variable to set the desktop file name? for instance MOZ_DESKTOP_FILE_NAME=firefox can be set at launch script and set by notify_notification_set_hint() then.

Comment 14

[Mike Hommey [:glandium]]

nsXREAppData has a remotingName field. Arguably, that's what should be used.

Comment 15

[Martin Stránský [:stransky] (ni? me)]

(In reply to Mike Hommey [:glandium] from comment #14)
> nsXREAppData has a remotingName field. Arguably, that's what should be used.

Onder, can you please look at it? Thanks.

Comment 16

[Martin Stránský [:stransky] (ni? me)]

Ondrej, I propose to check MOZ_DESKTOP_FILE_NAME variable first and if it's set [1] then use it as the desktop name. If MOZ_DESKTOP_FILE_NAME is not set fallback to gAppData->name value.

[1] check it like we do at https://dxr.mozilla.org/mozilla-central/rev/4e56a2f51ad739ca52046723448f3129a58f1666/widget/gtk/nsWindow.cpp#7120

Comment 17

[Ondrej Zoder [:ozoder]]

Attachment: gnome-desktop-notification.patch

Comment 18

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8998149 [details] [diff] [review]
gnome-desktop-notification.patch

>diff --git a/toolkit/system/gnome/nsAlertsIconListener.cpp b/toolkit/system/gnome/nsAlertsIconListener.cpp
>--- a/toolkit/system/gnome/nsAlertsIconListener.cpp
>+++ b/toolkit/system/gnome/nsAlertsIconListener.cpp
>-    if (!notify_is_initted || !notify_init || !notify_get_server_caps || !notify_notification_new || !notify_notification_show || !notify_notification_set_icon_from_pixbuf || !notify_notification_add_action || !notify_notification_close) {
>+    notify_notification_set_hint = (notify_notification_set_hint_t)dlsym(libNotifyHandle, "notify_notification_set_hint");
>+    if (!notify_is_initted || !notify_init || !notify_get_server_caps || !notify_notification_new || !notify_notification_show || !notify_notification_set_icon_from_pixbuf || !notify_notification_add_action || !notify_notification_close || !notify_notification_set_hint) {
>       dlclose(libNotifyHandle);
>       libNotifyHandle = nullptr;
>     }

Please don't quit when notify_notification_set_hint is missing as it's 0.6 symbol only.

Comment 19

[Ondrej Zoder [:ozoder]]

Attachment: gnome-desktop-notification.patch

Comment 20

[Ondrej Zoder [:ozoder]]

Attachment: gnome-desktop-notification.patch

Comment 21

[Martin Stránský [:stransky] (ni? me)]

Attachment: patch for check-in

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e789bd1ead09
[Linux/GNOME] Use notify_notification_set_hint() to pair Firefox with a system notification, r=stransky

Comment 23

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/e789bd1ead09