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

Status: NEW

(Firefox :: General, defect)

Description

[John Wayne Hill (UX Intern)]

(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

[tymerkaev]

Attachment: alternative solution (about:support update, mockup)

Yeah, but I think that better idea add this feature to about:support. Feedback?

Comment 2

[Mike Beltzner [:beltzner, not reading bugmail]]

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.

Comment 3

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: 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.

Comment 4

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Screenshot of current patch.

Comment 5

[Curtis Bartley [:cbartley]]

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

Comment 6

[Curtis Bartley [:cbartley]]

(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

[Frank Yan (:fryn)]

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

Comment 8

[Frank Yan (:fryn)]

Another reason for removing from papercuts is that about:config is not an interface designed for exposure to a large segment of our users.

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.