858919 - Web Notifications support does not integrate with libnotify

Status: RESOLVED FIXED

(Toolkit Graveyard :: Notifications and Alerts, defect)

Description

[Stephan Sokolow]

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

Comment 1

[Marco Castelluccio [:marco]]

What are the plans for libnotify?

Comment 2

[Mike Hommey [:glandium]]

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.

Comment 3

[Stephan Sokolow]

(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

Comment 4

[Philip Chee]

Also see Bug 469880 esp Bug 469880 Comment 47

Comment 5

[Karl Tomlinson (:karlt)]

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

Comment 6

[Stephan Sokolow]

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

Comment 7

[Jan Alexander Steffens [:heftig]]

(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

Comment 10

[drago01]

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.

Comment 11

[Stephan Sokolow]

(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

[flying sheep]

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

Comment 13

[Stephan Sokolow]

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

Comment 14

[Matthew N. [:MattN]]

Bulk move to Toolkit::Notifications and Alerts

Filter on notifications-and-alerts-component.

Comment 15

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

Attachment: libnotify revert patch

Let's start with reverting the libnotify removal. Karl, are you okay with this? It's just what we have before...

Comment 16

[Marco Castelluccio [:marco]]

In the discussione on dev-platform about this no one was against re-introducing libnotify, so I think we can just do it.

Comment 17

[Karl Tomlinson (:karlt)]

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.

Comment 18

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

Attachment: patch v.2

Thanks, there's an updated one. I didn't notice the missing header in the patch.

Comment 19

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

Comment on attachment 8463935 [details] [diff] [review]
patch v.2

Try run: https://tbpl.mozilla.org/?tree=Try&rev=6859fd891aa0

Comment 20

[Karl Tomlinson (: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.

Comment 21

[Heiko Adams]

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.

Comment 22

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

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.

Comment 23

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

Attachment: notify images patch

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

Comment 24

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

Sorry, wrong URL. Correct one for the delayed load is:

http://mxr.mozilla.org/mozilla-central/source/widget/cocoa/OSXNotificationCenter.mm#245

Comment 25

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

Attachment: send showalert event

The mochitest is broken (timed out) because it waits to showalert event which libnotify implementation does not send now.

Comment 26

[Karl Tomlinson (: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

Comment 27

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

Attachment: image loading

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.

Comment 28

[Karl Tomlinson (: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.

Comment 29

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

Attachment: image v.2

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.

Comment 30

[Karl Tomlinson (: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.

Comment 31

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

Attachment: images v.3

Thanks, there's an updated one.

Comment 32

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

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

Comment 33

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

Attachment: timer.patch

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

Comment 34

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

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

Comment 35

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

Attachment: patch v.2 for check-in

Trunk has an extra parameter aData in nsAlertsService::ShowAlertNotification() which is unused.

Comment 36

[Karl Tomlinson (:karlt)]

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?

Comment 37

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

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

Comment 38

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

Attachment: close event implementation

This one address the timeout during mochitest which tries to close the notification.

Comment 39

[Karl Tomlinson (: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.

Comment 40

[Karl Tomlinson (:karlt)]

If you like, parts 2 and 3 could be pushed NPOTB.
Similarly for part 1 if the moz.build and nsGnomeModule.cpp are removed.

Comment 41

[Karl Tomlinson (:karlt)]

Or disable the failing part of the test, and we can start using this.

Comment 42

[Karl Tomlinson (:karlt)]

Comment on attachment 8463935 [details] [diff] [review]
patch v.2

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

Comment 43

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

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

Comment 44

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

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

Comment 45

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

Attachment: notify fallback to XUL

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

Comment 46

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

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!

Comment 47

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

Attachment: images v.3 for check-in

Fixes param type change in LoadImageXPCOM() from nullptr to 0 which means a default image load policy.

Comment 48

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

Try build: https://tbpl.mozilla.org/?tree=Try&rev=9d1c10b49de0

Comment 49

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

Attachment: images v.4 for check-in

Unbitrotten patch (due to Bug 1084136).

Comment 50

[Ryan VanderMeulen [:RyanVM]]

These failures all look relevant to your patches, no?

https://tbpl.mozilla.org/php/getParsedLog.php?id=52436802&tree=Try
https://tbpl.mozilla.org/php/getParsedLog.php?id=52436804&tree=Try
https://tbpl.mozilla.org/php/getParsedLog.php?id=52440035&tree=Try

Comment 51

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8496802 [details] [diff] [review]
send showalert event

Also, please stick to just using checkin-needed and marking obsolete patches as such.

Comment 52

[Karl Tomlinson (:karlt)]

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.

Comment 53

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

Attachment: notification close

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.

Comment 54

[Karl Tomlinson (: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.

Comment 55

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

Attachment: complete libnotify patch for check-in

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

Comment 56

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/616a7e93b73e

Comment 57

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/616a7e93b73e

Comment 58

[Robert Bindar]

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.

Comment 59

[Karl Tomlinson (: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.

Comment 60

[Ian Nartowicz]

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?

Comment 61

[Jonas Platte]

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?

Comment 62

[Stephan Sokolow]

(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

[Ian Nartowicz]

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

[Jonas Platte]

This is how it looks for me, both in i3 and in gnome-shell: https://postimg.org/image/4eji9x8sx/

Comment 65

[Jan Alexander Steffens [:heftig]]

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

[Jonas Platte]

(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

[flying sheep]

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.