1219063 - Remove obsolete "push" permission

Status: VERIFIED FIXED

(Core :: DOM: Push Subscriptions, defect)

Description

[Lina Butler [:lina]]

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.

Comment 1

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1219063, Part 2 - Remove obsolete "push" permission. r?mt

Bug 1219063, Part 1 - Use transactions for updating Push subscription permissions. r?mt

Comment 2

[Lina Butler [:lina]]

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

Comment 3

[Martin Thomson [:mt:]]

Comment on attachment 8680338 [details]
MozReview Request: Bug 1219063, Part 2 - Remove obsolete "push" permission. r?mt

https://reviewboard.mozilla.org/r/23615/#review21119

Comment 4

[Lina Butler [:lina]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=300428e3ce88

Comment 5

[Lina Butler [:lina]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a7a278648367a73ba842f297f2c4e042b28edfe
Bug 1219063, Part 1 - Use transactions for updating Push subscription permissions. r=mt

https://hg.mozilla.org/integration/mozilla-inbound/rev/9cc31dc74e391cc233fe310218f090c790a7ecc1
Bug 1219063, Part 2 - Remove obsolete "push" permission. r=mt

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1a7a27864836
https://hg.mozilla.org/mozilla-central/rev/9cc31dc74e39

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/1a7a27864836
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/9cc31dc74e39

Comment 8

[Carsten Book [:Tomcat]]

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion

Comment 9

[Lina Butler [:lina]]

Attachment: 2.patch

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.

Comment 10

[Lina Butler [:lina]]

Attachment: 3.patch

Same as comment 9.

Comment 11

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8700919 [details] [diff] [review]
3.patch

Same comment as above.

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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?

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8700919 [details] [diff] [review]
3.patch

Beta44+

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/d14a19cedf0c
https://hg.mozilla.org/releases/mozilla-beta/rev/24692f1a1939

Comment 17

[Lina Butler [:lina]]

Thanks for uplifting this!

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/d14a19cedf0c
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/24692f1a1939

Comment 19

[Michelle Funches - QA]

Verified Beta 44.0b4