STR: 1. Start Firefox with a fresh profile 2. Ctrl-Shift-Del, to bring up Clear Private History dialog 3. Click "Details" ACTUAL RESULTS: "Form & Search History" is grayed out & force-checked. (I can't uncheck it.) Presumably this is because I don't *have* any saved form & search history. However, this behavior is still quite counter-intuitive, because... 1. There's nothing to clear, so if anything, it seems like the box should be force-UNchecked. 2. I don't have any saved data in any of the other categories, either (Cookies, Cache, Active Logins, etc), but *they're* not grayed out -- I can still check & uncheck them all I want.
8 years ago
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090611 Minefield/3.6a1pre
Created attachment 561310 [details] [diff] [review] Clear History: Disabled Preferences Should Not Be Checked Summary of Changes: On initialization, iterate through checkboxes with preferences rather than the preference elements themselves. Then any preference that cannot be cleared is untied from the checkbox so we can show a cleared disabled checkbox regardless of the set preference.
Comment on attachment 561310 [details] [diff] [review] Clear History: Disabled Preferences Should Not Be Checked I don't know the prefs code at all (and I'm out this week on a camping trip), so I'm not an appropriate reviewer. Clearing r?dholbert.
Comment on attachment 561310 [details] [diff] [review] Clear History: Disabled Preferences Should Not Be Checked This looks good to me. I only have two comments: 1. Please keep using this.sanitizePreferences, as the old code does. 2. Can you please add a test as well? You can modify this existing test to start: <http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/browser_sanitizeDialog.js>
Created attachment 561651 [details] [diff] [review] Clear History: Disabled Preferences Should Not Be Checked Additional Changes: - Added Tests I did not understand why we would keep using this.sanitizePreference as per your suggestion ehsan. Since we are detaching the preference from the checkbox, (i.e. removing the preference attribute), if we iterate through this.sanitizePreference then we would need to run a query for each pref to find the related checkbox. It'd also be slightly more confusing because the mapping isn't one pref to one checkbox, there being less checkboxes than prefs.
Comment on attachment 561651 [details] [diff] [review] Clear History: Disabled Preferences Should Not Be Checked Yes, you're right. I was confused. :-) If you have tested this on the try server, I can land it for you. Thanks a lot for your patch!
Try run for d02e6e4995c3 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=d02e6e4995c3 Results (out of 96 total builds): success: 91 warnings: 5 Builds available at http://email@example.com
6 years ago