Closed
Bug 1320551
Opened 8 years ago
Closed 8 years ago
Fix Advanced prefs Disk Space behavior
Categories
(Thunderbird :: Preferences, defect)
Thunderbird
Preferences
Tracking
(thunderbird52+ fixed, thunderbird53 fixed)
RESOLVED
FIXED
Thunderbird 53.0
People
(Reporter: alta88, Assigned: Paenglab)
References
Details
Attachments
(2 files, 3 obsolete files)
(deleted),
patch
|
jorgk-bmo
:
review+
Paenglab
:
feedback+
jorgk-bmo
:
approval-comm-beta+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jorgk-bmo
:
review+
jorgk-bmo
:
approval-comm-aurora+
|
Details | Diff | Splinter Review |
1. Clear Now button does not actually work after the cache2 conversion work, it should copy Fx here:
https://dxr.mozilla.org/mozilla-central/rev/2a0abcff5cfce087c12f3e4820b5e8b773cffaca/browser/components/preferences/in-content/advanced.js#460
2. The Tb UI should match the Fx UI, displaying the size, with a checkbox and similar wording.
3. This needs to go into 52.
Blocks: mail-cache2
tracking-thunderbird52:
--- → ?
Assignee | ||
Comment 1•8 years ago
|
||
This patch makes the button working again.
I tried to add the sentence with the actual used space, but failed. Maybe something for a followup bug.
Comment 2•8 years ago
|
||
My apologies, I overlooked this when cutting over to cache2.
(In reply to Richard Marti (:Paenglab) from comment #1)
> This patch makes the button working again.
What's the symptom of it not working?
> I tried to add the sentence with the actual used space, but failed.
How far did you get? Alta88 made it part of the bug, so it would be good if you/we could fix it now and land those strings while there is still time.
Assignee | ||
Comment 3•8 years ago
|
||
(In reply to Jorg K (GMT+1) from comment #2)
> My apologies, I overlooked this when cutting over to cache2.
>
> (In reply to Richard Marti (:Paenglab) from comment #1)
> > This patch makes the button working again.
> What's the symptom of it not working?
I made about:cache as my start page. Then you see the Storage in use. Press the "Clear now" button and load the start page again -> no change. With this patch it's "0 Kib".
> > I tried to add the sentence with the actual used space, but failed.
> How far did you get? Alta88 made it part of the bug, so it would be good if
> you/we could fix it now and land those strings while there is still time.
I copied the code from browser (updateTotalSiteDataSize function and it's calls and the label in xul file) added DownloadUtils.jsm reference and the strings in locale. But got always string parse error. Without the string parsing there was also no value shown.
If we want this we need to do it fast because of the strings. Maybe you can do this?
Comment 4•8 years ago
|
||
Can you please add or sent that patch? I hope you still have it.
Comment 5•8 years ago
|
||
s/sent/send/
Comment 6•8 years ago
|
||
Comment on attachment 8815453 [details] [diff] [review]
clearCache.patch
This is a good start. We also need to adapt
mail/base/content/sanitize.js
from
browser/base/content/sanitize.js#206
Can you please add this and submit this as beta patch. The change-over to cache2 happened at TB 51, so I'll uplift the beta patch straight away.
For TB 52 and TB 53 we may match FF. It would be great to have your WIP patch.
Attachment #8815453 -
Flags: review?(jorgk) → feedback+
Comment 7•8 years ago
|
||
Here's the beta version, please review.
Attachment #8815453 -
Attachment is obsolete: true
Attachment #8815660 -
Flags: review+
Attachment #8815660 -
Flags: feedback?(richard.marti)
Assignee | ||
Comment 8•8 years ago
|
||
Comment on attachment 8815660 [details] [diff] [review]
clearCache.patch (beta version)
Not tested, but looks like I made it today.
Attachment #8815660 -
Flags: feedback?(richard.marti) → feedback+
Assignee | ||
Comment 9•8 years ago
|
||
This is the wip with the cache size display.
Attachment #8815698 -
Flags: feedback?(jorgk)
Comment 10•8 years ago
|
||
Comment on attachment 8815660 [details] [diff] [review]
clearCache.patch (beta version)
(In reply to Richard Marti (:Paenglab) from comment #8)
> Not tested, ...
I tested it for both "Clear Now" in options and "Clear History".
Beta (TB 51):
https://hg.mozilla.org/releases/comm-beta/rev/43149a0d3b0850bdf4292ed0e27def00a94ad698
Attachment #8815660 -
Flags: approval-comm-beta+
Comment 11•8 years ago
|
||
Comment on attachment 8815698 [details] [diff] [review]
clearCache.patch (wip version)
Very nice. Only that the whole thing leaves me confused. Why is this WIP? Mild testing shows it working.
Also:
(In reply to Richard Marti (:Paenglab) from comment #3)
> I copied the code from browser (updateTotalSiteDataSize function and it's
> calls and the label in xul file) added DownloadUtils.jsm reference and the
> strings in locale. But got always string parse error. Without the string
> parsing there was also no value shown.
So why is this working now when the first attempt didn't work to the point that it seemed too hard and was abandoned.
I'd give this r+ unless there's something I should know that you haven't told me ;-)
One thing I'd change to be closer to FF:
I'd put the this.updateActualCacheSize(); after the this.initTelemetry(); call since that's where it is in FF.
Once again, very nice job and I'm sorry I overlooked this.
Attachment #8815698 -
Flags: feedback?(jorgk) → feedback+
Assignee | ||
Comment 12•8 years ago
|
||
Strange, I also don't know why it did not work. Maybe because I only edited in the obj-dir.
I removed two not needed imports and moved the this.updateActualCacheSize(); as proposed. But to be sure,please check this patch again.
Attachment #8815698 -
Attachment is obsolete: true
Attachment #8815843 -
Flags: review?(jorgk)
Comment 13•8 years ago
|
||
Thanks.
You need the
Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
I'll land this now with a slightly changed commit message.
Attachment #8815843 -
Attachment is obsolete: true
Attachment #8815843 -
Flags: review?(jorgk)
Attachment #8815857 -
Flags: review+
Comment 14•8 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 53.0
Comment 15•8 years ago
|
||
Comment on attachment 8815857 [details] [diff] [review]
clearCache.patch (final version)
This needs to go into TB 52 currently in Aurora. Uplift early next week together with all the other bugs that have string changes.
So approval‑comm‑aurora? for now.
Attachment #8815857 -
Flags: approval-comm-aurora?
Comment 16•8 years ago
|
||
Comment on attachment 8815857 [details] [diff] [review]
clearCache.patch (final version)
Review of attachment 8815857 [details] [diff] [review]:
-----------------------------------------------------------------
::: mail/base/content/sanitize.js
@@ +81,5 @@
> try {
> // Cache doesn't consult timespan, nor does it have the
> // facility for timespan-based eviction. Wipe it.
> + let cache = Components.classes["@mozilla.org/netwerk/cache-storage-service;1"]
> + .getService(Ci.nsICacheStorageService);
Could have used Services.cache2 for these (https://dxr.mozilla.org/comm-central/rev/bad312aefb42982f492ad2cf36f4c6c3d698f4f7/mozilla/toolkit/modules/Services.jsm#68)...
Comment 17•8 years ago
|
||
Comment 18•8 years ago
|
||
Comment on attachment 8815857 [details] [diff] [review]
clearCache.patch (final version)
Aurora (TB 52):
https://hg.mozilla.org/releases/comm-aurora/rev/d59fc6f32e66b24e29e7f42a72d05ab94d7142f8
Attachment #8815857 -
Flags: approval-comm-aurora? → approval-comm-aurora+
Updated•8 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•