Page MenuHomePhabricator
Differential D13131

Fix sound issue for peer-based notifications
ClosedPublic
Actions

Authored by marcin on Wed, Aug 21, 4:18 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Sep 13, 1:20 AM
Unknown Object (File)
Tue, Sep 10, 11:11 PM
Unknown Object (File)
Mon, Sep 9, 7:35 PM
Unknown Object (File)
Mon, Sep 9, 3:14 PM
Unknown Object (File)
Mon, Sep 9, 7:56 AM
Unknown Object (File)
Sun, Sep 8, 6:48 PM
Unknown Object (File)
Sat, Sep 7, 2:18 PM
Unknown Object (File)
Sat, Sep 7, 12:37 PM
View All 39 Files
Subscribers
ashoat

Details

Reviewers
tomek
kamil
Commits
rCOMM44b5ff880b3a: Fix sound issue for peer-based notifications
Summary

The context is here. In this differential we disable encryption of sound property for peer notifications.

Test Plan

Ensure that peer notifications make sound. Execute parent differential test plan and ensure that each delivered notification makes sound.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

marcin created this revision.Wed, Aug 21, 4:18 AM
Herald added a subscriber: ashoat. · View Herald TranscriptWed, Aug 21, 4:18 AM
marcin added a parent revision: D13123: Update sendDMOperation to send notifs as well.Wed, Aug 21, 4:19 AM
Harbormaster completed remote builds in B31195: Diff 43549.Wed, Aug 21, 4:35 AM
marcin requested review of this revision.Wed, Aug 21, 4:35 AM
marcin updated this revision to Diff 43553.Wed, Aug 21, 9:52 AM

Update stack order

Harbormaster failed remote builds in B31199: Diff 43553!Wed, Aug 21, 9:56 AM
marcin removed a parent revision: D13123: Update sendDMOperation to send notifs as well.Wed, Aug 21, 10:08 AM
marcin added a child revision: D12882: Implement sending E2EE notification from the client.
ashoat added a comment.Thu, Aug 22, 3:42 PM
  1. Are there any other APNs fields that we need to surface outside of the encrypted blob like this? For instance, how about priority?
  2. Do we have a similar problem with other encrypted notifs, such as the FCM ones we use for Android?
marcin added a comment.Fri, Aug 23, 2:15 AM
In D13131#371051, @ashoat wrote:
  1. Are there any other APNs fields that we need to surface outside of the encrypted blob like this? For instance, how about priority?

Sound was the only one. The priority field is not a part of notification but a part of headers of HTTP request to APNS which are never encrypted.

  1. Do we have a similar problem with other encrypted notifs, such as the FCM ones we use for Android?

No, we don't send notification sound on Android. It is set locally on when notifications channel is first created. See here. createChannel implementation is in native code here. It does not mention sound but according to documentation notifications sent to this channel will make sound since its importance is HIGH. The value of this sound is some default set by the system. User can change it in settings.

tomek accepted this revision.Mon, Aug 26, 5:19 AM
This revision is now accepted and ready to land.Mon, Aug 26, 5:19 AM
marcin updated this revision to Diff 43703.Tue, Aug 27, 10:50 AM

Rebase before landing

Harbormaster completed remote builds in B31302: Diff 43703.Tue, Aug 27, 11:07 AM
Closed by commit rCOMM44b5ff880b3a: Fix sound issue for peer-based notifications (authored by Marcin Wąsowicz <marcinwasowicz@MacBook-Pro-Marcin.local>). · Explain WhyTue, Aug 27, 11:29 AM
This revision was automatically updated to reflect the committed changes.
Marcin Wąsowicz <marcinwasowicz@MacBook-Pro-Marcin.local> added a commit: rCOMM44b5ff880b3a: Fix sound issue for peer-based notifications.

Revision Contents
Changeset List

PathSize
lib/
push/
3 lines
types/
13 lines

Diff 43710

View Options

lib/push/crypto.js

Loading...
View Options

lib/types/notif-types.js

Loading...