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

VERIFIED FIXED in Firefox 3.6a1

Status

()

VERIFIED FIXED
10 years ago
8 years ago

People

(Reporter: tabmix.onemen, Assigned: roberto.estrada)

Tracking

({verified1.9.1})

Trunk
Firefox 3.6a1
verified1.9.1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

10 years ago
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

10 years ago
Created attachment 355251 [details] [diff] [review]
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 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

10 years ago
Created attachment 355252 [details] [diff] [review]
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.
Attachment #355251 - Attachment is obsolete: true
Attachment #355252 - Flags: review?(gavin.sharp)

Updated

10 years ago
Assignee: nobody → roberto.estrada
(Assignee)

Updated

10 years ago
Attachment #355252 - Flags: ui-review?(beltzner)
(Assignee)

Updated

10 years ago
Attachment #355252 - Flags: ui-review?(beltzner)
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

10 years ago
Created attachment 356968 [details] [diff] [review]
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.
Attachment #355252 - Attachment is obsolete: true
Attachment #356968 - Flags: review?
(Assignee)

Updated

10 years ago
Attachment #356968 - Flags: review? → review?(gavin.sharp)
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

10 years ago
Created attachment 357732 [details] [diff] [review]
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!
Attachment #356968 - Attachment is obsolete: true
Attachment #356968 - Flags: review?(gavin.sharp)
Keywords: checkin-needed
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+
Did this get missed ?
Status: NEW → ASSIGNED
http://hg.mozilla.org/mozilla-central/rev/6e53a81c8e00
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.2a1
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
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
Flags: wanted-firefox3.1?
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+
Keywords: checkin-needed
Whiteboard: [checkin needed on 1.9.1]
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d78564bc0129
Flags: wanted-firefox3.1?
Keywords: checkin-needed → fixed1.9.1
Whiteboard: [checkin needed on 1.9.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
Keywords: fixed1.9.1 → verified1.9.1

Updated

10 years ago
Duplicate of this bug: 484986
You need to log in before you can comment on or make changes to this bug.