When an application triggers a notification and the application gets closed, further activation of the notification will trigger a 'notification' system message that can be caught in the app using the mozSetMessageHandler(). The handler defined this way gets called with an object in parameter. This object is simply built with the original notification properties. This is done at https://hg.mozilla.org/mozilla-central/file/b980c2dee2e7/b2g/chrome/content/shell.js#l782, so one can see that we resend: title, body, imageURL. Which is matching the old notification API. In case of a notification sent from the new API, we are missing at least: dir, lang and id fields. I did some debug in shell.js, and this is what we get from low level: I/GeckoDump( 3722): XXX alert_receiveMessage: data={"imageURL":"app://communications.gaiamobile.org/dialer/style/icons/Dialer_60.png","title":"Missed call","text":"From +336xxxxxxx","uid":"app://communications.gaiamobile.org/manifest.webapp#notag:{33e828f5-0bde-4b5f-9f97-6228ea76adbf}","details":{"dir":"auto","id":"app://communications.gaiamobile.org/manifest.webapp#notag:{33e828f5-0bde-4b5f-9f97-6228ea76adbf}","lang":"","manifestURL":"app://communications.gaiamobile.org/manifest.webapp","textClickable":true}}

May I ask Julien and Vivien your feedback on this ? Since you seem to be the last ones to have hacked around this part of shell.js :)

(In reply to Alexandre LISSY :gerard-majax from comment #1) > May I ask Julien and Vivien your feedback on this ? Since you seem to be the > last ones to have hacked around this part of shell.js :) What is the question ?

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #2) > (In reply to Alexandre LISSY :gerard-majax from comment #1) > > May I ask Julien and Vivien your feedback on this ? Since you seem to be the > > last ones to have hacked around this part of shell.js :) > > What is the question ? Should the object passed to the handler of the system message notification be a better copy of the original notification ? Or do we want to keep it as it is currently ?

I think you should discuss about this with Michael and/or Kyle. The Notification subsystem is essentialy un-owned so it's really great to see you working on this. You (as the System Front end team) should take ownership over this and take decisions ;) Now, my own take on this is that we didn't use to store the notification, and maybe we do now (see bug 899574) although Mike will know better. Also, it's better to have a real question. Which feedback do you ask on what? :)

IMHO the only reason why the object was stripped down is because the old API was not providing the useful informations. Then if you want to forward more data because the new API has more data, that makes sense to me.

Thanks for your feedback.

After working on Dialer notification migration, it turns out since we can use Notification.get(), this issue might not be that important for now.

Sorry about the late response. I agree that the system message should have all the relevant information for the new API. Also, how is Notification.get() a workaround here if we don't even send the "tag" property in the system message? ie, how do we know which notification was clicked when the info we do send (title, body, and imageURL) are not guaranteed to be unique?

I think this is application-specific code. In the SMS app, we have a unique imageURL, for example. Yes, this is a hack.

(In reply to Michael Henretty [:mhenretty] from comment #8) > Sorry about the late response. I agree that the system message should have > all the relevant information for the new API. > > Also, how is Notification.get() a workaround here if we don't even send the > "tag" property in the system message? ie, how do we know which notification > was clicked when the info we do send (title, body, and imageURL) are not > guaranteed to be unique? Excellent. As we would like to use the 'tag' element in WapPush, I'm starting to work on this.

(In reply to Alexandre LISSY :gerard-majax from comment #10) > Excellent. As we would like to use the 'tag' element in WapPush, I'm > starting to work on this. +1 BTW the e-mail app is already using the |tag| field so I wonder if they did encounter some bugs related to that.

Hm. The interface http://hg.mozilla.org/mozilla-central/file/9409405e0739/dom/interfaces/notification/nsIDOMDesktopNotification.idl references AppNotificationServiceOptions http://hg.mozilla.org/mozilla-central/file/124d80d7556d/dom/webidl/AppNotificationServiceOptions.webidl which is not containing any tag at all :(

(In reply to Alexandre LISSY :gerard-majax from comment #12) > Hm. > > The interface > http://hg.mozilla.org/mozilla-central/file/9409405e0739/dom/interfaces/ > notification/nsIDOMDesktopNotification.idl references > AppNotificationServiceOptions > http://hg.mozilla.org/mozilla-central/file/124d80d7556d/dom/webidl/ > AppNotificationServiceOptions.webidl which is not containing any tag at all > :( This is a major meh :-/ The spec draft mandates the |tag| property to be supported, it even gives examples on how to use it: http://www.w3.org/TR/notifications/#tags-example

(In reply to Gabriele Svelto [:gsvelto] from comment #13) > (In reply to Alexandre LISSY :gerard-majax from comment #12) > > Hm. > > > > The interface > > http://hg.mozilla.org/mozilla-central/file/9409405e0739/dom/interfaces/ > > notification/nsIDOMDesktopNotification.idl references > > AppNotificationServiceOptions > > http://hg.mozilla.org/mozilla-central/file/124d80d7556d/dom/webidl/ > > AppNotificationServiceOptions.webidl which is not containing any tag at all > > :( > > This is a major meh :-/ The spec draft mandates the |tag| property to be > supported, it even gives examples on how to use it: > > http://www.w3.org/TR/notifications/#tags-example It's only in the case of system messages :)

First WIP patch. As far as I could test in the dialer handler of the system notification, I now have access to the tag value.

(In reply to Gabriele Svelto [:gsvelto] from comment #13) > (In reply to Alexandre LISSY :gerard-majax from comment #12) > > Hm. > > > > The interface > > http://hg.mozilla.org/mozilla-central/file/9409405e0739/dom/interfaces/ > > notification/nsIDOMDesktopNotification.idl references > > AppNotificationServiceOptions > > http://hg.mozilla.org/mozilla-central/file/124d80d7556d/dom/webidl/ > > AppNotificationServiceOptions.webidl which is not containing any tag at all > > :( > > This is a major meh :-/ The spec draft mandates the |tag| property to be > supported, it even gives examples on how to use it: > > http://www.w3.org/TR/notifications/#tags-example Yeah, it's bad. The problem stems from the fact that when we implemented the new Web Notifications API, we re-used a lot of the scaffolding from the old DesktopNotifications API. My guess is that the system message case (which only happens if the app process is killed before a notification event fires), is enough of an edge case that is was never properly updated for the new API. In any case, its good we are fixing this now.

Michael, I've searched the whole morning without finding anything: do we have absolutely no test at all against the notification system message mechanism ?

I've sent the patch to try, https://tbpl.mozilla.org/?tree=Try&rev=db3450d6db9a

(In reply to Alexandre LISSY :gerard-majax from comment #17) > Michael, I've searched the whole morning without finding anything: do we > have absolutely no test at all against the notification system message > mechanism ? I don't know of any. We did have the workings for a gaia integration test that makes sure clicking on a notification will launch an app (which I believe would use the system mechanism), but it is blocked by OOP afaict: https://github.com/mozilla-b2g/gaia/blob/5e3a70295bb3c242228beeaca85fc4344d8e2538/apps/system/test/marionette/notification_launch_app.js#L36 In any case, my opinion is we need tests for this up in gaia more so than in the platform right now.

(In reply to Michael Henretty [:mhenretty] from comment #19) > (In reply to Alexandre LISSY :gerard-majax from comment #17) > > Michael, I've searched the whole morning without finding anything: do we > > have absolutely no test at all against the notification system message > > mechanism ? > > I don't know of any. We did have the workings for a gaia integration test > that makes sure clicking on a notification will launch an app (which I > believe would use the system mechanism), but it is blocked by OOP afaict: > https://github.com/mozilla-b2g/gaia/blob/ > 5e3a70295bb3c242228beeaca85fc4344d8e2538/apps/system/test/marionette/ > notification_launch_app.js#L36 > > In any case, my opinion is we need tests for this up in gaia more so than in > the platform right now. Thanks. Then I'll flip the review switch on this patch. If you can share a bug number for this OOP thing, we might be able to make it.

Follow up opened as bug 958654 for dealing with testing the content of the system message.

(In reply to Alexandre LISSY :gerard-majax from comment #18) > I've sent the patch to try, > https://tbpl.mozilla.org/?tree=Try&rev=db3450d6db9a FYI Try is green.

Comment on attachment 8357796 [details] [diff] [review] 0001-Bug-951150-Sync-notification-system-message-with-Not.patch LGTM. Ran this against the notification integration tests in gaia, and it looks good.

(In reply to Michael Henretty [:mhenretty] from comment #23) > Comment on attachment 8357796 [details] [diff] [review] > 0001-Bug-951150-Sync-notification-system-message-with-Not.patch > > LGTM. Ran this against the notification integration tests in gaia, and it > looks good. Sure, I'm rebuilding with my patch right now to run gaia integration tests.

As far as I can say, only system and email app have integration tests for notification. I ran both, and nothing is broken \o/

I think the SMS app has some as well, but part of the Python-based Integration tests (as opposed to the JS-based Integration tests).