Closed Bug 1219063 Opened 4 years ago Closed 4 years ago

Remove obsolete "push" permission

Categories

(Core :: DOM: Push Notifications, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla45
Tracking Status
firefox44 --- fixed
firefox45 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: lina, Assigned: lina)

Details

Attachments

(2 files, 1 obsolete file)

https://dxr.mozilla.org/mozilla-central/source/dom/push/PushService.jsm#1299 refers to the old "push" permission, as do the tests. Let's also add a mochitest that exercises this observer. The existing xpcshell test should take care of the other cases.
Bug 1219063, Part 1 - Use transactions for updating Push subscription permissions. r?mt
Attachment #8680338 - Flags: review?(martin.thomson)
Comment on attachment 8680338 [details]
MozReview Request: Bug 1219063, Part 2 - Remove obsolete "push" permission. r?mt

Bug 1219063, Part 2 - Remove obsolete "push" permission. r?mt
Attachment #8680338 - Attachment description: MozReview Request: Bug 1219063, Part 1 - Use transactions for updating Push subscription permissions. r?mt → MozReview Request: Bug 1219063, Part 2 - Remove obsolete "push" permission. r?mt
Attachment #8680338 - Flags: review?(martin.thomson) → review+
Comment on attachment 8680338 [details]
MozReview Request: Bug 1219063, Part 2 - Remove obsolete "push" permission. r?mt

https://reviewboard.mozilla.org/r/23615/#review21119
https://hg.mozilla.org/mozilla-central/rev/1a7a27864836
https://hg.mozilla.org/mozilla-central/rev/9cc31dc74e39
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
Attached patch 2.patchSplinter Review
Approval Request Comment
[Feature/regressing bug #]: Bug 1233387.
[User impact if declined]: A site may continue sending push messages even if a user revokes permission. While the messages won't be posted to the service worker, the site has no indication that the subscription is no longer valid. Additionally, the `pushsubscriptionchange` event will not fire if the user restores the permission.
[Describe test coverage new/current, TreeHerder]: Test added in bug 1223202, also requested for uplift.
[Risks and why]: Medium risk. This patch has been on Nightly for more than six weeks, and verified working in bug 1233387. However, it's a moderate, two-part refactor of the `pushsubscriptionchange` logic.
[String/UUID change made/needed]: None.
Attachment #8680338 - Attachment is obsolete: true
Attachment #8700918 - Flags: review+
Attachment #8700918 - Flags: approval-mozilla-beta?
Attached patch 3.patchSplinter Review
Same as comment 9.
Attachment #8700919 - Flags: review+
Attachment #8700919 - Flags: approval-mozilla-beta?
Comment on attachment 8700918 [details] [diff] [review]
2.patch

Given the medium risk, I would prefer significant changes to feature design (unless critical) ride the trains than be uplifted to Beta directly. Please let me know if you have any concerns.
Attachment #8700918 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Comment on attachment 8700919 [details] [diff] [review]
3.patch

Same comment as above.
Attachment #8700919 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Kit, given the scenario that we are trying to fix seems important, is there another (hacky simpler) fix that can be made in Beta44? I am still trying to understand how important it is for us to fix this. Would we block the release for it?
Flags: needinfo?(kcambridge)
Comment on attachment 8700918 [details] [diff] [review]
2.patch

On second thoughts, it does seem like a mainline scenario that we can uplift to Beta44 and get QA to sign off on.
Attachment #8700918 - Flags: approval-mozilla-beta- → approval-mozilla-beta+
Comment on attachment 8700919 [details] [diff] [review]
3.patch

Beta44+
Attachment #8700919 - Flags: approval-mozilla-beta- → approval-mozilla-beta+
Thanks for uplifting this!
Flags: needinfo?(kcambridge)
Verified Beta 44.0b4
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.