1320551 - Fix Advanced prefs Disk Space behavior

Status: RESOLVED FIXED

(Thunderbird :: Preferences, defect)

Description

[alta88]

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.

Comment 1

[Richard Marti (:Paenglab)]

Attachment: clearCache.patch

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

[Jorg K (CEST = GMT+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 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.

Comment 3

[Richard Marti (:Paenglab)]

(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

[Jorg K (CEST = GMT+2)]

Can you please add or sent that patch? I hope you still have it.

Comment 5

[Jorg K (CEST = GMT+2)]

s/sent/send/

Comment 6

[Jorg K (CEST = GMT+2)]

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.

Comment 7

[Jorg K (CEST = GMT+2)]

Attachment: clearCache.patch (beta version)

Here's the beta version, please review.

Comment 8

[Richard Marti (:Paenglab)]

Comment on attachment 8815660 [details] [diff] [review]
clearCache.patch (beta version)

Not tested, but looks like I made it today.

Comment 9

[Richard Marti (:Paenglab)]

Attachment: clearCache.patch (wip version)

This is the wip with the cache size display.

Comment 10

[Jorg K (CEST = GMT+2)]

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

Comment 11

[Jorg K (CEST = GMT+2)]

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.

Comment 12

[Richard Marti (:Paenglab)]

Attachment: clearCache.patch

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.

Comment 13

[Jorg K (CEST = GMT+2)]

Attachment: clearCache.patch (final version)

Thanks.

You need the
Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");

I'll land this now with a slightly changed commit message.

Comment 14

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/8652090829b8285cc6cfd3f9bffa3e2d7fddba5c

Comment 15

[Jorg K (CEST = GMT+2)]

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.

Comment 16

[:aceman]

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

[Jorg K (CEST = GMT+2)]

We did copy/paste:
https://dxr.mozilla.org/mozilla-central/rev/741a720c98cdb92c229376be0badbf036f653bff/browser/base/content/sanitize.js#207
https://dxr.mozilla.org/mozilla-central/rev/741a720c98cdb92c229376be0badbf036f653bff/browser/components/preferences/in-content/advanced.js#463

Sorry ;-(

Comment 18

[Jorg K (CEST = GMT+2)]

Comment on attachment 8815857 [details] [diff] [review]
clearCache.patch (final version)

Aurora (TB 52):
https://hg.mozilla.org/releases/comm-aurora/rev/d59fc6f32e66b24e29e7f42a72d05ab94d7142f8