Support resetting preferences when disabling blocklisted add-ons

RESOLVED FIXED in mozilla27

Status

()

Toolkit
Add-ons Manager
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: kmag, Assigned: sachin)

Tracking

(Blocks: 1 bug)

unspecified
mozilla27
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [squeaky])

Attachments

(1 attachment, 2 obsolete attachments)

Based on bug 784189 comment 2, and Asa's comments in a meeting, we should make it possible to reset preference changes made by add-ons when blocking them based on data in the blocklist entry. We might want to opt for something more general, such as including a fragment of cleanup code for a particular add-on, or otherwise just include a list of preferences (or other types of settings) which should be reset.

There would need to be a separate addons.mozilla.org bug to provide an interface for these changes.
Blocks: 782261
(Assignee)

Updated

5 years ago
Assignee: nobody → sachinhosmani2
(Assignee)

Comment 1

5 years ago
Created attachment 771340 [details] [diff] [review]
Resets prefs when blocklisted add-on is disabled.
Attachment #771340 - Flags: review?(jorge)
Comment on attachment 771340 [details] [diff] [review]
Resets prefs when blocklisted add-on is disabled.

>+    let prefs = [];
>+
>+    for (let x = 0; x < childNodes.length; x++) {
>+      var childElement = childNodes.item(x);
>+      if (!(childElement instanceof Ci.nsIDOMElement))
>         continue;
>-
>-      blockEntry.versions.push(new BlocklistItemData(versionRangeElement));
>+      if (childElement.localName === "prefs") {
>+        let prefElements = childElement.childNodes;
>+        for (let i = 0; i < prefElements.length; i++) {
>+          let prefElement = prefElements.item(i);
>+          if (!(prefElement instanceof Ci.nsIDOMElement) ||
>+              prefElement.localName !== "pref")
>+            continue;
>+          prefs.push(prefElement.textContent);

For consistency, I think it's best to push directly to blockEntry.prefs in the loop, like it's done with blockEntry.versions.

> 
>+        if (state === Ci.nsIBlocklistService.STATE_BLOCKED) {
>+          // It's a hard block. We must reset certain preferences.
>+          let prefs = self._getAddonPrefs(addon.id);
>+          resetPrefs(prefs);
>+         }
>+

We want to be able to do this for softblocks also.
Attachment #771340 - Flags: review?(jorge) → review-
(Assignee)

Comment 3

5 years ago
(In reply to Jorge Villalobos [:jorgev] from comment #2)
> Comment on attachment 771340 [details] [diff] [review]
> Resets prefs when blocklisted add-on is disabled.
> 
> We want to be able to do this for softblocks also.

It works for softblocks also. See here:
https://bugzilla.mozilla.org/attachment.cgi?id=771340&action=diff#a/toolkit/mozapps/extensions/nsBlocklistService.js_sec6
Oh, okay. I guess the comment needs some clarification, because it sounds like we're only doing it for hard blocks.
(Assignee)

Comment 5

5 years ago
Created attachment 774546 [details] [diff] [review]
Addresses the comments.
Attachment #771340 - Attachment is obsolete: true
Attachment #774546 - Flags: review?(jorge)
Comment on attachment 774546 [details] [diff] [review]
Addresses the comments.

Looks good to me, but needs additional review.
Attachment #774546 - Flags: review?(jorge)
Attachment #774546 - Flags: review?(bmcbride)
Attachment #774546 - Flags: review+
Comment on attachment 774546 [details] [diff] [review]
Addresses the comments.

Review of attachment 774546 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good. 

Really needs a test though. See test_blocklistchange.js and test_blocklist_regexp.js for inspiration (add a new test file - test_blocklistchange.js is already too big).

::: toolkit/mozapps/extensions/nsBlocklistService.js
@@ +918,5 @@
>  
> +    // A helper function that reverts the prefs passed to default values.
> +    function resetPrefs(prefs) {
> +      for (let i = 0; i < prefs.length; i++) {
> +        let pref = prefs[i];

New code should use for-of loops. ie:
  for (let pref of prefs) {
    ...
  }

@@ +920,5 @@
> +    function resetPrefs(prefs) {
> +      for (let i = 0; i < prefs.length; i++) {
> +        let pref = prefs[i];
> +        let branch = gPref.getBranch(pref);
> +        branch.clearUserPref("");

Don't need to get the branch for every pref (it has a performance cost). Just gPrefs.clearUserPref() will do.
Attachment #774546 - Flags: review?(bmcbride) → review-
(Assignee)

Comment 8

5 years ago
Created attachment 804916 [details] [diff] [review]
Includes test case
Attachment #774546 - Attachment is obsolete: true
Attachment #804916 - Flags: review?(bmcbride)
Comment on attachment 804916 [details] [diff] [review]
Includes test case

Review of attachment 804916 [details] [diff] [review]:
-----------------------------------------------------------------

Good to go!
Attachment #804916 - Flags: review?(bmcbride) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a7d9f7af109f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Blocks: 945875
Blocks: 947446
Blocks: 951266
You need to log in before you can comment on or make changes to this bug.