Last Comment Bug 786086 - preferences ui displays wrong value for max cache size
: preferences ui displays wrong value for max cache size
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking: Cache (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla18
Assigned To: Nicholas Hurley [:nwgh][:hurley]
:
: Patrick McManus [:mcmanus]
Mentors:
Depends on:
Blocks: 783755
  Show dependency treegraph
 
Reported: 2012-08-27 15:44 PDT by Nicholas Hurley [:nwgh][:hurley]
Modified: 2012-11-04 10:02 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
original patch from bug 783755 (4.90 KB, patch)
2012-08-27 15:44 PDT, Nicholas Hurley [:nwgh][:hurley]
no flags Details | Diff | Splinter Review
patch (3.06 KB, patch)
2012-10-03 10:48 PDT, Nicholas Hurley [:nwgh][:hurley]
michal.novotny: review-
Details | Diff | Splinter Review
patch (3.28 KB, patch)
2012-10-04 10:43 PDT, Nicholas Hurley [:nwgh][:hurley]
michal.novotny: review+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Nicholas Hurley [:nwgh][:hurley] 2012-08-27 15:44:10 PDT
Created attachment 655797 [details] [diff] [review]
original patch from bug 783755

In bug 783755, we fixed the operation of using the new max cache size (350MB). However, in the network tab of the advanced preferences UI, the incorrect old value is shown (even though the new value is in effect). We should fix that, to avoid confusing the users who may look in the advanced prefs UI.

The attached patch is the original one from bug 783755 which both fixes the functionality and the display. We need to extract the display-fixing code from this patch, decide on the right way to do it, and land it.
Comment 1 Nicholas Hurley [:nwgh][:hurley] 2012-10-03 10:48:28 PDT
Created attachment 667543 [details] [diff] [review]
patch

Here's a patch to make the display work just fine. Try is green, and the actual functionality works fine on my machine.

Michal, I'd like to get this landed before the merge day on Monday if at all possible. It's pretty much exactly what you gave feedback on in the other bug, so there shouldn't be any major surprises here. Thanks!
Comment 2 Michal Novotny (:michal) 2012-10-04 08:03:30 PDT
Comment on attachment 667543 [details] [diff] [review]
patch

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

(In reply to Nick Hurley [:hurley] from comment #1)
> Michal, I'd like to get this landed before the merge day on Monday if at all
> possible. It's pretty much exactly what you gave feedback on in the other
> bug, so there shouldn't be any major surprises here. Thanks!

I meant to always post the event nsDisableOldMaxSmartSizePrefEvent, not to create a new one. Sorry if it wasn't clear.

::: netwerk/cache/nsCacheService.cpp
@@ +1637,5 @@
>          NS_DispatchToMainThread(new nsDisableOldMaxSmartSizePrefEvent());
>      }
> +
> +    nsCOMPtr<nsIPrefBranch> branch = do_GetService(NS_PREFSERVICE_CONTRACTID);
> +    if (gService->mObserver->PermittedToSmartSize(branch, false)) {

Preference service must not be used off the main thread.
Comment 3 Nicholas Hurley [:nwgh][:hurley] 2012-10-04 10:43:27 PDT
Created attachment 668069 [details] [diff] [review]
patch

OK, here we go. Everything coalesced into one event, that is always dispatched to the main thread, to avoid locking issues. I'm running this through try since we've slightly changed the main functionality, but unit tests run locally + manual checking of the functionality look good.

https://tbpl.mozilla.org/?tree=Try&rev=663180cdb971
Comment 4 Nicholas Hurley [:nwgh][:hurley] 2012-10-05 12:31:04 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/bcf8b2fac636
Comment 5 Ed Morley [:emorley] 2012-10-06 12:47:35 PDT
https://hg.mozilla.org/mozilla-central/rev/bcf8b2fac636
Comment 6 Onno 2012-10-08 08:33:42 PDT
This doesn't seem to work on my existing nightly-profile. Works with a new profile.
Comment 7 Nicholas Hurley [:nwgh][:hurley] 2012-10-29 14:33:39 PDT
Comment on attachment 668069 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 709297
User impact if declined: The "Limit cache to" box in Prefs->Advanced->Network will display the incorrect size until/unless the user creates a new profile.
Testing completed (on m-c, etc.): On m-c and m-a
Risk to taking this patch (and alternatives if risky): Relatively low. If we opt not to take this patch (or its companion in bug 783755), we should back out bug 709297 AND add a patch to delete the pref created by the patch in that bug, to avoid the situation the patch was designed to avoid in the first place.
String or UUID changes made by this patch: None.

Note as mentioned above, both this and the patch in bug 783755 should be either approved or denied together.
Comment 8 Lukas Blakk [:lsblakk] use ?needinfo 2012-10-29 15:28:48 PDT
Comment on attachment 668069 [details] [diff] [review]
patch

As mentioned in bug 783755, this has been on central/aurora for a while so we'll take it in tomorrow's beta (Beta 4) so please land asap.
Comment 9 Nicholas Hurley [:nwgh][:hurley] 2012-10-29 15:49:16 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/e7395cee09d3
Comment 10 Onno 2012-10-29 16:09:35 PDT
In nightly, the "Limit cache to" box still displays 1024mb (with existing profile).

Note You need to log in before you can comment on or make changes to this bug.