Closed Bug 264675 Opened 16 years ago Closed 15 years ago

Make sure we respect locked prefs throughout UI

Categories

(SeaMonkey :: Preferences, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: iann_bugzilla, Assigned: iann_bugzilla)

References

(Blocks 1 open bug)

Details

(Keywords: fixed-seamonkey1.1a, Whiteboard: [verified-seamonkey1.0.3 verified-seamonkey1.1a])

Attachments

(2 files, 17 obsolete files)

25.17 KB, patch
neil
: review+
dveditz
: superreview+
Details | Diff | Splinter Review
2.67 KB, patch
iann_bugzilla
: review+
iann_bugzilla
: approval-seamonkey1.1a+
Details | Diff | Splinter Review
This is the mozilla suite version of bug 241526 and bug 230462
sorry, but this is a duplicate :-)

*** This bug has been marked as a duplicate of 70538 ***
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → DUPLICATE
Attached patch Provisional patch (v0.1) (obsolete) — Splinter Review
This patch:
* Adds a new preftype of "localfile" to nsPrefWindow.js
* Implements missing pref locking for pref-navigator
* Works around bug 264680 in pref-navigator
* Implements missing pref locking for pref-cache
Reopening as bug 70538 is a tracking bug
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Status: REOPENED → ASSIGNED
Blocks: 70538
sorry, I was thinking this was a tracking bug as well

you can't show the path as normal value for a file pref. on mac, it is expected
to show just the leaf name, disabled, and only allow a file picker. furthermore,
paths are a lossy way to describe files on mac.
I'm sure I didn't change any of the logic just where it was doing the work. Are
you saying it is already broken on the Mac?
yes, the cache code is. my point is, you are now adding an api that does the
wrong thing by design, which sounds like a bad thing to me.
This patch:
* Adds a new preftype of "localfile" to nsPrefWindow.js for just get/setPref
* Implements missing pref locking for pref-navigator
* Works around bug 264680 in pref-navigator
* Implements missing pref locking for pref-cache
* Implements missing pref locking for pref-smart_browsing-ac
Attachment #162396 - Attachment is obsolete: true
Blocks: 7840
Attachment #162426 - Flags: review?(cbiesinger)
Comment on attachment 162426 [details] [diff] [review]
Revised and extended patch (v0.2)

a diff -w would be helpful for the navigator.js changes...

+  if (path == "!/!ERROR_UNDEFINED_PREF!/!")

this is the usual way to do this, eh? :(

+  if (!path && !gCacheParentDirLocked)

why the second condition?
I can see that you wouldn't want to save it, but wouldn't you want to _display_
the directory?

+      prefWindow.setPref("localfile", kCacheParentDirPref, path);

is naming an nsILocalFile object "path" a good idea?

+    if (navigator.platform.search(/mac/i) > -1)

if (/Mac/.test(navigator.platform))

(twice)
you want to disable the field on mac too, since editing just the leafname isn't
useful


so, review-. note: I didn't look at the patch in detail, I concentrated on the
mac question
Attachment #162426 - Flags: review?(cbiesinger) → review-
Yes, this is the usual way. getPref returns "!/!ERROR_UNDEFINED_PREF!/!" when
the pref is not found, pref-fonts.js traps it this way.

I'd assumed that if the pref is locked then we'd not want to alter it, though
thinking about it if it is locked then would path ever be null? If it can be
locked to null, it would probably be silly to honour that, so I'll remove that
second condition.

Renamed path to dir throughout.

The text box is read-only already so it cannot be directly editted anyway.

New patch to follow shortly.
This patch (white space changes not shown)
* Adds a new preftype of "localfile" to nsPrefWindow.js for just get/setPref
* Implements missing pref locking for pref-navigator
* Works around bug 264680 in pref-navigator
* Implements missing pref locking for pref-cache
* Only shows leaf name instead of full path in pref-cache for Mac OSes
* Implements missing pref locking for pref-smart_browsing-ac
* Implements missing pref locking for pref-keynav
* Hides tabNavigationForms checkbox for Mac OSes
* Implements missing pref locking for pref-mousewheel
* Implements missing pref locking for pref-smartupdate
Attachment #162426 - Attachment is obsolete: true
Attachment #162693 - Attachment is obsolete: true
Attachment #162694 - Attachment is obsolete: true
Comment on attachment 162860 [details] [diff] [review]
Patch v0.4w includes changes to pref-popups

As per patch v0.3 plus:
* Implements missing pref locking in pref-popups
* Ensures sound.url is a URL
* Persists the last sound directory and passes it in to the filepicker in
pref-popups
* Filters by .wav / .wave in pref-popups
Attachment #162860 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #162860 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #162862 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #162862 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #162860 - Attachment is obsolete: true
pref-popups changes are now in patch on bug 266195
Attachment #162862 - Attachment is obsolete: true
Attachment #163493 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #163492 - Attachment description: Revised patch without pref-popups changes - wpud8 version → Revised (v0.4aw) patch without pref-popups changes - wpud8 version
No longer blocks: 7840
Depends on: 7840
Product: Browser → Seamonkey
Updated patch - same as v0.4a except:
* Took out nsPrefWindow.js changes - they landed in bug 7840
* Took out pref-navigator.js changes - this needs to be fixed once bug 264680
and bug 109932 are fixed
Attachment #163492 - Attachment is obsolete: true
Attachment #163493 - Attachment is obsolete: true
Attachment #163493 - Flags: review?(neil.parkwaycc.co.uk)
Changes since last patch:
* Removed accidental addition of another patch to pref-mousewheel.xul
* Merged doEnableElement and enableField functions in pref-mousewheel.xul
Attachment #169535 - Attachment is obsolete: true
Attachment #169536 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 169536 [details] [diff] [review]
Locked pref and tidy up for some pref windows patch v0.4c

>+  var dir = prefWindow.getPref("localfile", kCacheParentDirPref);
>+  if (dir == "!/!ERROR_UNDEFINED_PREF!/!")
>+    dir = null;
>+
>+  if (!dir)
>   {
This looks silly. Put the dir = null; in the catch below, and condition the try
on the first if.

>+  document.getElementById("chooseDiskCacheFolder").disabled = gCacheParentDirLocked;
This appears to be the only use so why is it a global?

>+  var initialDir = prefWindow.getPref("localfile", kCacheParentDirPref);
>+
>+  // file picker will open at default location if no pref set
[This would seem unlikely given the code above]

>+    gData.linksOnlyLocked = parent.hPrefWindow.getPrefIsLocked('accessibility.typeaheadfind.linksonly');
There's a better way of doing this one; simply use the default prefwindow
processing for this preference item, by setting the appropriate attributes.

>+  function enableField(aCheckbox, aNodeID, setFocus)
>+  {
>+    var el = document.getElementById(aNodeID);
>+    if (aCheckbox.checked)
>+      el.setAttribute("disabled", "true");
>+    else
>+    {
>+      var prefstring = el.getAttribute("prefstring");
>+      if (!parent.hPrefWindow.getPrefIsLocked(prefstring))
>+        el.removeAttribute("disabled");
>+    }
>+
>+    if (!el.disabled && setFocus)
>+      el.focus();
>+  }
Why use a mixture of disabled attribute and property?

>+function Startup()
>+{
>+  var prefElements = document.getElementsByAttribute("prefdisabled", "*");
>+  for (var i = 0; i < prefElements.length; i++){
>+    var prefstring = prefElements[i].getAttribute("prefstring");
>+    var isPrefLocked = parent.hPrefWindow.getPrefIsLocked(prefstring);
>+    if (isPrefLocked)
>+      prefElements[i].setAttribute("prefdisabled", "true");
>+  }
>+}
Doesn't the prefwindow already set the correct disabled attribute?

>+      gShowSearchLocked = (window.arguments[6] == "true") ? true : false;
== "true" already returns true or false, no need to repeat yourself.

>+        document.getElementById(aName).setAttribute("disabled", "true");
Again, property rather than attribute, as per later on.
Attachment #169536 - Flags: review?(neil.parkwaycc.co.uk) → review-
Changes since last patch:
* Made changes as per review.
* Simplified pref-smartupdate.xul's code.
Attachment #169536 - Attachment is obsolete: true
Attachment #169829 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 169829 [details] [diff] [review]
xpfe prefwindow locked pref and tidy up patch v0.4d

>     <radiogroup id="findAsYouTypeAutoWhat" style="margin-left: 2em" 
Fixing this to use class="indent" would be nice :-)

>+      onload="parent.initPanel('chrome://communicator/content/pref/pref-smartupdate.xul'); toggleFrequency();"
Strictly speaking the startup should be in a function named Startup, but I just
can't decide if that's appropriate here.
Attachment #169829 - Flags: review?(neil.parkwaycc.co.uk) → review+
Changes since v0.4d:
* style="margin-left: 2em" -> class="indent"
* onload in pref-smartupdate.xul changed and function toggleFrequency ->
Startup
* onload in pref-keynav.xul changed and function initPrefs -> Startup
Attachment #169829 - Attachment is obsolete: true
Attachment #169901 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 169901 [details] [diff] [review]
xpfe prefwindow respect locked prefs and tidy up patch v0.4e

>-              accesskey="&enableUN.accesskey;" oncommand="toggleFrequency();"
>+              accesskey="&enableUN.accesskey;" oncommand="Startup();"
Now you see how silly that looks...
Attachment #169901 - Flags: review?(neil.parkwaycc.co.uk)
Revisions since v0.4e
* Reverted to v0.4d of pref-smartupdate.xul
Attachment #169901 - Attachment is obsolete: true
Attachment #169947 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #169947 - Flags: review?(neil.parkwaycc.co.uk) → review+
Attachment #169947 - Flags: superreview?(jag)
Attachment #169947 - Flags: superreview?(jag)
Unbitrotted prefs-keynav.js part of patch after landing of bug 187508.
Carrying forward r+
Attachment #169947 - Attachment is obsolete: true
Attachment #170639 - Flags: superreview?(jag)
Attachment #170639 - Flags: review+
Comment on attachment 170639 [details] [diff] [review]
Unbitrotted version of xpfe lockprefs and tidy up patch v0.4g

Since saveKeyNavPrefs() is no longer used on the Mac you could theoretically
not register it on that platform...
Attachment #170639 - Flags: superreview?(jag)
Changes since v0.4g
* By Neil's suggestion prevented registerOKCallbackFunc of saveKeyNavPrefs on
Mac so test can be removed from saveKeyNavPrefs function

Carrying forward r+
Attachment #170639 - Attachment is obsolete: true
Attachment #170765 - Flags: superreview?(jag)
Attachment #170765 - Flags: review+
Attachment #170765 - Flags: superreview?(jag) → superreview?(alecf)
Attachment #170765 - Flags: superreview?(alecf) → superreview?(jag)
Attachment #170765 - Flags: superreview?(jag)
Changes since v0.4h
* Unbitrots patch after landing of bug 274179
* Adds back lockpref checking for pref-keynav which got lost after v0.4f
* Revises how pref-keynav stores its accessibility.tabfocus pref (simpler I
hope)
Attachment #170765 - Attachment is obsolete: true
Attachment #173420 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 173420 [details] [diff] [review]
Unbitrotted again plus additional fixes for xpfe lockprefs and tidy up patch v0.4i

The accessibility.tabfocus pref is deliberately not set on the mac to allow the
look and feel code to read the os setting.

While you're changing things I'd prefer pref-smartupdate.xul to use Startup()
(automatically called by initPanel).

Also, any thoughts on a central locked preference away enabling function?
Attachment #173420 - Flags: review?(neil.parkwaycc.co.uk) → review-
Comment on attachment 173420 [details] [diff] [review]
Unbitrotted again plus additional fixes for xpfe lockprefs and tidy up patch v0.4i

Oh, and please change .setChecked(value) to .checked = value, thanks.
Changes since v0.4j:
* Reverted back to old way of doing pref-keynav but put the lockpref back in
* Added Startup function to pref-smartupdate.xul
Attachment #173420 - Attachment is obsolete: true
Attachment #173424 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #173424 - Flags: review?(neil.parkwaycc.co.uk) → review+
Attachment #173424 - Flags: superreview?(jag)
Attachment #173424 - Flags: superreview?(jag) → superreview?(dveditz)
Comment on attachment 173424 [details] [diff] [review]
Revised xpfe lockprefs plus additional fixes patch v0.4j (Checked in)

sr=dveditz
Attachment #173424 - Flags: superreview?(dveditz) → superreview+
Comment on attachment 173424 [details] [diff] [review]
Revised xpfe lockprefs plus additional fixes patch v0.4j (Checked in)

Checking in pref-cache.js;
/cvsroot/mozilla/xpfe/components/prefwindow/resources/content/pref-cache.js,v 
<--  pref-cache.js
new revision: 1.17; previous revision: 1.16
done
Checking in pref-keynav.xul;
/cvsroot/mozilla/xpfe/components/prefwindow/resources/content/pref-keynav.xul,v
 <--  pref-keynav.xul
new revision: 1.5; previous revision: 1.4
done
Checking in pref-keynav.js;
/cvsroot/mozilla/xpfe/components/prefwindow/resources/content/pref-keynav.js,v 
<--  pref-keynav.js
new revision: 1.4; previous revision: 1.3
done
Checking in pref-mousewheel.xul;
/cvsroot/mozilla/xpfe/components/prefwindow/resources/content/pref-mousewheel.x
ul,v  <--  pref-mousewheel.xul
new revision: 1.43; previous revision: 1.42
done
Checking in pref-smart_browsing.js;
/cvsroot/mozilla/xpfe/components/prefwindow/resources/content/pref-smart_browsi
ng.js,v  <--  pref-smart_browsing.js
new revision: 1.16; previous revision: 1.15
done
Checking in pref-smart_browsing-ac.xul;
/cvsroot/mozilla/xpfe/components/prefwindow/resources/content/pref-smart_browsi
ng-ac.xul,v  <--  pref-smart_browsing-ac.xul
new revision: 1.15; previous revision: 1.14
done
Checking in pref-smartupdate.xul;
/cvsroot/mozilla/xpfe/components/prefwindow/resources/content/pref-smartupdate.
xul,v  <--  pref-smartupdate.xul
new revision: 1.45; previous revision: 1.44
done
Attachment #173424 - Attachment description: Revised xpfe lockprefs plus additional fixes patch v0.4j → Revised xpfe lockprefs plus additional fixes patch v0.4j (Checked in)
Status: ASSIGNED → RESOLVED
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.9a1) Gecko/20060318 SeaMonkey/1.5a] (nightly) (W98SE)

We miss a var declaration:
{{
Warning: assignment to undeclared variable gShowSearchLocked
Source File: chrome://communicator/content/pref/pref-smart_browsing-ac.xul
Line: 61
}}
when going to
> Preferences > Navigator > Smart Browsing > Advanced
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.9a1) Gecko/20060319 SeaMonkey/1.5a] (nightly) (W98SE)

A |var| declaration,
and a bunch of space removals.
Attachment #215612 - Flags: review?(iann_bugzilla)
Same as Bv1, with a little less space nit removals to preserve history.
Attachment #215612 - Attachment is obsolete: true
Attachment #215704 - Flags: review?(iann_bugzilla)
Attachment #215612 - Flags: review?(iann_bugzilla)
Attachment #215704 - Flags: review?(iann_bugzilla) → review+
Comment on attachment 215704 [details] [diff] [review]
(Bv2) <pref-smart_browsing-ac.xul>
[Checked in: Comment 41]

'approval&#8209;seamonkey1.1a=?': (SeaMonkey only)
Trivial U.I. code fix, no risk.
Attachment #215704 - Flags: approval-seamonkey1.1a?
Attachment #215704 - Flags: approval-seamonkey1.1a? → approval-seamonkey1.1a+
Comment on attachment 215704 [details] [diff] [review]
(Bv2) <pref-smart_browsing-ac.xul>
[Checked in: Comment 41]

'approval-seamonkey1.0.1=?': (SeaMonkey only)
Trivial U.I. code fix, no risk.
I wonder if this could be considered a "security" issue: I submit it and let you decide.
Comment on attachment 215704 [details] [diff] [review]
(Bv2) <pref-smart_browsing-ac.xul>
[Checked in: Comment 41]

'approval-seamonkey1.0.1=?': (SeaMonkey only)
Trivial U.I. code fix, no risk.
I wonder if this could be considered a "security" issue: I submit it and let you decide.
Attachment #215704 - Flags: approval-seamonkey1.0.1?
Comment on attachment 215704 [details] [diff] [review]
(Bv2) <pref-smart_browsing-ac.xul>
[Checked in: Comment 41]

Too late for 1.0.1 as that one's practically wrapped up, we don't take anything else than fixes for major regressions there any more.

It's nice cleanup/polish though, please renominate it for 1.0.2 once 1.0.1 is out and we have the new flag(s).
Attachment #215704 - Flags: approval-seamonkey1.0.1? → approval-seamonkey1.0.1-
Comment on attachment 215704 [details] [diff] [review]
(Bv2) <pref-smart_browsing-ac.xul>
[Checked in: Comment 41]


Check in: {
2006-03-26 04:34	bugzilla%standard8.demon.co.uk 	mozilla/xpfe/components/prefwindow/resources/content/pref-smart_browsing-ac.xul 	1.16

and

2006-03-26 04:37	bugzilla%standard8.demon.co.uk 	mozilla/xpfe/components/prefwindow/resources/content/pref-smart_browsing-ac.xul 	1.15.8.1 	MOZILLA_1_8_BRANCH
}
Attachment #215704 - Attachment description: (Bv2) <pref-smart_browsing-ac.xul> → (Bv2) <pref-smart_browsing-ac.xul> [Checked in: Comment 41]
Status: REOPENED → RESOLVED
Closed: 16 years ago15 years ago
Resolution: --- → FIXED
Whiteboard: [SergeG: Bv2 patch awaits approval&#8209;seamonkey1.0.2? flag.]
Attachment #215704 - Flags: approval-seamonkey1.0.2?
Whiteboard: [SergeG: Bv2 patch awaits approval&#8209;seamonkey1.0.2? flag.]
Comment on attachment 215704 [details] [diff] [review]
(Bv2) <pref-smart_browsing-ac.xul>
[Checked in: Comment 41]

The tree is already frozen for Gecko 1.8.0.4 / SeaMonkey 1.0.2, and this is not critical, so it can't make it any more.
Re-nominate for 1.0.3 if it's still wanted (and once the flag for it exists), please.
Attachment #215704 - Flags: approval-seamonkey1.0.2? → approval-seamonkey1.0.2-
Comment on attachment 215704 [details] [diff] [review]
(Bv2) <pref-smart_browsing-ac.xul>
[Checked in: Comment 41]

'approval-seamonkey1.0.3':
Missed 1.0.1 and 1.0.2 ... Retrying.
Attachment #215704 - Flags: approval-seamonkey1.0.3?
Comment on attachment 215704 [details] [diff] [review]
(Bv2) <pref-smart_browsing-ac.xul>
[Checked in: Comment 41]

first-a=me for 1.0.3, need one more.
Comment on attachment 215704 [details] [diff] [review]
(Bv2) <pref-smart_browsing-ac.xul>
[Checked in: Comment 41]

a=me for SM1.0.3 (minor change only one needed)
Attachment #215704 - Flags: approval-seamonkey1.0.3? → approval-seamonkey1.0.3+
[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.1a3) Gecko/20060702 SeaMonkey/1.1a] (nightly) (W98SE)

V.Fixed on MOZILLA_1_8_BRANCH, per comment 34 steps.
Whiteboard: [verified-seamonkey1.1a]
Comment on attachment 215704 [details] [diff] [review]
(Bv2) <pref-smart_browsing-ac.xul>
[Checked in: Comment 41]

patch committed to MOZILLA_1_8_0_BRANCH
Whiteboard: [verified-seamonkey1.1a] → [fixed-seamonkey1.0.3 verified-seamonkey1.1a]
[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.0.5) Gecko/20060703 SeaMonkey/1.0.2] (nightly) (W98SE)

V.Fixed on MOZILLA_1_8_0_BRANCH, per comment 34 steps.
Whiteboard: [fixed-seamonkey1.0.3 verified-seamonkey1.1a] → [verified-seamonkey1.0.3 verified-seamonkey1.1a]
You need to log in before you can comment on or make changes to this bug.