497664 - "Clear Recent History" dialog has "Form & Search History" grayed-out & force-checked, when there's nothing to clear

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect)

Description

[Daniel Holbert [:dholbert]]

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.

Comment 1

[Daniel Holbert [:dholbert]]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090611 Minefield/3.6a1pre

Comment 2

[Felix Fung (:felix)]

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

[Daniel Holbert [:dholbert]]

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 4

[(no longer active)]

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>

Comment 5

[Felix Fung (:felix)]

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

[(no longer active)]

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!

Comment 7

[Mozilla RelEng Bot]

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://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-d02e6e4995c3

Comment 8

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a3850b04dc0

Comment 9

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/1a3850b04dc0