The default bug view has changed. See this FAQ.

preferences ui displays wrong value for max cache size

RESOLVED FIXED in Firefox 17

Status

()

Core
Networking: Cache
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: nwgh, Assigned: nwgh)

Tracking

Trunk
mozilla18
Points:
---

Firefox Tracking Flags

(firefox17 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Assignee: nobody → hurley
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!
Attachment #655797 - Attachment is obsolete: true
Attachment #667543 - Flags: review?(michal.novotny)
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.
Attachment #667543 - Flags: review?(michal.novotny) → review-
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
Attachment #667543 - Attachment is obsolete: true
Attachment #668069 - Flags: review?(michal.novotny)
Attachment #668069 - Flags: review?(michal.novotny) → review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/bcf8b2fac636
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/bcf8b2fac636
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18

Comment 6

5 years ago
This doesn't seem to work on my existing nightly-profile. Works with a new profile.
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.
Attachment #668069 - Flags: approval-mozilla-beta?
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.
Attachment #668069 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/e7395cee09d3
status-firefox17: --- → fixed

Comment 10

5 years ago
In nightly, the "Limit cache to" box still displays 1024mb (with existing profile).
You need to log in before you can comment on or make changes to this bug.