Closed Bug 975362 Opened 7 years ago Closed 6 years ago

Support HTTP cache v2 API in the UI

Categories

(SeaMonkey :: Page Info, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.27

People

(Reporter: neil, Assigned: neil)

References

Details

Attachments

(3 files, 1 obsolete file)

The HTTP cache v2 provides a new API. Calls to the new API are transparently redirected to the old cache while it is still enabled, but also work with the new cache. Calls to the old API fail if the new cache is enabled.
Attached patch Proposed patch (obsolete) — Splinter Review
Sorry, I'm not going to bother fixing the tests.

Rather fortunately, one of the callbacks has the same name, and the callback info even has the same dataSize attribute, which is why there aren't as many changes as you might imagine.

I wanted to avoid using Services.cache2 in case it gets renamed in future.
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #8379651 - Flags: review?(bugzilla)
For reference: Bug 913807 introduced the new cache API.
(In reply to neil@parkwaycc.co.uk from comment #1)
> Sorry, I'm not going to bother fixing the tests.
Could you at least disable the relevant tests?
Depends on: 913807
The tests won't break until the HTTP cache v2 is turned on by default.
Neil: Should about:cache also use the new backend then? I'm wondering a bit as with browser.cache.use_new_backend set to 1 about:cache says:
"Disk cache device
Number of entries: 	10817
Maximum storage size: 	358400 KiB
Storage in use: 	340196 KiB

Offline cache device
Number of entries: 	0
Maximum storage size: 	512000 KiB
Storage in use: 	0 KiB"


And the cache pref pane says:
"Your cache is currently using 2,4 GB of disk space"

Does that number include more than what's listed on about:cache? Not sure if this "problem" is really related to the new cache backend though.
(In reply to Frank Wein from comment #5)
> Neil: Should about:cache also use the new backend then?

That's bug 916052.
I'm a bit puzzled now, in Page Info it seems to say for all pictures (media) on a website "Not cached", with the old backend it said "Disk cache". I'll check with Firefox now what it says.
Ok, Firefox has no direct cache entry in Page Info, but if it's cached, it will display the file size in KB and that seems to work here. So I think there might be some bug with the patch, I used http://www.seamonkey-project.org/start/ to test this.
Attached image Screenshot
Here you see what I mean: On the left side Firefox, on the right side SeaMonkey (both built locally and both with the new backend pref set to 1). On the left you can see Firefox knows about the file size of the image, on the right side nothing about the file size of the image is known.
Attached patch Fixed patchSplinter Review
The old cache reads an uninitialised variable, and this happened to work for me. The new cache doesn't, so I have to explicitly return the correct value ;-)
Attachment #8379651 - Attachment is obsolete: true
Attachment #8379651 - Flags: review?(bugzilla)
Attachment #8383602 - Flags: review?(bugzilla)
Comment on attachment 8383602 [details] [diff] [review]
Fixed patch

Patch looks good and works fine. One thing I noticed though, I got this error when looking at the offline apps pref panel in my test profile:
Timestamp: 07.03.2014 12:04:03
Error: NS_ERROR_NOT_IMPLEMENTED: Component returned failure code: 0x80004001 (NS_ERROR_NOT_IMPLEMENTED) [nsICacheStorage.asyncVisitStorage]
Source File: chrome://communicator/content/pref/pref-offlineapps.js
Line: 62

Not sure why, does one need to explicitly enable offline apps storage with the new cache? With the old backend this error does not seem to happen.
Actually I found the answer ;) http://hg.mozilla.org/mozilla-central/annotate/b0e7f63c2138/netwerk/cache2/AppCacheStorage.cpp#l148 it's not implemented...yet?! So currently one cannot look at the app cache storage looks like.
Attachment #8383602 - Flags: review?(bugzilla) → review+
Comment on attachment 8383602 [details] [diff] [review]
Fixed patch

> +++ b/suite/browser/metadata.js
> +        Components.utils.import("resource://gre/modules/NetUtil.jsm");
We already import this module in makeHrefAbsolute(). How about moving the import to the top of the file?

> +++ b/suite/browser/pageinfo/pageInfo.js
> +const diskCacheStorage =
> +    Components.classes["@mozilla.org/netwerk/cache-storage-service;1"]
> +              .getService(Components.interfaces.nsICacheStorageService)
> +              .diskCacheStorage({ isPrivate: opener.gPrivate }, false);
Services.cache2.diskCacheStorage(...);

> +++ b/suite/common/pref/pref-cache.js
> +  Components.classes["@mozilla.org/netwerk/cache-storage-service;1"]
> +            .getService(Components.interfaces.nsICacheStorageService).clear();
Services.cache2.clear()

> -  Services.cache.visitEntries(visitor);
> +  Components.classes["@mozilla.org/netwerk/cache-storage-service;1"]
> +            .getService(Components.interfaces.nsICacheStorageService)
> +            .appCacheStorage({}, null).asyncVisitStorage(visitor, false);
Services.cache2.appCacheStorage(...)

> +++ b/suite/common/pref/pref-offlineapps.js
See pref-cache.js

> +++ b/suite/modules/Sanitizer.jsm
Services.jsm is imported so we can use Services.cache2
Pushed comm-central changeset babfac82821d.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.27
Unfortunately I accidentally landed these changes from bug 514280 along with this patch, so here's hoping I get post-facto review.
Attachment #8392749 - Flags: review?(Pidgeot18)
Attachment #8392749 - Flags: review?(Pidgeot18) → review+
(In reply to Frank Wein from comment #11)
> http://hg.mozilla.org/mozilla-central/annotate/b0e7f63c2138/netwerk/cache2/
> AppCacheStorage.cpp#l148 it's not implemented...yet?! So currently one
> cannot look at the app cache storage looks like.

Oops, I hadn't realised this. Do you want me to back out the offline app changes on 2.27 and later?
(Bah, I didn't see where my focus had gone and accidentally submitted by mistake.)

(In reply to Frank Wein from comment #11)
> http://hg.mozilla.org/mozilla-central/annotate/b0e7f63c2138/netwerk/cache2/
> AppCacheStorage.cpp#l148 it's not implemented...yet?! So currently one
> cannot look at the app cache storage looks like.

Oops, I hadn't realised this. Do you want me to back out the offline app changes on 2.27 and later?
Flags: needinfo?(bugzilla)
Looks like Bug 916052 fixed this for Gecko 32/SeaMonkey 2.29. So yes, back out the changes to pref-offlineapps.js on beta and aurora. Trunk is fine. And on beta and aurora we don't use the new backend anyway (by default).
Status: RESOLVED → REOPENED
Flags: needinfo?(bugzilla)
Resolution: FIXED → ---
(In reply to Frank Wein from comment #17)
> back out the changes to pref-offlineapps.js on beta and aurora.

Sorry I don't think this happened, but it's too late now.
Status: REOPENED → RESOLVED
Closed: 7 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.