1188686 - Clear all push subscriptions in `ForgetAboutSite.jsm`

Status: RESOLVED FIXED

(Core :: DOM: Push Subscriptions, defect)

Description

[Lina Butler [:lina]]

This is a follow-up to bug 1149274.

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: MozReview Request: Bug 1188686 - Clear push subscriptions when forgetting about site. r?kitcambridge

Bug 1188686 - Clear push subscriptions when forgetting about site. r?kitcambridge

I have added tests but they currently fail since they do not use mocking. I will amend the commit in the evening, but I figured you could review the code if you have time.

Comment 2

[Lina Butler [:lina]]

Comment on attachment 8640623 [details]
MozReview Request: Bug 1188686 - Clear push subscriptions when forgetting about site. r?kitcambridge

https://reviewboard.mozilla.org/r/14351/#review12991

Thanks! Looks good; just noted some nits.

::: toolkit/forgetaboutsite/test/unit/test_removeDataFromDomain.js:505
(Diff revision 1)
> +             getService(Ci.nsIPushNotificationService);

This throws if we exclude the push interfaces from the build.

::: dom/push/PushService.jsm:1034
(Diff revision 1)
> +    /**

I wish `ForgetAboutSite.jsm` could just call `pushService.clearIf(hasRootDomain)`, so we didn't have to duplicate this function...but XPIDL doesn't seem to make that easy. Oh, well.

::: dom/push/PushDB.jsm:171
(Diff revision 1)
> +              if (testFn(cursor.value)) {

I think this should be `testFn(this.toPushRecord(cursor.value))`, or `record.origin` won't exist.

::: dom/push/PushService.jsm:1069
(Diff revision 1)
> +      .catch(_ => Promise.resolve());

I'm wondering if we should log the error here. There's not much that can be done about it, unless the user enables debug mode, so it's probably OK to leave it.

Comment 3

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8640623 [details]
MozReview Request: Bug 1188686 - Clear push subscriptions when forgetting about site. r?kitcambridge

Bug 1188686 - Clear push subscriptions when forgetting about site. r?kitcambridge

Comment 4

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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

Comment 5

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8640623 [details]
MozReview Request: Bug 1188686 - Clear push subscriptions when forgetting about site. r?kitcambridge

Bug 1188686 - Clear push subscriptions when forgetting about site. r?kitcambridge

Comment 6

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://reviewboard.mozilla.org/r/14351/#review13087

::: toolkit/forgetaboutsite/test/unit/head_forgetaboutsite.js:5
(Diff revision 3)
> -const Cc = Components.classes;
> -const Ci = Components.interfaces;
> -const Cu = Components.utils;
> +let Cc = Components.classes;
> +let Ci = Components.interfaces;
> +let Cu = Components.utils;

JS strict mode does not like const redeclaration.

Comment 7

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=15ac3d51a62b

Comment 8

[Lina Butler [:lina]]

> JS strict mode does not like const redeclaration.

I don't think `let` can be redeclared, either; you have to use `var`. :-/

Comment 9

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to Kit Cambridge [:kitcambridge] from comment #8)
> > JS strict mode does not like const redeclaration.
> 
> I don't think `let` can be redeclared, either; you have to use `var`. :-/

Spidermonkey does not seem to complain about let. the tests work after the change.

Comment 10

[Lina Butler [:lina]]

Comment on attachment 8640623 [details]
MozReview Request: Bug 1188686 - Clear push subscriptions when forgetting about site. r?kitcambridge

https://reviewboard.mozilla.org/r/14351/#review13099

Ship It!

Comment 11

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/b080520b4a151b8ee6beda1adc71af2c8626f138
changeset:  b080520b4a151b8ee6beda1adc71af2c8626f138
user:       Nikhil Marathe <[e-mail]>
date:       Wed Jul 29 11:33:48 2015 -0700
description:
Bug 1188686 - Clear push subscriptions when forgetting about site. r=kitcambridge

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b080520b4a15