GNOME doesn't know that Firefox is sending the notification

RESOLVED FIXED in Firefox 63

Status

()

defect
P3
normal
RESOLVED FIXED
a year ago
8 months ago

People

(Reporter: corentin, Assigned: corentin)

Tracking

58 Branch
mozilla63
x86_64
Linux
Points:
---

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

a year ago
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
(Assignee)

Comment 1

a year ago
(Assignee)

Comment 2

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

Updated

a year ago
Component: Untriaged → Notifications and Alerts
OS: Unspecified → Linux
Product: Firefox → Toolkit
Hardware: Unspecified → x86_64
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).
(Assignee)

Comment 4

a year ago
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

a year ago
(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.
(Assignee)

Comment 6

a year ago
@(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.
Assignee: nobody → corentin
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: -- → P3
Comment on attachment 8950802 [details] [diff] [review]
gnome-notification-desktop-entry.diff

Martin, are you someone who would be able to review this?
Attachment #8950802 - Flags: review?(stransky)
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
Attachment #8950802 - Flags: review?(stransky) → review-
(Assignee)

Comment 9

11 months ago
(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?
(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?
(Assignee)

Comment 11

11 months ago
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…
(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.
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.
nsXREAppData has a remotingName field. Arguably, that's what should be used.
(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.
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
Attachment #8998149 - Flags: review?(stransky)
Attachment #8950802 - Attachment is obsolete: true
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.
Attachment #8998149 - Flags: review?(stransky)
Attachment #8998149 - Attachment is obsolete: true
Attachment #8999512 - Flags: review?(stransky)
Attachment #8999512 - Attachment is obsolete: true
Attachment #8999512 - Flags: review?(stransky)
Attachment #8999514 - Flags: review?(stransky)
Attachment #8999514 - Flags: review?(stransky) → review+

Comment 22

8 months ago
Pushed by ebalazs@mozilla.com:
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
Keywords: checkin-needed

Comment 23

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e789bd1ead09
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.