910915 - [shell.js] New style notifications don't trigger system messages

Status: VERIFIED FIXED

(Firefox OS Graveyard :: General, defect)

Comment 1

[James Lal [:lightsofapollo]]

Attachment: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/11850

Pointer to Github pull-request

Comment 2

[James Lal [:lightsofapollo]]

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.

Comment 3

[James Burke [:jrburke]]

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.

Comment 4

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Looks good to me.

Comment 5

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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.

Comment 6

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

There's also a change this collides with bug 908978

Comment 7

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v1) - WIP: Remove old style notification check in shell.js

Comment 8

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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.

Comment 9

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v2) - WIP: Make sure new notifications also filter through AppNotification Service

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.

Comment 10

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

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

Comment 11

[Caio Lima(:caiolima)]

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.

Comment 12

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v3) - Add Notification ID Field to AppNotificationService Functions

Comment 13

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 2 (v1) - Add capabilities for new notifications to b2g shell/alertsservice

Comment 14

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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.

Comment 15

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 3 (v1) - Gaia UI Test for Notification App Opening

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

Comment 16

[Julien Wajsberg [:julienw]]

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 17

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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 18

[[:fabrice] Fabrice Desré]

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.

Comment 19

[[:fabrice] Fabrice Desré]

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

Comment 20

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

https://github.com/mozilla-b2g/gaia/commit/128694d47338554b1350221255ca7729a39b2155

Comment 22

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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

Comment 23

[Ryan VanderMeulen [:RyanVM]]

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

Comment 24

[Julien Wajsberg [:julienw]]

This probably doesn't work :)

cf https://github.com/mozilla-b2g/gaia/commit/128694d47338554b1350221255ca7729a39b2155#commitcomment-4085947

Comment 25

[Julien Wajsberg [:julienw]]

I mean, the Gaia UITests patch.

Comment 26

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Worked fine for me on my phone?

Comment 27

[Jason Smith [:jsmith]]

Verified per completion of initial test pass.