Web Notifications support does not integrate with libnotify

RESOLVED FIXED in mozilla36

Status

()

defect
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: from_bugzilla2, Assigned: stransky)

Tracking

(Blocks 1 bug)

22 Branch
mozilla36
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 13 obsolete attachments)

10.47 KB, patch
karlt
: feedback+
Details | Diff | Splinter Review
16.13 KB, patch
Details | Diff | Splinter Review
Reporter

Description

6 years ago
In Aurora 22.0a2, the new web notifications are, as I understand it, implemented in XUL on all platforms.

This poses several problems for libnotify-based notification systems:

1. Web Notifications are invisible to the "missed notifications" logs implemented by some notification daemons.

2. Many (but not all) desktops place their native notifications in the top-right corner of the screen where they'll either cover or be covered by Firefox-provided notifications.

3. Web notifications don't obey the theming settings for the rest of the desktop.

4. Web notifications will not be picked up by any kind of forwarding or filtering mechanism the user might have in place. (For example, one blogger implemented a notification-daemon proxy which batches up notifications which arrive rapidly and defers display until fullscreen games exit to prevent composited desktops from suspending the game's exclusive access to the GPU and causing everything to slow down and stutter.)

Updated

6 years ago
Component: General → XUL Widgets
Product: Firefox → Toolkit

Updated

6 years ago
Blocks: 853104
What are the plans for libnotify?
Blocks: 782211, 875383
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(wchen)
As I understand it, the problem is essentially the same as on Mac with system notifications: the web notification API doesn't map to what is provided by system notifications.
Reporter

Comment 3

6 years ago
(In reply to Mike Hommey [:glandium] from comment #2)
> As I understand it, the problem is essentially the same as on Mac with
> system notifications: the web notification API doesn't map to what is
> provided by system notifications.

In other words, it was rushed to standardization and the API didn't get enough thought.

Still, it's possible to map the notification API to libnotify without too many heuristic transformations.

In fact, the only really iffy part is body markup, since web notifications support arbitrary nonsense while libnotify accepts Pango's special HTML-like SGML grammar plus "actions" specified separately and other notification daemons might be written by people who didn't read the specs and assumed that Pango SGML was ONLY a subset of HTML.

Here's my suggestion, based on providing proper graceful degradation:

1. onclick and onclose map directly to the Glib "clicked" and "closed" signals offered by libnotify. Just proxy them through.

There's a notify_notification_get_closed_reason() function but the docs don't say what the possible return values mean.

2. The spec seems to indicate that onerror is completely an in-browser event (Browser refused permission, D-Bus call failed, etc.) so no issue there.

3. Using onshow to implement timeouts as in the spec examples is a bad API decision that needs to be worked around. The libnotify docs are poor and I can't tell if there's an event along the lines of "showed" but, at least on desktops, there will be enough vertical screen space for the notification to appear immediately 99.9% unless the daemon doesn't allow stacking.

(If there isn't, then the user is being flooded with notifications from some broken application and, whether or not they are, it's common for notification daemons to have a "missed messages" queue which receives any notifications that timed out or were closed by the notification.close() in the spec example.)

My advice is to just make notification.close() a no-op and let the OS default timeout handle things unless the spec gets amended to implement notification timeouts as an attribute specified on notification creation. (In which case, something like "0" could map to the "remain until explicitly dismissed" behaviour while null would be "use default")

4. Just pass `title` through. The notification daemon is supposed to accept unicode plaintext. (I'm not sure how to check titleDir but I doubt it'd be difficult to handle.)

   notify-send 'title is treated as üñïçōḑé <plaintext>'

5. If notify_get_server_caps() returns ICON_STATIC, then you've got something you can map to iconUrl. I'm not sure what ICON_MULTI means due to the bad docs but you'll also want to follow the IMAGE_SVG capability when determining what to send.

6. `tag` can be implemented in the browser. Just let everything with the same tag resolve to the same libnotify-level notification object.

7. If notify_get_server_caps() returns BODY and not BODY_MARKUP, render the notification as unicode plaintext with <pre>-style whitespace.

8. If notify_get_server_caps() does return BODY_MARKUP, then assume the common subset of HTML and Pango's SGML grammar... which means <b>, <big>, <i>, <sub>, <sup>, <small>, <span>, <tt>, <u>, and <s> with everything inside an implicit <pre> tag and no attributes allowed.

https://developer.gnome.org/pango/stable/PangoMarkupFormat.html

Any element or attribute not part of Pango's SGML grammar causes the reference implementation to not display the notification body at all.

    notify-send 'markup test' '<b>bold</b> <big>big</big> <i>italic</i> <sub>subscript</sub> <sup>superscript</sup> <small>small</small> <span>span</span> <tt>monospace</tt> <u>underline</u> <s>strikethrough</s>'

    notify-send 'unknown attributes prevent body from showing' '<b>bold</b> <span style="text-decoration: strikethrough">span</span>'

    notify-send 'unknown tags prevent body from showing' '<b>bold</b> <abbr title="foo">span</abbr>'

    notify-send "body can't contain buttons" "<input type='submit'>foo</input>" 
    notify-send "body can't contain buttons" "<input type='button'>foo</input>"
    notify-send "body can't contain buttons" "<button>foo</button>"

    notify-send "Whitespace in the body is literal" "As with
    &lt;pre&gt; elements"

9. If notify_get_server_caps() returns BODY_IMAGES, I assume the <img> tag will work, but the reference implementation, which I use, doesn't support it. Fall back to displaying ALT text.

10. If notify_get_server_caps() returns BODY_HYPERLINKS, trim inline links down to contain only the `href` and `title` attributes. If not, render them as plaintext to prevent body from being discarded for containing unsupported tags.

    notify-send 'Link support' 'Inline <a href="http://www.example.com/" title="foo">links</a> are supported by the reference notification-daemon.'

11. I have no idea how to use the `sound` capability and I'm not sure whether you'd consider it worth the effort to map <audio> tags in notifications to it, if present.

12. If notify_get_server_caps() returns ACTIONS and notify_get_server_info() doesn't say that "name" is "notify-osd", then extract all buttons (input[type="submit"], input[type="button"], button) before sanitizing the body markup and register their actions using notify_notification_add_action()

You check for notify-osd because Ubuntu is trying to strong-arm their own interpretation of notifications as not being something you can't interact with via the mouse and, according to their HIG, notify-osd falls back to displaying some sort of dialog box if you ask for interaction.

Applications under Ubuntu are expected to either just force-switch the user to the thing requiring interaction or integrate with UI elements like the Messaging Menu that aren't even present on Xubuntu, Kubuntu, or Lubuntu, let alone other Linux distros, so let them figure it out.

(Better to not display actions than trigger annoying dialog boxes... let the maintainers of the "Ubuntu Firefox Modifications" extension figure out how to work around the mess their unusual HIG and notify-osd design has created. They're already doing it for their MacOSX-style menubar.)

There's example code for adding actions here: http://developer.ubuntu.com/resources/technologies/notification/6/

Their example code for detecting all the capabilities (and then string-matching to detect notify-osd) is at http://developer.ubuntu.com/resources/technologies/notification/4/

The (anemic) API docs can be found at https://developer.gnome.org/libnotify/0.7/

Here's a list of notification daemons to demonstrate why checking the capabilities is so important: https://wiki.archlinux.org/index.php/Libnotify#Installation

dunst basically limits notifications to terminal text styling: http://www.knopwob.org/dunst/screenshots.html

...and statnot is just a proxy for injecting notifications into a tiling window manager's status bar: https://github.com/halhen/statnot
libnotify is pretty much a wrapper around the org.freedesktop.Notifications DBus interface documented at http://people.gnome.org/~mccann/docs/notification-spec/notification-spec-latest.html

(In reply to Stephan Sokolow from comment #3)
> There's a notify_notification_get_closed_reason() function but the docs
> don't say what the possible return values mean.

Probably the same as |reason| in
http://people.gnome.org/~mccann/docs/notification-spec/notification-spec-latest.html#signal-notification-closed

> 5. If notify_get_server_caps() returns ICON_STATIC, then you've got
> something you can map to iconUrl. I'm not sure what ICON_MULTI means due to
> the bad docs [...]

http://people.gnome.org/~mccann/docs/notification-spec/notification-spec-latest.html#command-get-capabilities
Reporter

Comment 6

6 years ago
(In reply to Karl Tomlinson (:karlt) from comment #5)
> libnotify is pretty much a wrapper around the org.freedesktop.Notifications
> DBus interface documented at
> http://people.gnome.org/~mccann/docs/notification-spec/notification-spec-
> latest.html

Thanks. Things originating in the Galago project have such a poor history for documentation and such a bad relationship with GoogleBot that I wasn't sure whether that didn't exist or just wasn't showing up in the queries I was trying.
(In reply to Stephan Sokolow from comment #3)
> 8. If notify_get_server_caps() does return BODY_MARKUP, then assume the
> common subset of HTML and Pango's SGML grammar... which means <b>, <big>,
> <i>, <sub>, <sup>, <small>, <span>, <tt>, <u>, and <s> with everything
> inside an implicit <pre> tag and no attributes allowed.
> 
> https://developer.gnome.org/pango/stable/PangoMarkupFormat.html
> 
> Any element or attribute not part of Pango's SGML grammar causes the
> reference implementation to not display the notification body at all.

Seems GNOME Shell (3.8) is even dumber, only recognizing <b>, <i> and <u>, leaving other tags as literal. It also transforms plain URLs into hyperlinks, but doesn't support explicit links.

Its reported capabilities are:
actions,action-icons,body,body-markup,icon-static,persistence,sound

Updated

6 years ago
Duplicate of this bug: 887748

Updated

6 years ago
Duplicate of this bug: 924055

Comment 10

6 years ago
So from https://dvcs.w3.org/hg/notifications/raw-file/tip/Notifications.html 

"body of type DOMString
    The value contains the secondary text, or body, of the notification.

    The user agent must process any markup in this string so that it appears as plain text when used as a string in the underlying notification platform."

So according to the current spec it has to pass plain text to the "underlying notification platform." ... so we should be able to just use libnotify for that.
Reporter

Comment 11

6 years ago
(In reply to drago01 from comment #10)
> So from https://dvcs.w3.org/hg/notifications/raw-file/tip/Notifications.html 
> 
> "body of type DOMString
>     The value contains the secondary text, or body, of the notification.
> 
>     The user agent must process any markup in this string so that it appears
> as plain text when used as a string in the underlying notification platform."
> 
> So according to the current spec it has to pass plain text to the
> "underlying notification platform." ... so we should be able to just use
> libnotify for that.

As I understand that, what it's actually saying is that the user agent must perform any transformations necessary to ensure that the underlying notification system displays the text without displaying any nonsense like escaped markup.

Or, to put it in more concrete terms, the spec requires that Firefox either filter out or translate any markup other than <b>, <i>, and <u> if the notification daemon is GNOME Shell 3.8.

Comment 12

5 years ago
there is no problem; the specs are compatible. y’all are too nitpicky.

“appears as plain text” means “no silly angled brackets”, i.e. all tags that aren’t supported are to be removed instead of being left in. so we just have to do:

1. accept HTML markup as notification body
2. replace non-whitelisted tags with their contents
3. remove non-whitelisted attributes from whitelisted tags
4. pass resulting HTML subset to libnotify
Reporter

Comment 13

5 years ago
(In reply to flying sheep from comment #12)
> there is no problem; the specs are compatible. y’all are too nitpicky.
> 
> “appears as plain text” means “no silly angled brackets”, i.e. all tags that
> aren’t supported are to be removed instead of being left in. so we just have
> to do:
> 
> 1. accept HTML markup as notification body
> 2. replace non-whitelisted tags with their contents
> 3. remove non-whitelisted attributes from whitelisted tags
> 4. pass resulting HTML subset to libnotify

The majority of what I said is just going into detail on what those actually mean. (eg. what to whitelist under which circumstances in order to get the best user experience without exceeding what the notification daemon will accept.)

The main other thing I'm proposing is to extract any buttons from the markup before sanitizing so they can be translated into native notification system actions rather than discarded.
Bulk move to Toolkit::Notifications and Alerts

Filter on notifications-and-alerts-component.
Component: XUL Widgets → Notifications and Alerts
Assignee

Comment 15

5 years ago
Posted patch libnotify revert patch (obsolete) — Splinter Review
Let's start with reverting the libnotify removal. Karl, are you okay with this? It's just what we have before...
Attachment #8462446 - Flags: review?(karlt)
Assignee

Updated

5 years ago
Attachment #8462446 - Attachment is patch: true
In the discussione on dev-platform about this no one was against re-introducing libnotify, so I think we can just do it.
Comment on attachment 8462446 [details] [diff] [review]
libnotify revert patch

Sounds like the right place to start at least, but I assume this won't build because nsAlertsIconListener.h no longer exists.
Attachment #8462446 - Flags: review?(karlt)
Attachment #8462446 - Flags: review-
Attachment #8462446 - Flags: feedback+
Assignee

Comment 18

5 years ago
Posted patch patch v.2 (obsolete) — Splinter Review
Thanks, there's an updated one. I didn't notice the missing header in the patch.
Attachment #8462446 - Attachment is obsolete: true
Attachment #8463935 - Flags: review?(karlt)
Comment on attachment 8463935 [details] [diff] [review]
patch v.2

The code looks good, thanks, but unfortunately it can't be checked in until the browser_notification_tab_switching.js test timeout is addressed somehow.
Attachment #8463935 - Flags: review?(karlt) → review+

Comment 21

5 years ago
The patch has been applied to fedora's firefox packages and allmost breaks the webnotifications function because no notification is shown if the notification uses unsupported tags. The only workaround for fedora users ATM is to install the gnotifier extension.
Assignee

Comment 22

5 years ago
The recent problem with the patch here (reproduced by http://www.bennish.net/web-notifications.html for instance) is when the notification contains an image.

When the notification contains an image, as on the page above, the image is loaded by firefox but not decoded. The imgINotificationObserver::FRAME_COMPLETE state in nsAlertsIconListener::Notify() is newer reached thus the notification is not launched.

It works when the notification is launched in imgINotificationObserver::LOAD_COMPLETE but it does not seem to be correct - the decoder is called twice in that case.
Assignee

Comment 23

5 years ago
Posted patch notify images patch (obsolete) — Splinter Review
WIP patch, fixes notifications with embedded images. It forces the decoder run. There's another option here - to wait some time before the icon is loaded. Both are already used on OSX:

http://mxr.mozilla.org/mozilla-central/source/widget/cocoa/OSXNotificationCenter.mm#377
http://mxr.mozilla.org/mozilla-central/source/widget/cocoa/nsMenuItemIconX.mm#315
Assignee

Comment 25

5 years ago
Posted patch send showalert event (obsolete) — Splinter Review
The mochitest is broken (timed out) because it waits to showalert event which libnotify implementation does not send now.
Attachment #8496802 - Flags: review?(karlt)
Comment on attachment 8496802 [details] [diff] [review]
send showalert event

I wasn't sure about dispatching "show" to JS synchronously, which might start a nested event loop, but OSXNotificationCenter::ShowAlertNotification() does this, so I guess it is safe.  Also, Notification::Constructor() queues a new NotificationTask to call ShowAlertNotification(), so this almost complies with the spec requirement to "queue a task to fire an event named show"

https://notifications.spec.whatwg.org/#displaying-notifications
Attachment #8496802 - Flags: review?(karlt) → review+
Assignee

Comment 27

5 years ago
Posted patch image loading (obsolete) — Splinter Review
This patch generally copies the bits from Mac OS implementation - nsMenuItemIconX.mm file. Implicitly requests image decoding, chancel failed request and remove it when it's completed.
Attachment #8494732 - Attachment is obsolete: true
Attachment #8497514 - Flags: review?(karlt)
Comment on attachment 8497514 [details] [diff] [review]
image loading

>+  if (aType == imgINotificationObserver::DECODE_COMPLETE) {
>+    if (mIconRequest && mIconRequest == aRequest) {
>+       mIconRequest->Cancel(NS_BINDING_ABORTED);
>+       mIconRequest = nullptr;
>+    }
>+  }

I assume this is removing resources that are no longer required?
Is there any other consequence of not removing mIconRequest?

StartRequest will only be called once on a single nsAlertsIconListener.
(Otherwise there would be leaks from reusing mNotification.)
That means that the mIconRequest == aRequest check is unnecessary because a
non-null mIconRequest will be aRequest, so please remove the check, but feel
free to add an assertion.

However, I'm concerned that a decode error here may occur before the load
completes.  This Cancel() would prevent receipt of the LOAD_COMPLETE
notification, and so the error would not be detected and the alert not shown.

> nsAlertsIconListener::OnStopRequest(imgIRequest* aRequest)
> {
>+  if (aRequest != mIconRequest)
>+    return NS_ERROR_FAILURE;

This is essentially a check that mIconRequest is non-null.

mIconRequest is always Cancel()ed before being set to null, and, when Cancel()
has been called on imgRequestProxy, it does not dispatch LOAD_COMPLETE.

That makes the null check unnecessary.

>   if (imgStatus == imgIRequest::STATUS_ERROR && !mLoadedFrame) {
>     // We have an error getting the image. Display the notification with no icon.
>     ShowAlert(nullptr);
>+
>+    // And cancel any pending request
>+    if (mIconRequest) {
>+      mIconRequest->Cancel(NS_BINDING_ABORTED);
>+      mIconRequest = nullptr;
>+    }
>   }
> 
>-  if (mIconRequest) {
>-    mIconRequest->Cancel(NS_BINDING_ABORTED);
>-    mIconRequest = nullptr;
>-  }

This change seems sensible sense to me.  I assume LOAD_COMPLETE can arrive
before FRAME_COMPLETE, in which case the request should not be cancelled.

But if LOAD_COMPLETE can arrive before FRAME_COMPLETE, then that means that
the GetImageStatus(&imgStatus) check is performed before decoding of the first
frame occurs, and so will not detect errors in decoding the first frame.
In that situation, no notification will be dispatched.

That makes me suspect that Notify should just listen to DECODE_COMPLETE
instead of LOAD_COMPLETE?

(The "if (mIconRequest)" test is not necessary here either.)

>+  if (NS_FAILED(rv)) return rv;

Put "return rv" on a new line please.  It makes setting breakpoints easier.

>+  // We need to request the icon be decoded (bug 573583, bug 705516).
>+  mIconRequest->StartDecoding();

Are there any particular comments in those bugs that are helpful?
I assume this is just the way that the imgIRequest API works now, so no need
for the comment unless there are any particular issues relevant there.
Attachment #8497514 - Flags: review?(karlt) → review-
Assignee

Comment 29

5 years ago
Posted patch image v.2 (obsolete) — Splinter Review
What about this one? on OnStopRequest are handled network errors, OnStopFrame covers decode failures. Each ShowAlert(nullptr) has it's own Cancel() to make sure we throw the "error" notification only once.

Cancel request is also used to abort multipart images load/decode because we use only first frame for the notification.

AFAIK the referenced bugs (bug 573583, bug 705516) does not contains instructions about explicit decode request, I just grab the line from MacOS X notification implementation, so we can remove it.
Attachment #8497514 - Attachment is obsolete: true
Attachment #8504788 - Flags: feedback?(karlt)
Comment on attachment 8504788 [details] [diff] [review]
image v.2

(In reply to Martin Stránský from comment #29)
> What about this one? on OnStopRequest are handled network errors,
> OnStopFrame covers decode failures.

I don't know the image proxy code well, but the hasImage test in this code
makes me suspect that FRAME_COMPLETE is not dispatched on decode failure.
http://hg.mozilla.org/mozilla-central/annotate/54217864bae9/image/src/imgStatusTracker.cpp#l393

So it still wonder whether LOAD_COMPLETE should be replaced with
DECODE_COMPLETE, but that need not be addressed in this patch.

> AFAIK the referenced bugs (bug 573583, bug 705516) does not contains
> instructions about explicit decode request, I just grab the line from MacOS
> X notification implementation, so we can remove it.

Yes, please remove it.  As far as we know those references are just sending people on a wild goose chase.

>+get_pixbuf_from_img_request(imgIRequest* aRequest)

Please use CamelCase GetPixbufFromImgRequest for consistency in Gecko.

>-  return il->LoadImageXPCOM(imageUri, nullptr, nullptr, nullptr, nullptr,
>+  nsresult rv = il->LoadImageXPCOM(imageUri, nullptr, nullptr, nullptr, nullptr,
>                             this, nullptr, nsIRequest::LOAD_NORMAL, nullptr,
>                             nullptr, getter_AddRefs(mIconRequest));

Please adjust the indentation here.
Attachment #8504788 - Flags: feedback?(karlt) → feedback+
Assignee

Comment 31

5 years ago
Posted patch images v.3 (obsolete) — Splinter Review
Thanks, there's an updated one.
Attachment #8504788 - Attachment is obsolete: true
Attachment #8505368 - Flags: review?(karlt)
Assignee

Comment 32

5 years ago
(In reply to Karl Tomlinson (:karlt) from comment #30)
> (In reply to Martin Stránský from comment #29)
> > What about this one? on OnStopRequest are handled network errors,
> > OnStopFrame covers decode failures.
> 
> I don't know the image proxy code well, but the hasImage test in this code
> makes me suspect that FRAME_COMPLETE is not dispatched on decode failure.
> http://hg.mozilla.org/mozilla-central/annotate/54217864bae9/image/src/
> imgStatusTracker.cpp#l393
> 
> So it still wonder whether LOAD_COMPLETE should be replaced with
> DECODE_COMPLETE, but that need not be addressed in this patch.

MacOS uses a different model - there's a timer which checks if the image is loaded or not and if the img load fails (for any reason), it launches the notification without it.
Attachment #8505368 - Flags: review?(karlt) → review+
Assignee

Comment 33

5 years ago
Posted patch timer.patch (obsolete) — Splinter Review
What about this check? It removes the error handling at LOAD_COMPLETE and checks if the image/alert is ever dispatched in the timer callback. Should cover all kind of errors (decode/network).
Attachment #8505414 - Flags: feedback?(karlt)
Assignee

Comment 34

5 years ago
Try run for the three r+ patches: https://tbpl.mozilla.org/?tree=Try&rev=8897b4e2fe46
Assignee

Comment 35

5 years ago
Posted patch patch v.2 for check-in (obsolete) — Splinter Review
Trunk has an extra parameter aData in nsAlertsService::ShowAlertNotification() which is unused.
Comment on attachment 8505414 [details] [diff] [review]
timer.patch

I'd prefer to send the notification as soon as an error is detected instead of waiting for the timeout.  I prefer the gnome implementation over the cocoa one.

Are network timeouts so long that we need an additional timer here?
Have you noticed any real issues, or is this just addressing the decode-error-after-load issue that I pointed out?
Is this patch related to the browser_notification_tab_switching.js timeout?
Attachment #8505414 - Flags: feedback?(karlt)
Assignee

Comment 37

5 years ago
(In reply to Karl Tomlinson (:karlt) from comment #36)
> Comment on attachment 8505414 [details] [diff] [review]
> timer.patch
> 
> I'd prefer to send the notification as soon as an error is detected instead
> of waiting for the timeout.  I prefer the gnome implementation over the
> cocoa one.

Okay.
 
> Are network timeouts so long that we need an additional timer here?
> Have you noticed any real issues, or is this just addressing the
> decode-error-after-load issue that I pointed out?

That's just addressing the decode-error-after-load issue, no real problems so far.

> Is this patch related to the browser_notification_tab_switching.js timeout?

No, the browser_notification_tab_switching.js timeout (in the latest try) is caused by missing implementation of nsAlertsService::CloseAlert(). But I'm working on that.
Assignee

Comment 38

5 years ago
Posted patch close event implementation (obsolete) — Splinter Review
This one address the timeout during mochitest which tries to close the notification.
Attachment #8509467 - Flags: review?(karlt)
Comment on attachment 8509467 [details] [diff] [review]
close event implementation

>+nsresult
>+nsAlertsIconListener::CloseAlert()
>+{
>+  return (mNotification && notify_notification_close(mNotification, nullptr)) ?
>+          NS_OK : NS_ERROR_FAILURE;
>+}

If mNotification has not yet been created (ShowAlert() not called), then
something needs to prevent it from being created.

The return value is not used, so please make this method return void.

(It's not ideal that close is synchronous, but we can't do much about that.
There's a /* FIXME: make this nonblocking! */ in the code.)

>+#include "nsAlertsIconListener.h"
> #include "nsCOMPtr.h"
>+#include "nsAutoPtr.h"
>+#include "nsCOMArray.h"

>+  nsTArray<nsRefPtr<nsAlertsIconListener> > mAlertListeners;

I don't think it is guaranteed that nsAlertsService::CloseAlert() will be
called, and so this can leak.

Probably make this a non-owning array, and remove the listener from the array
from ~nsAlertsIconListener().

nsAlertsService constructor and destructor are not defined in this header, and
so nsAlertsIconListener never needs to be accessed from definitions in the
header, and so it should be fine to forward declare class nsAlertsIconListener
instead of including nsAlertsIconListener.h.
Attachment #8509467 - Flags: review?(karlt) → review-
If you like, parts 2 and 3 could be pushed NPOTB.
Similarly for part 1 if the moz.build and nsGnomeModule.cpp are removed.
Or disable the failing part of the test, and we can start using this.
Comment on attachment 8463935 [details] [diff] [review]
patch v.2

This revert has lost the changes from bug 609784.
Can you reinstate them, please?
Assignee

Comment 43

5 years ago
(In reply to Karl Tomlinson (:karlt) from comment #41)
> Or disable the failing part of the test, and we can start using this.

I work on that bc1 mochitest failure. It looks like the "show" event is not propagated on the try server, but works fine when I run it locally....still investigating.
Assignee

Comment 44

5 years ago
The mochitest fails because the notify server there does not support actions and fallback to xul alerts does not work apparently.
Assignee

Comment 45

5 years ago
Posted patch notify fallback to XUL (obsolete) — Splinter Review
This patch should handle fallback to XUL notifications when system one fails from various reasons. Recent code suppresses the notification completelly.

Try: https://tbpl.mozilla.org/?tree=Try&rev=02c1cc681ab6
Assignee

Comment 46

5 years ago
Comment on attachment 8516720 [details] [diff] [review]
notify fallback to XUL

Doug, since you reviewed the code in Bug 782211, can you check this notify fallback please or point me to someone else? Thanks!
Attachment #8516720 - Flags: review?(dougt)
Attachment #8516720 - Flags: review?(dougt) → review+
Assignee

Comment 47

5 years ago
Posted patch images v.3 for check-in (obsolete) — Splinter Review
Fixes param type change in LoadImageXPCOM() from nullptr to 0 which means a default image load policy.
Assignee

Comment 49

5 years ago
Posted patch images v.4 for check-in (obsolete) — Splinter Review
Unbitrotten patch (due to Bug 1084136).
Attachment #8521292 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8521359 - Flags: checkin?
Assignee

Updated

5 years ago
Attachment #8516720 - Flags: checkin?
Assignee

Updated

5 years ago
Attachment #8506090 - Flags: checkin?
Assignee

Updated

5 years ago
Attachment #8496802 - Flags: checkin?
Assignee

Updated

5 years ago
Attachment #8505414 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Keywords: checkin-needed
Attachment #8505368 - Attachment is obsolete: true
Attachment #8463935 - Attachment is obsolete: true
Attachment #8509467 - Attachment is obsolete: true
Assignee: nobody → stransky
Comment on attachment 8496802 [details] [diff] [review]
send showalert event

Also, please stick to just using checkin-needed and marking obsolete patches as such.
Attachment #8496802 - Flags: checkin?
Attachment #8506090 - Flags: checkin?
Attachment #8516720 - Flags: checkin?
Attachment #8521359 - Flags: checkin?
Comment on attachment 8506090 [details] [diff] [review]
patch v.2 for check-in

Bug 609784 was already fixed, so please revert to the nsSystemAlertsService version.  i.e. fold attachment 713802 [details] [diff] [review] into this patch.
Attachment #8506090 - Flags: review-
Assignee

Comment 53

5 years ago
Updated close notification patch. I'll update it for Bug 609784 (the rename) later, when the rename is checked in.

> If mNotification has not yet been created (ShowAlert() not called), then
> something needs to prevent it from being created.

The mNotification is just a paint pointer so null check here is enough.

> Probably make this a non-owning array, and remove the listener from the array
> from ~nsAlertsIconListener().

why non-owning array? I added .Clear() to the destructor so all remaining elements are removed in destructor.
Attachment #8522234 - Flags: feedback?(karlt)
Comment on attachment 8522234 [details] [diff] [review]
notification close

(In reply to Martin Stránský from comment #53)
> > If mNotification has not yet been created (ShowAlert() not called), then
> > something needs to prevent it from being created.
> 
> The mNotification is just a paint pointer so null check here is enough.

The null check is enough to prevent the close.

However, ShowAlert() is called asynchronously, and so may not be called until
after CloseAlert() has been called.

> > Probably make this a non-owning array, and remove the listener from the array
> > from ~nsAlertsIconListener().
> 
> why non-owning array? I added .Clear() to the destructor so all remaining
> elements are removed in destructor.

I assume the nsSystemAlertsService is not destroyed until the application
shuts down.  So there is still a leak until shutdown.

The other changes are good.
Attachment #8522234 - Flags: feedback?(karlt) → feedback+
Assignee

Comment 55

5 years ago
A complete patch with rename from Bug 609784.

Try build https://tbpl.mozilla.org/?tree=Try&rev=d44ea7b77bb1 seems to be clear (at least unrelated to the notification failures which was caused by missing fixes from Bug 609784).
Attachment #8496802 - Attachment is obsolete: true
Attachment #8506090 - Attachment is obsolete: true
Attachment #8516720 - Attachment is obsolete: true
Attachment #8521359 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/616a7e93b73e
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla36

Updated

4 years ago
Depends on: 1144693

Updated

4 years ago
Depends on: 1162788

Updated

4 years ago
Flags: needinfo?(wchen)

Comment 58

4 years ago
Was there any reason you guys used the same CID as b2g system alerts service?
If you create a notification in b2g-desktop that runs over a gnome platform it gets a weird behavior and ends up falling back into using xul alerts.
Flags: needinfo?(stransky)
Flags: needinfo?(karlt)
The libnotify service was using NS_SYSTEMALERTSSERVICE_CID before the b2g system alerts service.

I suspect they are each using the correct CID, but b2g-desktop probably shouldn't be building the more than one system alerts service.
Flags: needinfo?(karlt)

Comment 60

4 years ago
Great that native notifications are possible again, but they don't appear on a significant proportion of occasions so not really so useful.  Is there a bug for this?
Assignee

Updated

3 years ago
Flags: needinfo?(stransky)

Comment 61

3 years ago
This bug is marked as fixed, but I don't think I've actually ever seen libnotify-based notifications from Firefox. Both in a default GNOME setup and my i3 setup, the notifications overlap with parts of the system UI and just generally don't integrate very well.

Is libnotify support still present? If it is, is there a way to force its usage over the Firefox-specific notification system? If not, is there another bug report about that already, or should this one maybe be reopened?
Reporter

Comment 62

3 years ago
(In reply to Jonas Platte from comment #61)
> This bug is marked as fixed, but I don't think I've actually ever seen
> libnotify-based notifications from Firefox. Both in a default GNOME setup
> and my i3 setup, the notifications overlap with parts of the system UI and
> just generally don't integrate very well.
> 
> Is libnotify support still present? If it is, is there a way to force its
> usage over the Firefox-specific notification system? If not, is there
> another bug report about that already, or should this one maybe be reopened?

The "Notify me!" demo button on this MDN page produces a native KDE Plasma notification for me under Firefox Developer Edition 48.0a2 and has done so for months if not years. What does it do for you?

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

Comment 63

3 years ago
Yes, libnotify support is present, but buggy.  It disappeared for a while (Firefox 20 - 35?) in favour of the hideous xul notification box.

Comment 64

3 years ago
This is how it looks for me, both in i3 and in gnome-shell: https://postimg.org/image/4eji9x8sx/
Said bugginess is the reason I'm stripping libnotify support from Arch's packages; mostly because of bug 1236150 (which prevents many notifications from appearing at all) but also bug 1236153.

Comment 66

3 years ago
(In reply to Jan Steffens from comment #65)

That explains why I have only seen the XUL notifications so far; I'm using arch linux.
Thanks for pointing out the relevant bugs (and for being an arch package maintainer)! :)

Comment 67

3 years ago
so this is why i don’t see them!

good reasons though, sadly :( i was about to say “better buggy notifications than butt-ugly ones”, but if some users don’t see some notifications at all, that’s bad.

https://github.com/mkiol/GNotifier works for the time being, though.

Updated

2 years ago
See Also: → 1418287
You need to log in before you can comment on or make changes to this bug.