442810 - update the UI for app cache changes

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect, P2)

Description

[Dave Camp (:dcamp)]

The patch in bug 442806 disables offline cache size selection in the UI.  We need to fix that.

Comment 1

[Dave Camp (:dcamp)]

Attachment: v1

Comment 2

[Dave Camp (:dcamp)]

To be clear, we still show the size of globalStorage in that dialog, but the size of the cache items isn't included.

Comment 3

[Shawn Wilsher :sdwilsh]

Marking as a P2 blocker since I don't think this needs to block beta 3

Comment 4

[Honza Bambas (:mayhemer)]

Comment on attachment 350941 [details] [diff] [review]
v1

Are you sure of this line? "var groups = cacheService.getGroups({});". getGroups is defined as void with two out parameters. Shouldn't it be "var groups = {}; cacheService.getGroups(groups, {});" ?

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 350941 [details] [diff] [review]
v1

>diff -r 8c541b023937 browser/base/content/browser.js

>   _checkUsage: function(aURI) {

>+      var cacheService = Components.classes["@mozilla.org/network/application-cache-service;1"].
>+                         getService(Components.interfaces.nsIApplicationCacheService);
>+      var groups = cacheService.getGroups({});
>+      var usage = this._getOfflineAppUsage(aURI.asciiHost, groups);

Wouldn't it make more sense to just let _getOfflineAppUsage get the groups, since it's already using nsIApplicationCacheService anyways? Same comment applies to the version in browser/components/preferences/advanced.js .

It also doesn't appear to be possible to have multiple groups for a given host, so I'm not sure why the code tries to handle it.

I'm not really familiar with the application cache stuff, but having to create nsIURIs for all groups just to get/clear the cache for a given host seems inefficient. Is there a reason we can't rather have an API more like:
nsIApplicationCacheService.getUsage(in string group)
nsIApplicationCacheService.discardCachesForGroup(in string group)

and just call them with makeURI(host).asciiHost as the group?

Canceling r? to get these questions answered, feel free to re-request if this is still what we want to go with.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #4)
> Are you sure of this line? "var groups = cacheService.getGroups({});".
> getGroups is defined as void with two out parameters. Shouldn't it be "var
> groups = {}; cacheService.getGroups(groups, {});" ?

Arrays are handled differently. The second array outparam becomes the return value when called from JS, so the patch is correct.

See e.g. http://weblogs.mozillazine.org/weirdal/XPCOM/XPCOM_CheatSheet.xml#pass-array

Comment 7

[Honza Bambas (:mayhemer)]

(In reply to comment #5)
There can be more then just a single group (group = manifest URI) for a single host. For example: http://example.com/apps/app1.manifest and http://example.com/apps/app2.manifest. Those are two different groups on the same host.

To select all groups belonging to a single host we have to parse each group URI to get the host from it (what the patch already does) or store the host to the database in a separate column. It would then have to be updated during every manifest update (which might be often) to safe makeURI during delete operation invoked by user ones a year. Just my opinion.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

OK, so the "groups" you get from getGroups() are really just URI specs pointing to cache manifests? None of the interface docs really describe what a "group" is so that wasn't clear. I retract my naive API comments! First comment still applies though, looks OK otherwise.

Comment 9

[Honza Bambas (:mayhemer)]

Comment on attachment 350941 [details] [diff] [review]
v1

For me this is OK.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 350941 [details] [diff] [review]
v1

Looks fine with the IIDs on those interfaces changed.

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

dcamp: any progress on getting this checked in? is it late-compat?

Comment 12

[Dave Camp (:dcamp)]

Yeah, late-compat.

Gavin wanted a change to one of the methods, I'll put a new one up today.

Comment 13

[Dave Camp (:dcamp)]

Attachment: v2

Fixed the iid stuff, and made groups an optional argument to getOfflineAppUsage().

I prefer to keep passing groups in for the prefs dialog, because we call that once per host with the permission, and getGroups() hits the DB.  I'll post an interdiff of the getOfflineAppUsage changes.

Comment 14

[Dave Camp (:dcamp)]

Attachment: interdiff (missing iid changes)

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 361408 [details] [diff] [review]
v2

(In reply to comment #13)
> I prefer to keep passing groups in for the prefs dialog, because we call that
> once per host with the permission, and getGroups() hits the DB.

Ah, makes sense - I hadn't noticed that caller was in a loop.

Comment 16

[Dave Camp (:dcamp)]

http://hg.mozilla.org/mozilla-central/rev/ad173cf77112
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d2d01a91f8e0