951150 - Notification system message object is not in sync with DesktopNotification API

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[:gerard-majax]

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}}

Comment 1

[:gerard-majax]

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

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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

Comment 3

[:gerard-majax]

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

Comment 4

[Julien Wajsberg [:julienw]]

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? :)

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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.

Comment 6

[:gerard-majax]

Thanks for your feedback.

Comment 7

[:gerard-majax]

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

Comment 8

[Michael Henretty [:mikehenrty][:mhenretty]]

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?

Comment 9

[Julien Wajsberg [:julienw]]

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

Comment 10

[:gerard-majax]

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

Comment 11

[Gabriele Svelto [:gsvelto]]

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

Comment 12

[:gerard-majax]

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

Comment 13

[Gabriele Svelto [:gsvelto]]

(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

Comment 14

[:gerard-majax]

(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 :)

Comment 15

[:gerard-majax]

Attachment: 0001-Bug-951150-Sync-notification-system-message-with-Not.patch

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.

Comment 16

[Michael Henretty [:mikehenrty][:mhenretty]]

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

Comment 17

[:gerard-majax]

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

Comment 18

[:gerard-majax]

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

Comment 19

[Michael Henretty [:mikehenrty][:mhenretty]]

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

Comment 20

[:gerard-majax]

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

Comment 21

[:gerard-majax]

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

Comment 22

[:gerard-majax]

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

[Michael Henretty [:mikehenrty][:mhenretty]]

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.

Comment 24

[:gerard-majax]

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

Comment 25

[:gerard-majax]

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

Comment 26

[Julien Wajsberg [:julienw]]

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

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/26bedf5c00c9

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/26bedf5c00c9