471898 - In Tools > Options > Privacy: if sanitize on shutdown is off settings... button should be disabled

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect)

Description

[tabmix.onemen]

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

Comment 1

[Roberto Estrada]

Attachment: Patch to address the bug #471898

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

[Nochum Sossonko [:Natch]]

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 )

Comment 3

[Roberto Estrada]

Attachment: Patch to address the bug #471898

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.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

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!

Comment 5

[Roberto Estrada]

Attachment: Patch to address the bug #471898 (v2)

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.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

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!

Comment 7

[Roberto Estrada]

Attachment: Patch to address the bug #471898 (v3)

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!

Comment 8

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 9

[Jim Jeffery not reading bug-mail 1/2/11]

Did this get missed ?

Comment 10

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/6e53a81c8e00

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 12

[Luke Iliffe (Harlequin99)]

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

Comment 13

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

Comment on attachment 357732 [details] [diff] [review]
Patch to address the bug #471898 (v3)

a191=beltzner

Comment 14

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d78564bc0129

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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