397416 - raise the globalStorage quota for offline apps.

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[Dave Camp (:dcamp)]

Attachment: v1

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 1

[Neil Deakin]

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'.

Comment 2

[Dave Camp (:dcamp)]

Attachment: replaced "soft quota" with "warn quota"

Comment 3

[Dave Camp (:dcamp)]

(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 4

[Daniel Veditz [:dveditz]]

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

Comment 5

[Daniel Veditz [:dveditz]]

filed bug 400092 on the scarcity of PM types

Comment 6

[Dave Camp (:dcamp)]

Attachment: v2

Comment 7

[Dave Camp (:dcamp)]

Fixed dveditz' review comments.

Asking for blocking, this is pretty important for useful offline apps.

Comment 8

[Neil Deakin]

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

Seems OK.

Comment 9

[Daniel Veditz [:dveditz]]

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

sr=dveditz

Comment 10

[Dave Camp (:dcamp)]

Not landing this yet due to a dependency on bug 402272

Comment 11

[Mike Schroepfer]

Why is this a P5?

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 13

[Dave Camp (:dcamp)]

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

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 15

[Dave Camp (:dcamp)]

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

Comment 16

[Dave Camp (:dcamp)]

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