Closed
Bug 471898
Opened 16 years ago
Closed 15 years ago
In Tools > Options > Privacy: if sanitize on shutdown is off settings... button should be disabled
Categories
(Firefox :: Settings UI, defect)
Firefox
Settings UI
Tracking
()
VERIFIED
FIXED
Firefox 3.6a1
People
(Reporter: tabmix.onemen, Assigned: roberto.estrada)
References
Details
(Keywords: verified1.9.1)
Attachments
(1 file, 3 obsolete files)
2.19 KB,
patch
|
Gavin
:
review+
beltzner
:
approval1.9.1+
|
Details | Diff | Splinter Review |
In Tools > Options > Privacy: if "Always clear my private data when i close Minefield" is not checked (privacy.sanitize.sanitizeOnShutdown is false) then settings... button, to clear private data should be disabled
Assignee | ||
Comment 1•16 years ago
|
||
This patch adresses the bug #471898, and makes the "Setings..." button on the Privacy Preferences tab to work properly as specified on the bug report. Firefox builds and passes the tests properly with the patch applied, the patch has also been tested manually
Comment 2•16 years ago
|
||
Comment on attachment 355251 [details] [diff] [review] Patch to address the bug #471898 Align your name with the rest of the names under the Contributors. Also you use tabs to indent your code, you should always use spaces. Finally, you have to request review from a browser peer for this patch. (See http://www.mozilla.org/projects/firefox/review.html )
Assignee | ||
Comment 3•16 years ago
|
||
I have fixed the patch to meet code conventions and requested review, thanks for the advice Natch, I'm a bit newbie, sorry for any inconveniences.
Attachment #355251 -
Attachment is obsolete: true
Attachment #355252 -
Flags: review?(gavin.sharp)
Updated•16 years ago
|
Assignee: nobody → roberto.estrada
Assignee | ||
Updated•16 years ago
|
Attachment #355252 -
Flags: ui-review?(beltzner)
Assignee | ||
Updated•16 years ago
|
Attachment #355252 -
Flags: ui-review?(beltzner)
Comment 4•16 years ago
|
||
Comment on attachment 355252 [details] [diff] [review] Patch to address the bug #471898 Thanks for the patch! Looks good, I just have some minor style nits... >diff -r 97fb6dbf8295 browser/components/preferences/privacy.js > init: function () > { > this._updateHistoryDaysUI(); >+ //By default privacy.sanitize.sanitizeOnShutdown is false >+ //this call checks the status of the checkbox which enables/disables >+ //the option and consequently activates or deactivates "the Setings..." button >+ this.onSanitizeOnShutdown(); I think "_updateSanitizeSettingsButton" would be a more descriptive method name. The comment is redundant, since the method is already described next to where it's declared, so I would just remove it. >+ /** >+ * Enables or disables the "Setings..." button depending >+ * on the privacy.sanitize.sanitizeOnShutdown value nit: "Setings..." -> "Settings...". Also, it's not quite setting it depending on the pref value (on purpose, since that would be wrong in the instantApply=false case), so "depending on the checkbox state" is probably better. r=me with those addressed, sorry for the review delay!
Attachment #355252 -
Flags: review?(gavin.sharp) → review+
Assignee | ||
Comment 5•16 years ago
|
||
Alright, here is the corrected patch which addresses this bug, I've fixed as you told me Gavin, thanks for the advices. I've tested the changes and works well. Also I requested again a review because that is why I understood from your comment Gavin, sorry if I misunderstood you.
Attachment #355252 -
Attachment is obsolete: true
Attachment #356968 -
Flags: review?
Assignee | ||
Updated•16 years ago
|
Attachment #356968 -
Flags: review? → review?(gavin.sharp)
Comment 6•16 years ago
|
||
Comment on attachment 356968 [details] [diff] [review] Patch to address the bug #471898 (v2) Typically you don't need to re-request review if the review was granted, assuming you only make the changes requested in the review. ...in this case, though, I think I just thought of a better way to do this. Instead of calling updateSanitizeSettingsButton in an oncommand handler on the checkbox, you can put it in an onchange handler on the privacy.sanitize.sanitizeOnShutdown <preference> element, e.g.: <preference id="privacy.sanitize.sanitizeOnShutdown" name="privacy.sanitize.sanitizeOnShutdown" onchange="gPrivacyPane._updateSanitizeSettingsButton();" type="bool"/> function _updateSanitizeSettingsButton() { var settingsButton = document.getElementById("clearDataSettings"); var sanitizeOnShutdown = document.getElementById("privacy.sanitize.sanitizeOnShutdown"); settingsButton.disabled = !sanitizeOnShutdown.value; } Not that different than the current patch, but more resilient to external changes to the pref value. Sorry I didn't mention that earlier!
Assignee | ||
Comment 7•16 years ago
|
||
I've improved the patch as you told me Gavin, and got it based on the preference value instead of the checkbox value, this is safer, because the preference could be modified externally and the ui would begin to behave incorrectly. Thanks! Hope this will be the final version!
Attachment #356968 -
Attachment is obsolete: true
Attachment #356968 -
Flags: review?(gavin.sharp)
Updated•16 years ago
|
Keywords: checkin-needed
Comment 8•16 years ago
|
||
Comment on attachment 357732 [details] [diff] [review] Patch to address the bug #471898 (v3) >diff -r 97fb6dbf8295 browser/components/preferences/privacy.js > init: function () > { >- this._updateHistoryDaysUI(); >+ this._updateHistoryDaysUI(); This whitespace change shouldn't be pushed, whoever commits this can fix before pushing though.
Attachment #357732 -
Flags: review+
Comment 9•15 years ago
|
||
Did this get missed ?
Updated•15 years ago
|
Status: NEW → ASSIGNED
Comment 10•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/6e53a81c8e00
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.2a1
Comment 11•15 years ago
|
||
Verified fixed on trunk with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090208 Minefield/3.2a1pre ID:20090208020444 and the identical build on Windows.
Status: RESOLVED → VERIFIED
Comment 12•15 years ago
|
||
Can this be submitted to 1.9.1 branch as well, or at least have the wanted firefox 3.1 flag set, (the option is disabled for me)? This bug makes a direct UI link between the settings button in the options > Privacy pane to clearing history on shutdown. It may reduce any potential confusion for users upgrading from 3.0.x and discovering there is no longer an option to prompt before clearing history. It seems a nice UI change that tidies up the consequences of bug 469158 and would be nice to have for 3.5
Updated•15 years ago
|
Attachment #357732 -
Flags: approval1.9.1?
Updated•15 years ago
|
Flags: wanted-firefox3.1?
Comment 13•15 years ago
|
||
Comment on attachment 357732 [details] [diff] [review] Patch to address the bug #471898 (v3) a191=beltzner
Attachment #357732 -
Flags: approval1.9.1? → approval1.9.1+
Updated•15 years ago
|
Keywords: checkin-needed
Whiteboard: [checkin needed on 1.9.1]
Comment 15•15 years ago
|
||
Verified with builds on OS X and Windows: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090322 Shiretoko/3.5b4pre ID:20090322030800
Keywords: fixed1.9.1 → verified1.9.1
You need to log in
before you can comment on or make changes to this bug.
Description
•