Pointer to Github pull-request

Comment on attachment 797565 [details] Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/11850 So I would clean this up but I wanted to submit something that was failing that _should_ work for this case.

Email is currently using notification_helper, but we want to switch to the `new Notification` model for email notifications so that tags for notifications for updates work (bug 892522 -- a top priority feature for v1.2). However, this issue is preventing the switch since clicking on the notification generated by `new Notification` does not seem to result in the email app getting a signal that the notification was clicked. For email, it does not bind an onclick or onclose handler, but relies on the listener passed to navigator.mozSetMessageHandler('notification', ...) to receive events about activations via a notification.

Looks good to me.

In shell.js line 687, the handleEvent function of AlertsHelper, we only trigger apps opening for old notifications. We need to cleanup the handling for this to deal with both old and new notifications.

There's also a change this collides with bug 908978

Getting the following error when trying to send system messages when notifications created with new API are tapped: [Exception... "Component returned failure code: 0x804b000a (NS_ERROR_MALFORMED_URI) [nsIIOService.newURI]" nsresult: "0x804b000a (NS_ERROR_MALFORMED_URI)" location: "JS frame :: chrome://browser/content/shell.js :: alert_handleEvent :: line 707" data: no] This relates to the following call: gSystemMessenger.sendMessage("notification", { clicked: (detail.type === "desktop-notification-click"), title: listener.title, body: listener.text, imageURL: listener.imageURL }, Services.io.newURI(listener.target, null, null), Services.io.newURI(listener.manifestURL, null, null) ); Both listener.target and listener.manifestURL are undefined here.

This patch mostly works. The only problem is detecting whether we need to send a system message or just ping the observer. The check for listener.mm in shell.js line ~733 doesn't seem to work for new style notifications.

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #9) > This patch mostly works. The only problem is detecting whether we need to > send a system message or just ping the observer. The check for listener.mm > in shell.js line ~733 doesn't seem to work for new style notifications. What about doing it the way Desktop Notifications work, which is ping the observer first and if that throws an exception (meaning the app is closed), then send the system message to wake it up?

qDot, did you verified with your code the "Notification.close()" is working? I remeber that I've tested it when I was debuging in bug https://bugzilla.mozilla.org/show_bug.cgi?id=908978 and using the nsIAppNotificationService.

This will have to land without integration tests, as the way we see if an app is open (checking the validity of the message manager for the process) only works in OOP, which we can't do on desktop. I've added manual tests to the UI test.

Adding a UI test since we can't test with integration yet. Note this test will also only work on the phone.

Comment on attachment 802228 [details] [diff] [review] Patch 3 (v1) - Gaia UI Test for Notification App Opening r=me with the github comments addressed

Comment on attachment 797565 [details] Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/11850 Obsoleting for now, as this breaks on desktop due to OOP issues.

Comment on attachment 801659 [details] [diff] [review] Patch 2 (v1) - Add capabilities for new notifications to b2g shell/alertsservice Review of attachment 801659 [details] [diff] [review]: ----------------------------------------------------------------- ::: b2g/chrome/content/shell.js @@ +731,5 @@ > + try { > + listener.observer.observe(null, topic, listener.cookie); > + } catch (e) { } > + } > + else { nit: } else { ::: b2g/components/AlertsService.js @@ +73,5 @@ > + if(aId == "") { > + uid = "app-notif-" + uuidGenerator.generateUUID(); > + } else { > + uid = aId; > + } Replace by: let uid = (aId == "") ? "app-notif-" + uuidGenerator.generateUUID() : aId; @@ +75,5 @@ > + } else { > + uid = aId; > + } > + > + dump("UID IS " + uid + "\n"); Nit: remove that.

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #17) > Comment on attachment 797565 [details] > Pointer to Github pull request: > https://github.com/mozilla-b2g/gaia/pull/11850 > > Obsoleting for now, as this breaks on desktop due to OOP issues. Note that we are turning on OOP on b2g desktop Linux in bug 914584.

https://hg.mozilla.org/integration/b2g-inbound/rev/114983525d73 https://hg.mozilla.org/integration/b2g-inbound/rev/576142cafac3

https://hg.mozilla.org/mozilla-central/rev/114983525d73 https://hg.mozilla.org/mozilla-central/rev/576142cafac3

This probably doesn't work :) cf https://github.com/mozilla-b2g/gaia/commit/128694d47338554b1350221255ca7729a39b2155#commitcomment-4085947

I mean, the Gaia UITests patch.

Worked fine for me on my phone?

Verified per completion of initial test pass.