Last Comment Bug 497664 - "Clear Recent History" dialog has "Form & Search History" grayed-out & force-checked, when there's nothing to clear
: "Clear Recent History" dialog has "Form & Search History" grayed-out & force-...
Status: RESOLVED FIXED
[inbound]
:
Product: Firefox
Classification: Client Software
Component: Preferences (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 10
Assigned To: Felix Fung (:felix)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-06-11 12:31 PDT by Daniel Holbert [:dholbert] (largely AFK until June 28)
Modified: 2011-09-28 02:10 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Clear History: Disabled Preferences Should Not Be Checked (2.68 KB, patch)
2011-09-20 15:12 PDT, Felix Fung (:felix)
no flags Details | Diff | Review
Clear History: Disabled Preferences Should Not Be Checked (6.56 KB, patch)
2011-09-21 21:15 PDT, Felix Fung (:felix)
ehsan: review+
Details | Diff | Review

Description Daniel Holbert [:dholbert] (largely AFK until June 28) 2009-06-11 12:31:28 PDT
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] (largely AFK until June 28) 2009-06-11 12:37:02 PDT
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090611 Minefield/3.6a1pre
Comment 2 Felix Fung (:felix) 2011-09-20 15:12:24 PDT
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 3 Daniel Holbert [:dholbert] (largely AFK until June 28) 2011-09-20 17:13:12 PDT
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 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-21 08:05:05 PDT
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) 2011-09-21 21:15:27 PDT
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 6 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-22 09:02:33 PDT
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 2011-09-27 01:31:09 PDT
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/ffung@mozilla.com-d02e6e4995c3
Comment 8 Daniel Holbert [:dholbert] (largely AFK until June 28) 2011-09-27 14:03:52 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a3850b04dc0
Comment 9 Marco Bonardo [::mak] 2011-09-28 02:10:23 PDT
https://hg.mozilla.org/mozilla-central/rev/1a3850b04dc0

Note You need to log in before you can comment on or make changes to this bug.