Observe attached screenshot of the Network setting panel when "user-set cache size" is in effect. I'm not schooled in UI, but I think the general idiom for "input field with checkbox controlling if it's greyed out" is that you check the box in order to enable the field, rather than uncheck it, as we currently have here. I.e., this UI would be better if the checkbox were selected instead of empty, and the text said "Manually control the size of my cache". (My eye at least is trained to associate an unchecked box to the upper left of an indented field as "that setting is off")

Yes, agreed — sorry I didn't catch this the first time around. If we're string-frozen, a workable solution would be to make sure the input box is grayed out too — or just hide the entire line, which is even clearer. If we can still change strings, something like: "Manually override cache settings (not recommended)" would work. I'm sure there's a better phrasing for this, but I'm tired right now. :) The main point is to reinforce that in most cases, you want Firefox to be managing this for you.

Requesting blocking-beta7, as this needs a string change. I'll have a patch by end of day, assuming limi and I can work out an agreed-upon wording. I think we want to change both the wording for both the checkbox and the indented value. "Override automatic cache management" -- "Limit cache to XX MB of space" My current favorite: it conveys the idea that Firefox is managing the size of the cache for you, and that you're "limiting" things if you tweak it. Seems likely to scare away all but those who actually need it (i.e. those who are have a disk space crunch). "Manually limit the size of my cache" -- "Limit cache to XX MB of space" Suitably scary, but doesn't make it clear that there still is a limit on cache size, even if you don't set this.

(In reply to comment #2) > I think we want to change both the wording for both the checkbox and the > indented value. > > "Override automatic cache management" > -- "Limit cache to XX MB of space" > > My current favorite: it conveys the idea that Firefox is managing the size of > the cache for you, and that you're "limiting" things if you tweak it. Seems > likely to scare away all but those who actually need it (i.e. those who are > have a disk space crunch). Agreed, this is the best option so far. We could even go further and replace "automatic" with "optimized" if we really want to push the idea that the setting is doing the right thing for you, but I'm fine with either.

OK, here's the patch. Wording already approved by limi. Gavin: At first I thought I needed to set "disabled" for useCacheBefore/cacheSize/useCacheAfter (since they need to be greyed out initially), but then discovered that the function that greys them out gets called when the prefs panel loads.

Comment on attachment 477641 [details] [diff] [review] Changed names of altered string variables >diff --git a/browser/components/preferences/advanced.js b/browser/components/preferences/advanced.js >- updateCacheSizeUI: function (smartSizeEnabled) >+ updateCacheSizeUI: function (smartSizeDisabled) > { >- document.getElementById("useCacheBefore").disabled = smartSizeEnabled; >- document.getElementById("cacheSize").disabled = smartSizeEnabled; >- document.getElementById("useCacheAfter").disabled = smartSizeEnabled; >+ document.getElementById("LimitCacheSizeBefore").disabled = smartSizeDisabled; >+ document.getElementById("cacheSize").disabled = smartSizeDisabled; >+ document.getElementById("LimitCacheSizeAfter").disabled = smartSizeDisabled; I don't think you want this change - the disabled state of these elements depends on whether "smart sizing" is enabled, so .disabled = smartSizeEnabled is the correct semantic. > readSmartSizeEnabled: function () > { > var enabled = document.getElementById("browser.cache.disk.smart_size.enabled").value; >- this.updateCacheSizeUI(enabled); >+ this.updateCacheSizeUI(!enabled); This could use a comment - "The smart_size.enabled preference element is inverted='true', so it's value is the opposite of the actual pref value.". Also just name the variable "disabled". >diff --git a/browser/components/preferences/advanced.xul b/browser/components/preferences/advanced.xul >+ <label id="LimitCacheSizeBefore" control="cacheSize" >+ <label id="LimitCacheSizeAfter" flex="1">&LimitCacheSizeAfter.label;</label> No need to change the IDs for these. >diff --git a/browser/locales/en-US/chrome/browser/preferences/advanced.dtd b/browser/locales/en-US/chrome/browser/preferences/advanced.dtd >+ The entities LimitCacheSizeBefore.label and LimitCacheSizeAfter.label appear on a single nit: use "limitCacheSizeBefore" capitalization to be consistent with all the others (and with string names in general)

Updated with gavin's changes. > I don't think you want this change The code worked (I do check my patches! :), but yes, the variable name implied the inverse. Much clearer now.

Comment on attachment 477681 [details] [diff] [review] updated with changes from comment 6 >diff --git a/browser/components/preferences/advanced.xul b/browser/components/preferences/advanced.xul > <textbox id="cacheSize" type="number" size="4" max="1024" >- aria-labelledby="useCacheBefore cacheSize useCacheAfter"/> >+ aria-labelledby="limitCacheSizeBefore cacheSize limitCacheSizeAfter"/> aria-labelledby references IDs, not entities, so this change needs to be reverted too. r=me with that.