If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

VERIFIED FIXED in Firefox 26

Status

Firefox OS
General
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: lightsofapollo, Assigned: qdot)

Tracking

(Blocks: 1 bug)

unspecified
1.2 FC (16sep)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:koi+, firefox26 fixed)

Details

(Whiteboard: [systemsfe])

Attachments

(3 attachments, 3 obsolete attachments)

Comment hidden (empty)
(Reporter)

Updated

4 years ago
Summary: ' → [shell.js] New style notifications don't trigger system messages
(Reporter)

Comment 1

4 years ago
Created attachment 797565 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/11850

Pointer to Github pull-request
(Reporter)

Comment 2

4 years ago
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.
Attachment #797565 - Flags: feedback?(kyle)
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.
Blocks: 892522
Looks good to me.
Attachment #797565 - Flags: feedback?(kyle) → feedback+
Assignee: nobody → kyle
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.
blocking-b2g: --- → koi?
There's also a change this collides with bug 908978
Created attachment 799269 [details] [diff] [review]
Patch 1 (v1) - WIP: Remove old style notification check in shell.js
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.
Created attachment 800495 [details] [diff] [review]
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.
Attachment #799269 - Attachment is obsolete: true
(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.
Created attachment 801658 [details] [diff] [review]
Patch 1 (v3) - Add Notification ID Field to AppNotificationService Functions
Attachment #800495 - Attachment is obsolete: true
Attachment #801658 - Flags: review?(fabrice)
Created attachment 801659 [details] [diff] [review]
Patch 2 (v1) - Add capabilities for new notifications to b2g shell/alertsservice
Attachment #801659 - Flags: review?(fabrice)
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.

Updated

4 years ago
Component: Gaia → General

Updated

4 years ago
Blocks: 911002
Created attachment 802228 [details] [diff] [review]
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.
Attachment #802228 - Flags: review?(felash)
Comment on attachment 802228 [details] [diff] [review]
Patch 3 (v1) - Gaia UI Test for Notification App Opening

r=me with the github comments addressed
Attachment #802228 - Flags: review?(felash) → review+
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.
Attachment #797565 - Attachment is obsolete: true
Attachment #801658 - Flags: review?(fabrice) → review+
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.
Attachment #801659 - Flags: review?(fabrice) → review+
(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://github.com/mozilla-b2g/gaia/commit/128694d47338554b1350221255ca7729a39b2155
Duplicate of this bug: 908978
https://hg.mozilla.org/integration/b2g-inbound/rev/114983525d73
https://hg.mozilla.org/integration/b2g-inbound/rev/576142cafac3
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED

Updated

4 years ago
Keywords: verifyme
QA Contact: jsmith
https://hg.mozilla.org/mozilla-central/rev/114983525d73
https://hg.mozilla.org/mozilla-central/rev/576142cafac3

Updated

4 years ago
Whiteboard: [systemsfe]
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?

Updated

4 years ago
blocking-b2g: koi? → koi+
status-firefox26: --- → fixed
Target Milestone: --- → 1.2 FC (16sep)
Verified per completion of initial test pass.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.