-
Notifications
You must be signed in to change notification settings - Fork 22.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
storage.session quota enforcement #35629
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bug did not only introduce enforcement of the quota, but also support for the storage.session.getBytesInUse
method and the storage.session.QUOTA_BYTES
constant.
Could you update the docs and BCD?
Co-authored-by: Rob Wu <[email protected]>
|
||
{{AddonSidebar}} | ||
|
||
The maximum amount of data (in bytes) that can be stored in session storage. The amount of data in session storage is the sum of the size of every value (as measured by the StructuredCloneHolder) plus the sum of every key's length. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The maximum amount of data (in bytes) that can be stored in session storage. The amount of data in session storage is the sum of the size of every value (as measured by the StructuredCloneHolder) plus the sum of every key's length. | |
The maximum amount of data (in bytes) that can be stored in session storage. |
StructuredCloneHolder is an internal inplementation detail. Let's drop it.
May make sense to refer to getBytesInUse to query hoe much quota is used.
|
||
{{Compat}} | ||
|
||
<!-- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's drop this whole section.
@@ -23,16 +24,14 @@ let gettingSpace = browser.storage.<storageType>.getBytesInUse( | |||
) | |||
``` | |||
|
|||
`<storageType>` can only be {{WebExtAPIRef("storage.sync")}}, not {{WebExtAPIRef("storage.local")}} because of [this bug](https://bugzil.la/1385832). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<storageType>
refers to the fragment in the code sample. Let's keep it but refer to all formats that are supported. For example, see https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/storage/StorageArea/remove
While looking at other docs I noticed that "writable" should be dropped from https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/storage/StorageArea/get . Let's drop it (managed storage is not writable).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved provided that the one comment in getBytesInUse gets addressed. Feel free to request another review.
`<storageType>` can only be {{WebExtAPIRef("storage.sync")}}, not {{WebExtAPIRef("storage.local")}} because of [this bug](https://bugzil.la/1385832). | ||
Where `<storageType>` is one of the storage types — {{WebExtAPIRef("storage.sync", "sync")}}, {{WebExtAPIRef("storage.local", "local")}}, {{WebExtAPIRef("storage.session", "session")}}, or {{WebExtAPIRef("storage.managed", "managed")}}. | ||
|
||
In Firefox, `<storageType>` can't be {{WebExtAPIRef("storage.local")}}, because of [bug 1385832](https://bugzil.la/1385832), and {{WebExtAPIRef("storage.managed")}} isn't supported. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current phrasing misleadingly claims that storage.managed is unsupported in Firefox. That is not the case. Another potential interpretation is that it is somehow related to the previous claim about storage.local.
To avoid confusion, consider dropping the mention of "managed" from getBytesInUse above.
Note that support is limited:
- Firefox does not offer a storage.managed.getBytesInUse method.
- Chrome does have a method, but its resolved value is always 0, as seen in this unit test: https://source.chromium.org/chromium/chromium/src/+/main:chrome/test/data/extensions/api_test/settings/managed_storage/background.js;drc=4a8573cb240df29b0e4d9820303538fb28e31d84
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not the best wording - followed your suggestion and added the info about Chrome to the BCD
This pull request has merge conflicts that must be resolved before it can be merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved with the minor issue addressed.
files/en-us/mozilla/add-ons/webextensions/api/storage/storagearea/getbytesinuse/index.md
Outdated
Show resolved
Hide resolved
This pull request has merge conflicts that must be resolved before it can be merged. |
Yes I approved and it is ready from the data perspective. There is a merge conflict now though... |
Description
Provides a release note for the enforcement of the session.storage quota in Firefox Nightly 131. Addresses the documentation requirements of Bug 1908925 storage.session quota is not enforced.