Add a button "reset all settings" in about:config"?

ASSIGNED
Assigned to

Status

()

ASSIGNED
9 years ago
a year ago

People

(Reporter: jhill, Assigned: mmm)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Reporter)

Description

9 years ago
(Note: this is filed as part of the “Paper Cut” bugs — we assume that there may be multiple existing bugs on this. Please make them block this bug, and we will de-dupe if they are indeed exactly the same. Thanks!)

To reproduce:
1. Go to about:config and make lots of changes
2. Later, try to remember what changes you make
3. want to restore these to the defaults

Recommendation:
Add a button to reset all settings in the about:config page.

Comment 1

9 years ago
Created attachment 450868 [details]
alternative solution (about:support update, mockup)

Yeah, but I think that better idea add this feature to about:support. Feedback?
Tend to agree with comment 1; about:config is meant for people who know what they're doing, about:support is meant for people who got themselves into trouble thinking that they knew what they were doing :)

Would this remove newly created prefs, or merely reset all prefs to defaults where they're declared?

How (if at all) does this intersect with a "reset all preferences" button? Note that resetting to defaults will mean changing a bunch of user-visible prefs as well.
Created attachment 488634 [details] [diff] [review]
Patch per comment 1.

Added "Remove All" button to about:support under Modified Preferences.
Also added an observer so that Modified Preferences updates as preferences are changed.

This adds a "Remove All" entity but it is the same as http://mxr.mozilla.org/mozilla-central/source/toolkit/locales/en-US/chrome/passwordmgr/passwordManager.dtd#53 if that helps for getting past the string freeze?

Marking cbartley for feedback as this touches his code.
Assignee: nobody → mars.martian+bugmail
Status: NEW → ASSIGNED
Attachment #488634 - Flags: feedback?(cab)
Created attachment 488635 [details]
Screenshot of current patch.
(In reply to comment #3)
> Created attachment 488634 [details] [diff] [review]
> ...
> Marking cbartley for feedback as this touches his code.

My number one concern about this idea is that the "Modified Preferences" shown in about:support is an incomplete list.  It should really be something like "Some Modified Preferences" or something like that.  Many, possibly most modified prefs are filtered out for privacy/security reasons (see bug 519077).

The UI implemented here might make too easy for the user to shoot themselves in the foot since they're seeing a subset of changed prefs but the page never actually mentions anywhere that it's a subset.  (There really should be a bug about that.)

I'll provide some feedback on the actual JavaScript in a later comment.  However, I should mention now that I'm not the ideal person to review this patch for various reasons.  It might be hard to track down a good reviewer, so you might want to get started on that right away.
(In reply to comment #3)
> Created attachment 488634 [details] [diff] [review]
> ...
> Marking cbartley for feedback as this touches his code.

The following comments are neither authoritative nor complete.  Hopefully they are helpful.


> let modifiedPrefs;
> function populatePreferencesSection() {
>   modifiedPrefs = getModifiedPrefs();

I'd probably write this something like:

> let gModifiedPrefs = null;
>
> function populatePreferencesSection() {
>   gModifiedPrefs = getModifiedPrefs();

>   // Because removing all modified preferences can send many notifications,
>   // we use a short circuit property to avoid unnecessarily calling
>   // populatePreferencesSection.
>   duringMassRemoval: false,
>   observe: function(aSubject, aTopic, aData) {

I think it's easier to read if you add a blank line:

>   // Because removing all modified preferences can send many notifications,
>   // we use a short circuit property to avoid unnecessarily calling
>   // populatePreferencesSection.
>   duringMassRemoval: false,
>
>   observe: function(aSubject, aTopic, aData) {

I don't know what 

>         !PREFS_WHITELIST.some(function(x) aData.substr(0,x.length) == x))

means in this line:

>     if (isBlacklisted(aData) ||
>         !PREFS_WHITELIST.some(function(x) aData.substr(0,x.length) == x))

I think a helper function analogous to isBlacklisted() would be better than
just inlining the test.  (I'm not sure everybody would agree with me on this
point though.)

> function resetModifiedPreferences() {
>   prefsObserver.duringMassRemoval = true;
> 
>   for each (let pref in modifiedPrefs) {
>     prefsObserver._prefBranch.clearUserPref(pref.name);
>   }
> 
>   prefsObserver.duringMassRemoval = false;
> 
>   populatePreferencesSection();
> }

It may not matter in this particular case, but as a general principle, it's a
good idea to use try .. finally when using a flag this way.  E.g. 

> function resetModifiedPreferences() {
> 
>   prefsObserver.duringMassRemoval = true;
>   try {
>     for each (let pref in modifiedPrefs) {
>       prefsObserver._prefBranch.clearUserPref(pref.name);
>     }
>   }
>   finally {
>     prefsObserver.duringMassRemoval = false;
>   }
> 
>   populatePreferencesSection();
> }

I'd also consider doing

> function resetModifiedPreferences() {
> 
>   .. // remove observer temporarily
>   try {
>     for each (let pref in modifiedPrefs) {
>       prefsObserver._prefBranch.clearUserPref(pref.name);
>     }
>   }
>   finally {
>     .. // restore observer
>   }
> 
>   populatePreferencesSection();
> }

and ditching the prefsObserver.duringMassRemoval flag altogether.  I'm not
certain that's really better though.  Just an idea to consider.

I'd also suggest verifying that the observer management in window.onload() and
window.onunload() work the way you expect.  This isn't so much a "concern" as
maybe simple paranoia on my part.  I think it's probably fine, you just always
want to be careful that you don't sometimes leave observers hanging around that
you didn't intend to.  (This is, in fact, something to watch out for for any
code that uses the Observer pattern, not just Mozilla code.)

Comment 7

8 years ago
(In reply to comment #5)
> My number one concern about this idea is that the "Modified Preferences" shown
> in about:support is an incomplete list.  It should really be something like
> "Some Modified Preferences" or something like that.  Many, possibly most
> modified prefs are filtered out for privacy/security reasons (see bug 519077).
> 
> The UI implemented here might make too easy for the user to shoot themselves in
> the foot since they're seeing a subset of changed prefs but the page never
> actually mentions anywhere that it's a subset.  (There really should be a bug
> about that.)

I concur. I also don't think that there needs to be such a button in about:support.
We already have a message upon opening about:config that warns you not to mess with the prefs unless you know what you are doing or you are willing to take risks.
A much better solution for restoring a profile would be to have a simple process for the user to sync everything to the Sync server, delete the profile, and create a new one with the Sync data.
I think this bug should be WONTFIX'd.

I'm taking this out of cuts-control, since we already provide an interface to reset preferences.

If we really want to do this, I think the button text should be changed to "Reset All", since we are not actually removing the listed preferences.
No longer blocks: 565512

Comment 8

8 years ago
Another reason for removing from papercuts is that about:config is not an interface designed for exposure to a large segment of our users.
Attachment #488634 - Flags: feedback?(cab) → feedback-

Updated

a year ago
Duplicate of this bug: 952349
You need to log in before you can comment on or make changes to this bug.