I notice that nsCacheService::SetDiskCacheCapacity() is getting called at *each keystroke* of the user when setting a fixed-size for the cache. I.e. if you erase the value we set the cache size to 0, then as you type "128" we set it to "1", "12", then "128". This will cause the whole cache to get evicted if anything hits it in the interim. Gavin: I assume there's a way to only get the event once the dialog is closed, or the control goes out of focus, or something less granular than per-keystroke?

You should be able to just add instantApply="false" to the <preference> element for the capacity pref. I'm not sure offhand whether that would have any other implications, need to give it some thought.

We'd need to verify on OSX/windoze/Linux that the setting gets updated if the window is closed in any intuitive way by the user ("Close" button obviously must work, but what happens if user hits escape, or clicks window close button?)

That is all handled by the pref window code, so it should work fine.

http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/advanced.xul#257 is the relevant code that requires changing according to comment 1. I'm happy to work with someone to investigate whether that addresses the issue described here.

Would this bug be okay for a beginner? Conceptually, I understand what the problem is and the proposed solution seems to be mostly setting a boolean and doing extensive testing. I have a Linux and Windows 7 installation I can test this on, as soon as I have the nightly build up and running under Windows. @jdm: Would you be willing to mentor me on this one?

Absolutely! I have a Mac setup, so I can perform any tests you're missing.

Hi! I am interested in working on this bug too! But is this assigned already?

Yes, Diana has been investigating it.

Completed testing under Windows, results so far: Behavior is as expected. Via Tools > Options: When changing number spinner, cache is not set until the window is closed using Ok button. When *not* having changed number spinner, cache is not set when window is closed using Ok button. When pressing Cancel or Escape, cache is not set. Minor detail: When via Tools > Options leaving the value unchanged BUT having toyed with the number spinner (just press up once and then down again to get the same value) the cache *is* set.

Under Linux, instantApply="false" doesn't seem to have any effect. Possibly related: bug 469673

Testing under Linux, results so far: Bug 469673 indeed prevents instantApply="false" from having any effect. However, setting browser.preferences.instantApply="false" via about:config then cache setting works as intended, so instantApply="false" does put in place the required code for Windows and Linux to fix 596778, but needs 469673 to be fixed as well.

See https://bugzilla.mozilla.org/show_bug.cgi?id=469673#c14 for discussion on whether or not to have instantApply attribute on elements.

First apply patch 600759 for bug 469673, then apply this patch which uses the new functionality.

I just posted in bug 469673 suggesting that I don't think the fix there is suitable. I think that to fix this bug we can just avoid linking the textbox to the <preference> element directly, and implement some custom code to apply the preference after a timeout. (We used to support type="timed" textboxes, which would have been ideal for this, but those were deprecated by bug 449045, and the proposed alternative type="search" isn't suitable for this use case. I guess you might need to implement the equivalent behavior yourself.)

(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #14) > I just posted in bug 469673 suggesting that I don't think the fix there is > suitable. I think that to fix this bug we can just avoid linking the textbox > to the <preference> element directly, and implement some custom code to > apply the preference after a timeout. Unless the timeout is really short (only taking care of users typing quickly enough), you'd still need to save the pref when the window closes before the timeout fires. Of course you wouldn't need the timeout at all at that point.

The goal of this bug is to avoid calling SetDiskCacheCapacity() (via the pref observer) after every single key press. That would only require a short (~1s) delay. You're right that we also need to force any pending saves on window close.

As Dão indicates, it is enough to just do it on window close.

Ah, I see. That's potentially confusing behavior when the rest of the dialog is instantApply, but I guess that's not a big deal (not like there's much reason for users to ever be changing this pref, let alone doing it frequently or expecting it to apply instantly).

Turns out that I'm not a great mentor for this bug that is more complicated than it first appeared.

I've used the timeout approach in instant-apply dialogs to avoid repeatedly performing expensive or dangerous operations before the user finishes typing. A relatively long timer shouldn't be a problem here, since a user could care less whether a cache size is reset now or in five seconds time. Easy enough to sweep up the timer when the dialog closes if the implementation leaves a danger of it getting garbage collected. The user could still wipe the entire cache, but I guess if they set the value to "1" and then sit there for 5 seconds, that is their choice. Or do we simply wait for focus to leave that field? Not instant-apply any more, and wouldn't be my choice, but entirely avoids the problem.

Anything that prevents us from blowing away the entire cache when you start replacing the value would be a big improvement, and I'd love to see a patch. Waiting for focus to leave field would be ideal (at least in the sense that there'd be no race between user finishing typing and the timer), but I'd happily take a timeout fix. Implementor's choice.

Would this work?

Comment on attachment 8581150 [details] [diff] [review] patch v2 Review of attachment 8581150 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for working on this! I don't know my XUL-fu well enough to say if this is the right fix--so 'onchange' events somehow include a timeout? (i.e if I type "1024" for the cache size, it's not going to fire events for "1", "10", "102", just "1024", as long as I type it within some time period?)

onchange doesn't fire until focus moves to another element. There is no timeout involved, you can sit all day in the same textbox and no pref will be applied. The moment you leave, the cache size will be updated.

Great, that's perfect.

Comment on attachment 8581150 [details] [diff] [review] patch v2 Review of attachment 8581150 [details] [diff] [review]: ----------------------------------------------------------------- (Jared is a bit busy, so I'm stealing this review.) We actually have two different versions of preferences UI at the moment - this one in a dialog, and the newer version in browser/components/preferences/in-content/ which is accessible via the about:preferences URL. You'll need to update the in-content version too. It should just be the same change to the same files though. ::: browser/components/preferences/advanced.js @@ +407,5 @@ > + * not only after the final value is entered. > + */ > + updateCachePref: function (aCacheValue) > + { > + this.writeCacheSize(aCacheValue); writeCacheSize() doesn't take a parameter. Don't think we need this function anyway - the onchange handler can just call writeCacheSize() directly, and move the comment to be for writeCacheSize() too.

Sorry for the delay, somehow I wasn't CCed to the bug so didn't see the replies. I'll take the bug, Diana has no bugzilla activity since 2012.

(In reply to Blair McBride [:Unfocused] (UNAVAILABLE) from comment #27) > We actually have two different versions of preferences UI at the moment - > this one in a dialog, and the newer version in > browser/components/preferences/in-content/ which is accessible via the > about:preferences URL. You'll need to update the in-content version too. It > should just be the same change to the same files though. Thanks, done. > ::: browser/components/preferences/advanced.js > @@ +407,5 @@ > > + * not only after the final value is entered. > > + */ > > + updateCachePref: function (aCacheValue) > > + { > > + this.writeCacheSize(aCacheValue); > > Don't think we need this function > anyway - the onchange handler can just call writeCacheSize() directly, and > move the comment to be for writeCacheSize() too. Actually I forgot to write out the new value to the pref so that is what updateCachePref contains now (not just a trivial call to writeCacheSize).

Comment on attachment 8598900 [details] [diff] [review] 596778.patch Review of attachment 8598900 [details] [diff] [review]: ----------------------------------------------------------------- The comments are valid for both in-content and classic. The approach look fine to me, code can be merged a little bit: ::: browser/components/preferences/advanced.js @@ +55,5 @@ > document.getElementById("offlineAppsList") > .style.height = bundlePrefs.getString("offlineAppsList.height"); > > + var cacheSizeElem = document.getElementById("cacheSize"); > + cacheSizeElem.value = this.readCacheSize(); Since readCacheSize() is only used here, it's not worth keeping it around. Let's rather rename it to updateCacheSizeInputField() and move all of this code into it. Also, let's invoke it just before updateActualCacheSize() to group related code, so in the end we'll have: this.updateCacheSizeInputField(); this.updateActualCacheSize(); this.updateActualAppCacheSize(); @@ +409,5 @@ > + * We intentionally do not set preference="browser.cache.disk.capacity" > + * onto the textbox directly, as that would update the pref at each keypress > + * not only after the final value is entered. > + */ > + updateCachePref: function () updateCacheSizePref() @@ +412,5 @@ > + */ > + updateCachePref: function () > + { > + var preference = document.getElementById("browser.cache.disk.capacity"); > + preference.value = this.writeCacheSize(); Also, please merge writeCacheSize into this, it's not worth to have 2 helpers for a simple thing. the comment that is currently above writeCacheSize, can be moved before the conversion, to explain we are converting from MB to KB.

Thanks, something like this?

Comment on attachment 8602285 [details] [diff] [review] patch v4 Review of attachment 8602285 [details] [diff] [review]: ----------------------------------------------------------------- as usual, comments are valid for both flavors (dialog and in-content). ::: browser/components/preferences/advanced.js @@ +384,2 @@ > */ > + updateCacheSizeInputField: function () nit: ES6 supports shorthand format: updateCacheSizeInputField() see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Method_definitions @@ +386,3 @@ > { > + var cacheSizeElem = document.getElementById("cacheSize"); > + var cachePref = document.getElementById("browser.cache.disk.capacity"); nit: please use "let" in new (or modified) code @@ +399,2 @@ > */ > + updateCacheSizePref: function () updateCacheSizePref() @@ +401,5 @@ > { > + var cacheSizeElem = document.getElementById("cacheSize"); > + var cachePref = document.getElementById("browser.cache.disk.capacity"); > + // Converts the cache size as specified in UI (in MB) to KB. > + var intValue = parseInt(cacheSizeElem.value, 10); ditto

Thanks.

Please provide a Try link for this patch.

Sure, hopefully somebody pushes it to try server.

Thanks Jared. So did the necessary tests pass? Build should pass, as this was just JS change that would not break build.

Yeah, only one orange test suite in that try push and it's unrelated to your changes.

(In reply to Ian Nartowicz from comment #25) > onchange doesn't fire until focus moves to another element. There is no > timeout involved, you can sit all day in the same textbox and no pref will > be applied. The moment you leave, the cache size will be updated. What if you just close out completely? Assuming that's not a problem (or it's a fixable problem), would it not be better to apply the underlying change generally to all text fields?

(In reply to neil@parkwaycc.co.uk from comment #40) > (In reply to Ian Nartowicz from comment #25) > > onchange doesn't fire until focus moves to another element. There is no > > timeout involved, you can sit all day in the same textbox and no pref will > > be applied. The moment you leave, the cache size will be updated. > > What if you just close out completely? > > Assuming that's not a problem (or it's a fixable problem), would it not be > better to apply the underlying change generally to all text fields? Are you asking if onchange fires, for example, when you close the dialog/tab without explicitly leaving the textbox? That shouldn't be a problem, onchange is always going to fire before the dialog is destroyed and our preferemce should be applied. Or are you asking if this behaviour should be used automatically for all textboxes by some underlying change in Mozilla? That wouldn't be good, people want things to happen when textboxes change character by character. Except in this particular case, and perhaps other rare situations.