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: OFFLINE UNTIL 27 FEB 2017 Nicholas Hurley [:nwgh][:hurley] (also hurley@todesschaf.org)
:
: Patrick McManus [:mcmanus]
Mentors:
Depends on:
Blocks: 783755
  Show dependency treegraph
 
Reported: 2012-08-27 15:44 PDT by OFFLINE UNTIL 27 FEB 2017 Nicholas Hurley [:nwgh][:hurley] (also hurley@todesschaf.org)
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, OFFLINE UNTIL 27 FEB 2017 Nicholas Hurley [:nwgh][:hurley] (also hurley@todesschaf.org)
no flags Details | Diff | Splinter Review
patch (3.06 KB, patch)
2012-10-03 10:48 PDT, OFFLINE UNTIL 27 FEB 2017 Nicholas Hurley [:nwgh][:hurley] (also hurley@todesschaf.org)
michal.novotny: review-
Details | Diff | Splinter Review
patch (3.28 KB, patch)
2012-10-04 10:43 PDT, OFFLINE UNTIL 27 FEB 2017 Nicholas Hurley [:nwgh][:hurley] (also hurley@todesschaf.org)
michal.novotny: review+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description User image OFFLINE UNTIL 27 FEB 2017 Nicholas Hurley [:nwgh][:hurley] (also hurley@todesschaf.org) 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 User image OFFLINE UNTIL 27 FEB 2017 Nicholas Hurley [:nwgh][:hurley] (also hurley@todesschaf.org) 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 User image 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 User image OFFLINE UNTIL 27 FEB 2017 Nicholas Hurley [:nwgh][:hurley] (also hurley@todesschaf.org) 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 User image OFFLINE UNTIL 27 FEB 2017 Nicholas Hurley [:nwgh][:hurley] (also hurley@todesschaf.org) 2012-10-05 12:31:04 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/bcf8b2fac636
Comment 5 User image Ed Morley [:emorley] 2012-10-06 12:47:35 PDT
https://hg.mozilla.org/mozilla-central/rev/bcf8b2fac636
Comment 6 User image 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 User image OFFLINE UNTIL 27 FEB 2017 Nicholas Hurley [:nwgh][:hurley] (also hurley@todesschaf.org) 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 User image 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 User image OFFLINE UNTIL 27 FEB 2017 Nicholas Hurley [:nwgh][:hurley] (also hurley@todesschaf.org) 2012-10-29 15:49:16 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/e7395cee09d3
Comment 10 User image 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.