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 ***

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

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

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

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

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

pref-popups changes are now in patch on bug 266195

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

Changes since last patch: * Removed accidental addition of another patch to pref-mousewheel.xul * Merged doEnableElement and enableField functions in pref-mousewheel.xul

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.

Changes since last patch: * Made changes as per review. * Simplified pref-smartupdate.xul's code.

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.

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

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...

Revisions since v0.4e * Reverted to v0.4d of pref-smartupdate.xul

Unbitrotted prefs-keynav.js part of patch after landing of bug 187508. Carrying forward r+

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...

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+

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)

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?

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

Comment on attachment 173424 [details] [diff] [review] Revised xpfe lockprefs plus additional fixes patch v0.4j (Checked in) sr=dveditz

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

[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

[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.

Same as Bv1, with a little less space nit removals to preserve history.

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.

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.

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).

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 }

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.

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.

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)

[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.

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

[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.