[Usage] MessageHandler.js add method to send notifications

RESOLVED FIXED in Firefox OS v2.2

Status

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mai, Assigned: mai)

Tracking

unspecified
2.2 S6 (20feb)
x86_64
Windows 7

Firefox Tracking Flags

(b2g-v2.2 fixed, b2g-master fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
Adding a method to send notifications, we increase the readibility the code and removing duplicate code
(Assignee)

Updated

4 years ago
Assignee: nobody → marina.rodrigueziglesias
(Assignee)

Comment 1

4 years ago
Created attachment 8557076 [details] [review]
patch v1.0

Hi Salva,
would you mind to review the patch?
Regards
Attachment #8557076 - Flags: review?(salva)
Please make sure that you properly handle:
 - closing of the notification
 - notifications resent after reboot
Flags: needinfo?(mhenretty)
(In reply to Alexandre LISSY :gerard-majax from comment #2)
> Please make sure that you properly handle:
>  - closing of the notification
>  - notifications resent after reboot

Specifically, when the app is killed after it sent the notification (either through LMK or reboot), you need to set the notification system message handler to be able to launch the app and fetch/close the originating notification. Something like:

navigator.mozSetMessageHandler('notification', function(notificationData) {
  // fetch and close notification
  // launch cost control app
});
Flags: needinfo?(mhenretty)
Comment on attachment 8557076 [details] [review]
patch v1.0

This is a good polish bug and I like the changes but please, review my comments on GitHub. Value which of them are feasible and which not and why.

Furthermore, try to explain me why the addition of the `hashFromNotification` parameter. This seems non-related. If so, please, file another bug and fix it there.

Lastly, provide, if possible some tests to check the notifications we are sending display the proper information.

Ask for my review again and thank you.
Attachment #8557076 - Flags: review?(salva)
(Assignee)

Comment 5

4 years ago
Comment on attachment 8557076 [details] [review]
patch v1.0

Updated the pr with your changes,
Regards,
Mai
Attachment #8557076 - Flags: review?(salva)
Comment on attachment 8557076 [details] [review]
patch v1.0

r+ once you address my latest comments on GitHub.
Attachment #8557076 - Flags: review?(salva) → review+
(Assignee)

Comment 7

4 years ago
Master: eddf2fd8b75624557d59bec7f43ba1ab4e462efc
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Comment 8

4 years ago
Comment on attachment 8557076 [details] [review]
patch v1.0

[Approval Request Comment]
On this bug, we remove the hack hiding the message class in the icon URL and ensure the notification is closed.
[Bug caused by] (feature/regressing bug #): Feature
[User impact] if declined: Some notifications will not close after be opened.
[Testing completed]: yes
[Risk to taking this patch] (and alternatives if risky): Low risk
[String changes made]: No
Attachment #8557076 - Flags: approval-gaia-v2.2?(release-mgmt)

Updated

4 years ago
Attachment #8557076 - Flags: approval-gaia-v2.2?(release-mgmt) → approval-gaia-v2.2+
v2.2: https://github.com/mozilla-b2g/gaia/commit/ac797d05f23bf406aca140c7e6e68e5bb8c1f5a9
status-b2g-v2.2: --- → fixed
status-b2g-master: --- → fixed
Target Milestone: --- → 2.2 S6 (20feb)
You need to log in before you can comment on or make changes to this bug.