Integrate Firefox notifications with notify-osd bling

You should document somewhere what the difference is between "@mozilla.org/system-alerts-service;1" and "@mozilla.org/alerts-service;1" (in particular, why we need both).

+ if (imgStatus == imgIRequest::STATUS_ERROR) {
+ // We have an error getting the image. Display the notification with no icon.
+ ShowAlert(NULL);
+ }

Should check mLoadedFrame here; if an error occurs after the first frame we could show the alert twice!

Did you copy the code for getting the application name from somewhere else? Can we share it somehow?

+ nsCAutoString mAlertTitle;
+ nsCAutoString mAlertText;

nsCString

+ PRBool mLoadedFrame;

PRPackedBool

+ nsCOMPtr<nsAlertsIconListener> alertListener = new nsAlertsIconListener();
+ return alertListener->InitAlertAsync(aImageUrl, aAlertTitle, aAlertText);

OOM check

Needs build-system review from Ted

(From update of attachment 353279)
> { "GnomeVFS Service",
> NS_GNOMEVFSSERVICE_CID,
> NS_GNOMEVFSSERVICE_CONTRACTID,
>- nsGnomeVFSServiceConstructor }
>+ nsGnomeVFSServiceConstructor },
>+#ifdef MOZ_ENABLE_LIBNOTIFY
>+ { "Gnome Alerts Service",
>+ NS_SYSTEMALERTSSERVICE_CID,
>+ NS_SYSTEMALERTSERVICE_CONTRACTID,
>+ nsAlertsServiceConstructor },
>+#endif
> };

What if MOZ_ENABLE_LIBNOTIFY isn't #define'd... that trailing comma will be there. Will that cause a compilation error?

No. Notice how a trailing comma is on the end even if it is defined. Its all over the code to make it easier to add elements.

Created an attachment (id=353295)
Patch 2

Fix roc's comments.

(From update of attachment 353295)
Ted, I need your approval on the build system changes.

... make sure that --disable-compile-environment ignores the libnotify test. I'd do so even if it is currently not switched on by default.

(From update of attachment 353295)
Looks fine. As Pike said, that whole block in configure should really be inside
if test -z "$SKIP_LIBRARY_CHECKS";
fi

In fact, the dbus stuff up above (and maybe some more) should go inside a block like that as well.

Created an attachment (id=355343)
Patch 3

This adds support for that compile time check.

Just a note, this patch will be off by default until the build machines get libnotify dev headers installed (which I will talk to IT about after landing).

Pushed 9891b174d871

why did this end up in toolkit/system/gnome and not toolkit/components/alerts/src/gnome (similar to how the mac implementation is)?

So it can live in the dynamically linked libmozgnome, so we can link to libnotify and if it's not there the browser will still work.

Thanks! Would be nice if we'd get this on 1.9.1

Yes it would be, but since string freeze has passed (or if not, coming up extremely soon) I don't know if it will make it considering quite a few strings may have to be changed to remove the implied "click me for further details" action.

I just hope 1.9.2 will be quicker than this "quick" 1.9.1 release cycle was :)

1.9.0 backport would require the backport in bug 393936 -> depends.

(From update of attachment 355343)
improvements on the linux desktop integration front seems to justify to request for 1.9.1 approval here. Even more so as ubuntu is getting a new notification system: http://www.markshuttleworth.com/archives/253

Thanks for considering this.

(From update of attachment 355343)
Clearing the nomination to take this in 1.9.1, the lack of callback support feels like a usability regression, and I don't think we want to do that now, and probably ever. Need to sync with the right people to find a way forward.

So, this never had ui-review, and it's clearly making user experience changes. I'm inclined to back it out until it's not a UX regression, or until we've decided that the regression is worth the integration win.

I'm completely unconvinced right now that the current behaviour is an improvement. I've read https://wiki.ubuntu.com/NotificationDesignGuidelines (now that Ubuntu made the spec public) and I've discussed it with the designers. I think they're wrong, and I don't think we win by following platform specs if the platform specs are less useful.

I have no particular opinion about this, but I've CCed a few folks who might.

Actually, the API supports callbacks, but for some reason they weren't implemented? Bah, this is just multiple layers of wrong.

So, it's possible that some notification servers won't support actions. It's silly, but it's possible. We should implement actions in toolkit, or back this out (as much as the ugly toaster is something I want to kill).

(In reply to comment #18)
> I'm completely unconvinced right now that the current behaviour is an
> improvement. I've read https://wiki.ubuntu.com/NotificationDesignGuidelines
> (now that Ubuntu made the spec public) and I've discussed it with the
> designers. I think they're wrong, and I don't think we win by following
> platform specs if the platform specs are less useful.

OK, but don't they get some leeway to be wrong? Not unlimited leeway, but some?

Also, is this a toolkit issue or a product issue? We could easily enable/disable native notifications per-product.

(In reply to comment #23)
> Also, is this a toolkit issue or a product issue? We could easily
> enable/disable native notifications per-product.
IMO this is a toolkit issue. This patch changes the behavior of how something works consistently on all platforms to something that isn't consistent (namely, callback support). A toolkit peer probably would have flagged that, but one didn't even look at this patch...

I think this is a toolkit issue. We shouldn't partially implement libnotify and have nsIAlertsService be less capable on Linux. If distros disable actions in their notification server, that's their call, but I don't think our impl should ignore callbacks.

As a note, even Ubuntu's implementation supports actions, it's just not "as shiny" right now. We should let the caller decide whether they want a callback or not.

OK. I believe at the time this was implemented Ubuntu was planning to not support callbacks. I'm glad this has changed.

I can work on adding callback support, libnotify seems to have a good API for this at first glance.

Created an attachment (id=373102)
Add callbacks, and turn it on

This adds support for callbacks, and seems to work well.

I can still request ui-r if necessary (since I want to flip the switch now for 1.9.2) but with the callback support implemented as is in this patch, it works just like the toaster; clicking anywhere in the bubble (except the X) activates the callback, and the bubble disappears after the system default delay.

I'm still on ubuntu intrepid, I don't know what's happening for the final jaunty in the area of notifications (although jaunty will be well obsolete by the time 1.9.2 is even close to release...)

> This adds support for callbacks, and seems to work well.

If we do take this, and get proper callback support, I'm wondering if we'd still consider it for 1.9.1, since it would let me switch the Thunderbird notifications over to use it.

Can we get some tests too please? Pretty please, with sugar, and spice, and everything nice?

I think it's almost certainly too late for 1.9.1. A set of tests would be great in the service of changing that. There isn't a matter of "switching anything over" since it's the same API, it'll just be cleaner/nicer/better integrated.

As for ui-review, if it's now identical for the interaction model, and isn't requiring string changes because the behaviour is different, then ui-r=me.

+ nsAutoString alertCookie;

nsString (you normally don't want to use nsAuto* in structs)

+ nsAutoString mAlertCookie;

Same here

Instead of creating NotifyCallbackInfo, why don't you make the notify_ callbacks take the nsAlertsIconListener as their closure-data?

(In reply to comment #31)
> Can we get some tests too please? Pretty please, with sugar, and spice, and
> everything nice?

Litmus is the only way I can think of testing this, as this calls into the native toolkit and any tests I can think of would be no different to what we have for the toaster.

(In reply to comment #33)
> Instead of creating NotifyCallbackInfo, why don't you make the notify_
> callbacks take the nsAlertsIconListener as their closure-data?

I guess so, I was worried the nsAlertsIconListener would be destroyed before the callback happened but I suppose that's what XPCOM is for...

(In reply to comment #34)
> Litmus is the only way I can think of testing this, as this calls into the
> native toolkit and any tests I can think of would be no different to what we
> have for the toaster.
I don't even think we have those...

> I guess so, I was worried the nsAlertsIconListener would be destroyed before
> the callback happened but I suppose that's what XPCOM is for...

Right, NS_ADDREF the nsAlertsIconListener before you set the callback and NS_RELEASE it in the destroy callback.

Precisely my plan, yes. I'm just trying to think of a mochitest. I could probably test the alertfinished callback, but the alertclickcallback listener seems impossible to test due to the myriad of notification systems we now use.

Created an attachment (id=373274)
Callbacks 2

This includes a mochitest.

What I'm worried about is what happens if Growl is absent. Does the alerts service throw? If not, I'll have to update this patch to make an exception for non Windows and Linux platforms.

Created an attachment (id=373275)
Callback 2.1

Oops, that patch had some cruft in it.

You'll hit two cases with Growl:
1) if growl is not installed, the call to getService will fail (http://mxr.mozilla.org/mozilla-central/source/toolkit/components/alerts/src/mac/nsAlertsService.mm#150)
2) if growl is not running, dispatching the notification will fail (http://mxr.mozilla.org/mozilla-central/source/toolkit/components/alerts/src/mac/nsAlertsService.mm#97)

For the unit test boxes, you'll hit (1)

+ alertTimeout = setTimeout(killAndFail, ALERT_TIMEOUT);

Don't do this. Let the normal mochitest timeout mechanism trigger test failure.

+ nsAlertsIconListener* alert = (nsAlertsIconListener*) user_data;

static_cast

Created an attachment (id=373592)
Callbacks 2.2

Fix review comments.

Wow, that was fast.

(In reply to comment #26)
> As a note, even Ubuntu's implementation supports actions, it's just not "as
> shiny" right now. We should let the caller decide whether they want a callback
> or not.

while this is true, it doesn't mean that those actions should be used.

In general, apps should not add actions if there is no "actions" capability announced by the server through:

/**
 * Returns the capabilities of the notification server.
 *
 * @return A list of capability strings. These strings must be freed.
 */
GList *notify_get_server_caps(void);

So what we can do is two things (or maybe both):

1. omit the actions in system alerts service if the there is no actions support available
2. add a supportsActions method to system alerts component to allow the caller to decide on its own what to do in case there are no actions available on system.

1) has already been discussed and is the reason this bug has been reopened. I don't see what you expect the caller to do in 2). Seems like we should avoid that too. Ideally we'd fall back to our cross-platform implementation if the notification server isn't capable of actions.

(In reply to comment #45)
> 1) has already been discussed and is the reason this bug has been reopened. I

1) might have been discussed, but it wasn't implemented (the implementation just dropped actions no matter what.

> don't see what you expect the caller to do in 2). Seems like we should avoid
> that too. Ideally we'd fall back to our cross-platform implementation if the
> notification server isn't capable of actions.

I think the alertsservice - which currently delegates to the system one - could do that automatic fallback; anyway, in order to decide this, the system alertsservice needs to exports that info first. Thats basically what i meant with 2). Other xulapps could then look up the system alerts service and do their own decision (e.g. if they don't use actions they can just happily use system notification even when no actions are supported)

1. Capabilities of notify-osd (shipped by ubuntu default):
'body', 'body-markup', 'icon-static', 'image/svg+xml', 'x-canonical-private-
synchronous', 'x-canonical-append', 'x-canonical-private-icon-only', 'x-canonical-truncation', 'private-synchronous', 'append', 'private-icon-only', 'truncation'

2. Capabilities of notification-daemon:
'actions', 'body', 'body-hyperlinks', 'body-markup', 'icon-static'

(In reply to comment #46)
> 1) might have been discussed, but it wasn't implemented (the implementation
> just dropped actions no matter what.

Right, but it's bad for the same reason. We can't drop callbacks like the original implementation did it, and we also can't do it depending on the notification server -- unless we care only about a know set of servers and are confident that they'll continue to support actions.

(In reply to comment #48)
> (In reply to comment #46)
> > 1) might have been discussed, but it wasn't implemented (the implementation
> > just dropped actions no matter what.
>
> Right, but it's bad for the same reason. We can't drop callbacks like the
> original implementation did it, and we also can't do it depending on the
> notification server -- unless we care only about a know set of servers and are
> confident that they'll continue to support actions.

but if the server does not announce "actions" capability adding them does not make it better :) ... anyway, i think the right way to move forward would be 2) as in comment 46

One thing we could do is provide the click callback immediately as the alert appears, if the server doesn't support actions. For example, the downloads window will appear when the downloads complete alert is also shown. That would be compatible with our current system but might be annoying in some circumstances.

Comment 46 sounds good in theory I suppose, but changing the interface as well as all its users (not to mention third party extensions!) sounds a little much.

But that could probably be a topic of another bug, after this is checked in?

> One thing we could do is provide the click callback immediately as the alert
> appears, if the server doesn't support actions. For example, the downloads
> window will appear when the downloads complete alert is also shown. That would
> be compatible with our current system but might be annoying in some
> circumstances.

+1 -- we really should have parity with other notification systems we use (e.g. Growl), so that all the existing "click" logic works here too.

(In reply to comment #50)
> One thing we could do is provide the click callback immediately as the alert
> appears, if the server doesn't support actions. For example, the downloads
> window will appear when the downloads complete alert is also shown. That would
> be compatible with our current system but might be annoying in some
> circumstances.

That seems to destroy the idea of lightweight notifications.

(In reply to comment #51)
> But that could probably be a topic of another bug, after this is checked in?

Well, you want to turn it on in this bug, right? Yet notify-osd doesn't seem to
support actions officially. That doesn't seem right to me.

Specifically, we use notifications because we don't want to steal focus.

(In reply to comment #50)
> Comment 46 sounds good in theory I suppose, but changing the interface as well
> as all its users (not to mention third party extensions!) sounds a little much.

This was not exactly the idea I had in mind. AFAIK, currently alertsservice is the only place that decides whether to delegate to system service or not. ATM this decision only depends on whether the component is available or not.

We could extend that check to also consider whether actions are available; if there is no actions support, it would fall back to the xul implemention - thus, preserving the old contract.

However, toolkit apps/extensions that are happy with the actionless approach could explicitly opt-out from that by using a different method and/or by directly looking up/using the system-alerts component.

(In reply to comment #51)
> But that could probably be a topic of another bug, after this is checked in?

The current patch should at least not add actions when the server doesn't announce "actions" capability. That would also be in line with comment 25.

(In reply to comment #56)
> (In reply to comment #51)
> > But that could probably be a topic of another bug, after this is checked in?
>
> The current patch should at least not add actions when the server doesn't
> announce "actions" capability. That would also be in line with comment 25.

I'd rather do this than fallback to XUL, this bug seems like a giant waste if the most popular distribution doesn't benefit from it in any way.

> this bug seems like a giant waste if
> the most popular distribution doesn't benefit from it in any way.

What does this mean? Lack of unity in question of notification (notify-osd, notification-daemon) caused, mostly by Ubuntu, is the problem?

(In reply to comment #58)
> > this bug seems like a giant waste if
> > the most popular distribution doesn't benefit from it in any way.
>
> What does this mean? Lack of unity in question of notification (notify-osd,
> notification-daemon) caused, mostly by Ubuntu, is the problem?

Sort of, yes. I don't see how this can be resolved on our side if the notification server doesn't support actions, but I also want to kill that ugly popup for as many people as humanly possible, and adopt the native library.

I suppose I can just not add the action callback if the server doesn't support them.

(In reply to comment #57)
> > The current patch should at least not add actions when the server doesn't
> > announce "actions" capability. That would also be in line with comment 25.
>
> I'd rather do this than fallback to XUL, this bug seems like a giant waste if
> the most popular distribution doesn't benefit from it in any way.

I'm not sure that I agree with comment 25 there, or maybe I misunderstand it. If the caller wants a callback (which might even be reflected in the notification message) and the server tells us that it doesn't support it, falling back to XUL is the only sane solution, IMHO. I think this needs to be evangelized on the Ubuntu side.

> I think this needs to be evangelized on the Ubuntu side.

Talk to Mark Shuttleworth. It was his idea to make _another_ notification daemon, but with less possibilities (completely without actions).

(In reply to comment #60)
> I suppose I can just not add the action callback if the server doesn't support
> them.

So:

#define NOTIFY_CAPS_ACTIONS_KEY "actions"

GList *server_caps = notify_get_server_caps();
if (g_list_find (server_caps, NOTIFY_CAPS_ACTIONS_KEY))
...

if you do this, consider to cache the result somehow as you would otherwise do another dbus call for each notification.

We should absolutely fall back to XUL if there's no actions support, IMO. I think the Ubuntu system is unnecessarily restrictive, and the design guidelines actually are more damaging than helpful. I guess I need to write up that critique after all...

Let's do the sane thing, that Windows and OS X (via Growl) already do, and we can work to convince Ubuntu of the error of their ways... :)

Created an attachment (id=375563)
Callbacks 3

This falls back to XUL if the server doesn't support actions.

I tried to land this (http://hg.mozilla.org/mozilla-central/rev/99cdecb3734f), but will have to back it out because of this:

checking for libnotify >= 0.4... Package libnotify was not found in the pkg-config search path. Perhaps you should add the directory containing `libnotify.pc' to the PKG_CONFIG_PATH environment variable No package 'libnotify' found
configure: error: Library requirements (libnotify >= 0.4) not met; consider adjusting the PKG_CONFIG_PATH environment variable if your libraries are in a nonstandard prefix so pkg-config can find them.

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1241424892.1241424932.5678.gz

Er, I remember libnotify dev headers being installed on mozilla-central, I'll take another look at the bug.

Bug 473831 was marked fixed, but they mention only i386 packages.
The red only occurred on the 64-bit box, but the builds that happened around the same time on the 32-bit boxes still seem to be going.

linux64 slave now has a libnotify-devel installed, previously only had libnotify. Details in bug 473831 comment #9.

Wonderful, thanks.

http://hg.mozilla.org/mozilla-central/rev/c2ba27e9e639

For some reason, Linux x86-64 mozilla-central hasn't started building yet...

Dear all,
according to https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/610362 , Firefox's notifications don't work with Ubuntu's implementation.

is there something mozilla can do to help solving this ?

(In reply to comment #72)
> Dear all,
> according to https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/610362 ,
> Firefox's notifications don't work with Ubuntu's implementation.
>
> is there something mozilla can do to help solving this ?

Please read through this bug ... We need action callback support for our notifications.