Closed Bug 1064261 Opened 6 years ago Closed 5 years ago

When locking the privacy.sanitize.sanitizeOnShutdown pref, it is not displayed as locked

Categories

(Firefox :: Preferences, enhancement)

enhancement
Not set

Tracking

()

VERIFIED FIXED
Firefox 39
Tracking Status
firefox38 --- verified
firefox39 --- verified

People

(Reporter: lagu, Assigned: jaws)

References

Details

Attachments

(2 files, 3 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0
Build ID: 20140904030202

Steps to reproduce:

I set privacy.sanitize.sanitizeOnShutdown to true and looked it via CCK2.
The other settings i looked are greyed out.

This setting is in the UI still changeable.
Same behavior for the drag drop's there.
The settings are blocked, but the dragDrops are still working in the UI.
network.cookie.cookieBehavior - 3
network.cookie.lifetimePolicy - 2
Component: Untriaged → Preferences
Summary: privacy.sanitize.sanitizeOnShutdown is looked but not displayed as looked → When locking the privacy.sanitize.sanitizeOnShutdown pref, it is not displayed as locked
While we would take a fix for this, we won't hold up shipping the in-content preferences until this is fixed.
Blocks: 718011
No longer blocks: ship-incontent-prefs
Mike/Lars, can either of you help us triage this appropriately by providing screenshots/screencasts comparing the non-incontent prefs and the in-content ones? You can easily switch between them using the browser.preferences.inContent pref in about:config .
Blocks: ship-incontent-prefs
No longer blocks: 718011
Flags: needinfo?(mozilla)
Flags: needinfo?(lars.gusowski+bugzilla)
Priority: -- → P3
See Also: → 1137729
Duplicate of this bug: 1137729
Attached file Lock add-on
Here's a simple add-on that adds lock/unlock to about:config to make it easy to test this stuff.

I'll still be producing the information that is needed.
Here's a movie showing the problem:

https://www.dropbox.com/s/5h8vtm8yr1yd9p8/shutdownproblem.mov?dl=0
Flags: needinfo?(mozilla)
Flags: needinfo?(lars.gusowski+bugzilla)
This bug is not a regression in the in-content preferences, as Mike's video shows. The video shows, and I confirmed with Mike's add-on, that the preference is only visibly disabled when the preferences value is changed.
Status: UNCONFIRMED → NEW
Ever confirmed: true
No longer blocks: ship-incontent-prefs
Severity: normal → enhancement
Priority: P3 → --
Attached patch Patch (obsolete) — Splinter Review
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f1ee58b51ce3
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Attachment #8578481 - Flags: review?(gijskruitbosch+bugs)
Attached patch Patch v1.1 (obsolete) — Splinter Review
Attachment #8578481 - Attachment is obsolete: true
Attachment #8578481 - Flags: review?(gijskruitbosch+bugs)
Attachment #8578486 - Flags: review?(gijskruitbosch+bugs)
Attached patch Patch v1.2 (obsolete) — Splinter Review
Attachment #8578486 - Attachment is obsolete: true
Attachment #8578486 - Flags: review?(gijskruitbosch+bugs)
Attachment #8578489 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8578489 [details] [diff] [review]
Patch v1.2

Review of attachment 8578489 [details] [diff] [review]:
-----------------------------------------------------------------

f+ pending test changes to ensure this either only runs with icp or works with non-icp pref panes.

::: browser/components/preferences/in-content/tests/browser_sanitizeOnShutdown_prefLocked.js
@@ +1,2 @@
> +/* Any copyright is dedicated to the Public Domain.
> +* http://creativecommons.org/publicdomain/zero/1.0/ */

You don't need this license header for test code, as we discussed in a previous bug. :-)

@@ +6,5 @@
> +function runPaneTest(fn) {
> +  open_preferences((win) => {
> +    let doc = win.document;
> +    win.gotoPref("panePrivacy");
> +    fn(win, doc);

Nit: s/fn/callbackFn/

@@ +17,5 @@
> +  menulist.focus();
> +  EventUtils.sendKey("UP");
> +}
> +
> +function test() {

If you make this an add_task'd thing, and make runPaneTest a generator that returns the window...

@@ +30,5 @@
> +
> +  let checkbox = doc.getElementById("alwaysClear");
> +  is(checkbox.disabled, false, "Always Clear checkbox should be enabled when preference is not locked.");
> +
> +  gBrowser.removeCurrentTab();

This won't work with windowed prefs. Either enforce that this runs with in-content prefs, or (preferable) make it work with windowed preferences. :-)

@@ +36,5 @@
> +  Services.prefs.lockPref("privacy.sanitize.sanitizeOnShutdown");
> +  runPaneTest(testPrefStateWhenLocked);
> +}
> +
> +function testPrefStateWhenLocked(win, doc) {

Then these two functions could be refactored into a single function that just takes an extra boolean, and then make the task call the function twice with separate params (and calling lockPref after the first call). That seems like it'd be clearer then the continuation-style function calling anyway (to my mind, that is, which I accept is a subjective judgment...).
Attachment #8578489 - Flags: review?(gijskruitbosch+bugs) → feedback+
Attached patch Patch v2Splinter Review
Attachment #8578489 - Attachment is obsolete: true
Attachment #8578769 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8578769 [details] [diff] [review]
Patch v2

Review of attachment 8578769 [details] [diff] [review]:
-----------------------------------------------------------------

So... you've fixed it in non-incontent as well, but the test only tests in-content?

I guess we can just do that for now...
Attachment #8578769 - Flags: review?(gijskruitbosch+bugs) → review+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=abe49df2fde2
OS: Windows 7 → All
Hardware: x86_64 → All
Version: 32 Branch → Trunk
https://hg.mozilla.org/mozilla-central/rev/c5ecca8a071d
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 39
Comment on attachment 8578769 [details] [diff] [review]
Patch v2

Approval Request Comment
[Feature/regressing bug #]: new feature, in-content prefs (icp)
[User impact if declined]: icp behavior differences between firefox 38 and firefox 39
[Describe test coverage new/current, TreeHerder]: landed on mozilla-central for a week
[Risks and why]: none expected.
[String/UUID change made/needed]: none.
Attachment #8578769 - Flags: approval-mozilla-aurora?
Attachment #8578769 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified fixed using latest Aurora 39.0a2 (buildID: 20150427004010) and Firefox 38 Beta 8 (buildID: 20150427090451).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.