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.

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.

(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.

Comment on attachment 774546 [details] [diff] [review] Addresses the comments. Looks good to me, but needs additional 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.

Comment on attachment 804916 [details] [diff] [review] Includes test case Review of attachment 804916 [details] [diff] [review]: ----------------------------------------------------------------- Good to go!