Closed Bug 397416 Opened 15 years ago Closed 14 years ago

raise the globalStorage quota for offline apps.

Categories

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

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: dcamp, Assigned: dcamp)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch v1 (obsolete) — Splinter Review
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.
Attachment #282183 - Flags: review?(enndeakin)
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'.
Blocks: 398161
Attachment #282183 - Attachment is obsolete: true
Attachment #283457 - Flags: review?(enndeakin)
Attachment #282183 - Flags: review?(enndeakin)
(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.

Depends on: 394352
Attachment #283457 - Flags: review?(enndeakin) → review+
Attachment #283457 - Flags: superreview?(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
Attachment #283457 - Flags: superreview?(dveditz) → superreview-
filed bug 400092 on the scarcity of PM types
Attached patch v2Splinter Review
Attachment #283457 - Attachment is obsolete: true
Attachment #290456 - Flags: superreview?(dveditz)
Attachment #290456 - Flags: review?(enndeakin)
Fixed dveditz' review comments.

Asking for blocking, this is pretty important for useful offline apps.
Flags: blocking1.9?
Comment on attachment 290456 [details] [diff] [review]
v2

Seems OK.
Attachment #290456 - Flags: review?(enndeakin) → review+
Flags: blocking1.9? → blocking1.9+
Priority: -- → P5
Comment on attachment 290456 [details] [diff] [review]
v2

sr=dveditz
Attachment #290456 - Flags: superreview?(dveditz) → superreview+
Whiteboard: [needs landing]
Not landing this yet due to a dependency on bug 402272
Depends on: 402272
Whiteboard: [needs landing]
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.
Priority: P5 → P3
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
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Component: DOM: Mozilla Extensions → DOM
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.