Page MenuHomePhabricator
Differential D9818

Refactor Windows push code to match conventions for other platforms
ClosedPublic
Actions

Authored by marcin on Nov 10 2023, 5:43 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Mar 31, 10:31 PM
Unknown Object (File)
Mon, Mar 31, 5:11 PM
Unknown Object (File)
Tue, Mar 18, 11:15 AM
Unknown Object (File)
Tue, Mar 18, 11:15 AM
Unknown Object (File)
Tue, Mar 18, 11:15 AM
Unknown Object (File)
Tue, Mar 18, 11:15 AM
Unknown Object (File)
Tue, Mar 18, 10:59 AM
Unknown Object (File)
Tue, Mar 18, 10:37 AM
View All Files
Subscribers
ashoat
tomek
will

Details

Reviewers
michal
Commits
rCOMM694db6d6559a: Refactor Windows push code to match conventions for other platforms
Summary

This differential does not introduce new functionality but unifies typing and signature for push-related functions for all platforms.

Test Plan

Ensure that Windows notifications work correctly.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

marcin created this revision.Nov 10 2023, 5:43 AM
Herald added subscribers: will, tomek, ashoat. · View Herald TranscriptNov 10 2023, 5:43 AM
Harbormaster completed remote builds in B23990: Diff 33064.Nov 10 2023, 6:03 AM
marcin requested review of this revision.Nov 10 2023, 6:03 AM
marcin updated this revision to Diff 33067.Nov 10 2023, 6:08 AM

Rebase to update commit message

marcin added a parent revision: D9817: Add encryption order to encrypted notifications.Nov 10 2023, 6:09 AM
marcin added a child revision: D9819: Send visual notifications in the same order as they were encrypted.
Harbormaster completed remote builds in B23993: Diff 33067.Nov 10 2023, 6:50 AM
michal accepted this revision.Nov 14 2023, 7:21 AM
This revision is now accepted and ready to land.Nov 14 2023, 7:21 AM
marcin updated this revision to Diff 33545.Nov 23 2023, 2:07 AM

Rebase before landing

Harbormaster completed remote builds in B24365: Diff 33545.Nov 23 2023, 4:15 AM
Closed by commit rCOMM694db6d6559a: Refactor Windows push code to match conventions for other platforms (authored by marcin). · Explain WhyNov 23 2023, 5:50 AM
This revision was automatically updated to reflect the committed changes.
marcin added a commit: rCOMM694db6d6559a: Refactor Windows push code to match conventions for other platforms.

Revision Contents
Changeset List

PathSize
keyserver/
src/
push/
20 lines
11 lines
27 lines

Diff 33064

View Options

keyserver/src/push/send.js

Show First 20 Lines • Show All 396 Lines • ▼ Show 20 Lines async ensureAndroidPushNotifsEnabled() {
let [hasPermission] = permissionPromisesResult; let [hasPermission] = permissionPromisesResult;
const [, canRequestPermission] = permissionPromisesResult; const [, canRequestPermission] = permissionPromisesResult;
if (!hasPermission && canRequestPermission) { if (!hasPermission && canRequestPermission) {
const permissionResponse = await (async () => { const permissionResponse = await (async () => {
// We issue a call to sleep to match iOS behavior where prompt // We issue a call to sleep to match iOS behavior where prompt
// doesn't appear immediately but after logged-out modal disappears // doesn't appear immediately but after logged-out modal disappears
await sleep(10); await sleep(10);
await this.requestAndroidNotificationsPermission(); return await this.requestAndroidNotificationsPermission();
marcinUnsubmitted
Not Done
Inline Actions

We definitely should return something from this lambda since otherwise it is useless to assign it to variable.

When we find we don't have Android notif permissions, we ignore the result of requestAndroidNotificationsPermission. It seems like that would cause us to fail to register notif permissions on Android.

If we find that we don't have permissions then just calling await this.requestAndroidNotificationsPermission(); would result in a prompt asking for notifications permissions. However if the user grants those permissions then hasPermissions is still falsy (since promise returned nothing), so deviceToken will be set to null. Nevertheless permissions are actually granted byt the OS, so the state on the device and keyserver would heal itself on next render.

This differential fixes this case so that if user grants permissions correct state is achieved immediately without need for additional re-render to heal the state.

marcin: We definitely should return something from this lambda since otherwise it is useless to assign…
})(); })();
hasPermission = permissionResponse; hasPermission = permissionResponse;
} }
if (!hasPermission) { if (!hasPermission) {
this.failedToRegisterPushPermissionsAndroid(!canRequestPermission); this.failedToRegisterPushPermissionsAndroid(!canRequestPermission);
return; return;
} }
▲ Show 20 Lines • Show All 328 Lines • Show Last 20 Lines
View Options

keyserver/src/push/types.js

Show First 20 Lines • Show All 396 Lines • ▼ Show 20 Lines async ensureAndroidPushNotifsEnabled() {
let [hasPermission] = permissionPromisesResult; let [hasPermission] = permissionPromisesResult;
const [, canRequestPermission] = permissionPromisesResult; const [, canRequestPermission] = permissionPromisesResult;
if (!hasPermission && canRequestPermission) { if (!hasPermission && canRequestPermission) {
const permissionResponse = await (async () => { const permissionResponse = await (async () => {
// We issue a call to sleep to match iOS behavior where prompt // We issue a call to sleep to match iOS behavior where prompt
// doesn't appear immediately but after logged-out modal disappears // doesn't appear immediately but after logged-out modal disappears
await sleep(10); await sleep(10);
await this.requestAndroidNotificationsPermission(); return await this.requestAndroidNotificationsPermission();
marcinUnsubmitted
Not Done
Inline Actions

We definitely should return something from this lambda since otherwise it is useless to assign it to variable.

When we find we don't have Android notif permissions, we ignore the result of requestAndroidNotificationsPermission. It seems like that would cause us to fail to register notif permissions on Android.

If we find that we don't have permissions then just calling await this.requestAndroidNotificationsPermission(); would result in a prompt asking for notifications permissions. However if the user grants those permissions then hasPermissions is still falsy (since promise returned nothing), so deviceToken will be set to null. Nevertheless permissions are actually granted byt the OS, so the state on the device and keyserver would heal itself on next render.

This differential fixes this case so that if user grants permissions correct state is achieved immediately without need for additional re-render to heal the state.

marcin: We definitely should return something from this lambda since otherwise it is useless to assign…
})(); })();
hasPermission = permissionResponse; hasPermission = permissionResponse;
} }
if (!hasPermission) { if (!hasPermission) {
this.failedToRegisterPushPermissionsAndroid(!canRequestPermission); this.failedToRegisterPushPermissionsAndroid(!canRequestPermission);
return; return;
} }
▲ Show 20 Lines • Show All 328 Lines • Show Last 20 Lines
View Options

keyserver/src/push/utils.js

Show First 20 Lines • Show All 396 Lines • ▼ Show 20 Lines async ensureAndroidPushNotifsEnabled() {
let [hasPermission] = permissionPromisesResult; let [hasPermission] = permissionPromisesResult;
const [, canRequestPermission] = permissionPromisesResult; const [, canRequestPermission] = permissionPromisesResult;
if (!hasPermission && canRequestPermission) { if (!hasPermission && canRequestPermission) {
const permissionResponse = await (async () => { const permissionResponse = await (async () => {
// We issue a call to sleep to match iOS behavior where prompt // We issue a call to sleep to match iOS behavior where prompt
// doesn't appear immediately but after logged-out modal disappears // doesn't appear immediately but after logged-out modal disappears
await sleep(10); await sleep(10);
await this.requestAndroidNotificationsPermission(); return await this.requestAndroidNotificationsPermission();
marcinUnsubmitted
Not Done
Inline Actions

We definitely should return something from this lambda since otherwise it is useless to assign it to variable.

When we find we don't have Android notif permissions, we ignore the result of requestAndroidNotificationsPermission. It seems like that would cause us to fail to register notif permissions on Android.

If we find that we don't have permissions then just calling await this.requestAndroidNotificationsPermission(); would result in a prompt asking for notifications permissions. However if the user grants those permissions then hasPermissions is still falsy (since promise returned nothing), so deviceToken will be set to null. Nevertheless permissions are actually granted byt the OS, so the state on the device and keyserver would heal itself on next render.

This differential fixes this case so that if user grants permissions correct state is achieved immediately without need for additional re-render to heal the state.

marcin: We definitely should return something from this lambda since otherwise it is useless to assign…
})(); })();
hasPermission = permissionResponse; hasPermission = permissionResponse;
} }
if (!hasPermission) { if (!hasPermission) {
this.failedToRegisterPushPermissionsAndroid(!canRequestPermission); this.failedToRegisterPushPermissionsAndroid(!canRequestPermission);
return; return;
} }
▲ Show 20 Lines • Show All 328 Lines • Show Last 20 Lines