The current 5mb quota for globalStorage isn't enough for a useful offline web app. What beltzner proposed was something like: * By default give sites the same 5mb (controlled by the dom.storage.default_quota pref) * For pages which the user has elected to allow access to offline APIs: ** Give an increased, generous quota ** Warn the user if the usage exceeds a certain amount The attached patch implements this with a prefs for the offline app quota and the "soft" quota. By default it gives 200mb of storage and warns after 50mb, but we should talk about what the best numbers would be.

Comment on attachment 282183 [details] [diff] [review] v1 >+ if (uri && nsContentUtils::OfflineAppAllowed(uri)) { Did you mean to include the changes to nsContentUtils that add this method? Also, it might be clearer to use a term like 'warn quota' instead of 'soft quota'.

(In reply to comment #1) > (From update of attachment 282183 [details] [diff] [review]) > >+ if (uri && nsContentUtils::OfflineAppAllowed(uri)) { > > Did you mean to include the changes to nsContentUtils that add this method? Sorry; that method is added by the patch in 394352, which hasn't been committed yet. > Also, it might be clearer to use a term like 'warn quota' instead of 'soft > quota'. Done.

Comment on attachment 283457 [details] [diff] [review] replaced "soft quota" with "warn quota" >+static const char kOfflineAppQuota[] = "offline-apps.storage_quota"; >+static const char kOfflineAppWarnQuota[] = "offline-apps.warn_quota"; Take this as an optional nit, but my preference is to group related preferences hierarchically. (non-optional: don't mix hyphen and underscore separators. I know our prefs are a horrible mess, but the One True Way is lower-case hyphen separated names.) offline-apps.quota.warn-limit offline-apps.quota.max leaving name-space for a future per-site offline-apps.quota.domain.www.foo.com of course it's all just strings, the dots don't really "mean" anything so I can't get too worked up about it. Another thought, DOM storage itself may eventually want per-site quotas (at least I want them: I'd like to shrink the default way way down--effectively disable it--but enable it for sites I use), and in that case you might combine the two per-site quota lists under the dom.storage. namespace. If there's a specific per-site size for dom.storage then use that both offline and on (and don't bother with warning?). >+GetQuota(const nsAString &aDomain, PRInt32 *aQuota, PRInt32 *aWarnQuota) ... >+ if (uri && nsContentUtils::OfflineAppAllowed(uri)) { ... >+ if (NS_SUCCEEDED(permissionManager->TestExactPermission(uri, "offline-app-high-usage", &perm)) Here we have a problem, the current PermissionManager only supports eight "types", and we were already using four of them in Firefox 2. Here you're using 50% of the remaining, never mind any extensions out there that might also be using the permission manager. We need to fix that (I'll file a bug), but in the meantime you should use only a single "offline-app" PM type. You can define extra permissions (up to 15, currently) so you could use DENY, ALLOW and a new ALLOW-NO-WARN. Then you could save a permission manager lookup by making a single call and checking the permission type here. Don't care if it's an explicit TestExactPermission() call or if you want to tweak the nsContentUtils helper function to return three states instead of a boolean. If you go with the dom.storage per-site quota idea you could accomplish the no-warn by setting an explicit site quota to the offline-app default. This might be the best bet, everyone wins: on-line apps get adjustable quotas, off-line apps get per-site adjustable quotas (albeit no UI for either), and you don't have to tweak your current offline-enabled helper routine or PM types. If it's got a site quota use it with no warning, and if it doesn't use the normal warning. sr-minus primarily because of the PermissionManager type-scarcity concern

filed bug 400092 on the scarcity of PM types

Fixed dveditz' review comments. Asking for blocking, this is pretty important for useful offline apps.

Comment on attachment 290456 [details] [diff] [review] v2 Seems OK.

Comment on attachment 290456 [details] [diff] [review] v2 sr=dveditz

Not landing this yet due to a dependency on bug 402272

Why is this a P5?

I'm fine with raising this a bit if dcamp thinks that's the right thing

It's pretty important for decent offline apps, and the patch is there waiting on a dependency. So I'd up the priority.

You're welcome to do so yourself :) but I gave it a P3 for now.

We're going to need to decide how to handle this in the wake of 367373.

I brought up the concerns from comment #15 up in bug 367373 and landed this. Checking in dom/src/storage/Makefile.in; /cvsroot/mozilla/dom/src/storage/Makefile.in,v <-- Makefile.in new revision: 1.5; previous revision: 1.4 done Checking in dom/src/storage/nsDOMStorage.cpp; /cvsroot/mozilla/dom/src/storage/nsDOMStorage.cpp,v <-- nsDOMStorage.cpp new revision: 1.21; previous revision: 1.20 done Checking in dom/src/storage/nsDOMStorageDB.cpp; /cvsroot/mozilla/dom/src/storage/nsDOMStorageDB.cpp,v <-- nsDOMStorageDB.cpp new revision: 1.11; previous revision: 1.10 done Checking in dom/src/storage/nsDOMStorageDB.h; /cvsroot/mozilla/dom/src/storage/nsDOMStorageDB.h,v <-- nsDOMStorageDB.h new revision: 1.10; previous revision: 1.9 done Checking in modules/libpref/src/init/all.js; /cvsroot/mozilla/modules/libpref/src/init/all.js,v <-- all.js new revision: 3.718; previous revision: 3.717 done Checking in uriloader/prefetch/nsIOfflineCacheUpdate.idl; /cvsroot/mozilla/uriloader/prefetch/nsIOfflineCacheUpdate.idl,v <-- nsIOfflineCacheUpdate.idl new revision: 1.3; previous revision: 1.2 done