-
Notifications
You must be signed in to change notification settings - Fork 2k
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 Updates #24272
Storage Updates #24272
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.
I'm requesting changes, to apply all of the following suggested edits:
- inaccurate storage.local.getBytes & storage.sync.getBytes data (you swapped the two).
- Drop
extra_key
from managed API (2x) because it is not relevant. - Update chrome versions in
extra_key
to match the parent version.
After applying all of these suggested updates, please do a search and replace in this file, to replace ≤58
with ≤18
, since that is the version where these APIs existed; at least where the getBytesInUse method was introduced. Source:
webextensions/api/storage.json
Outdated
"description": "Supports empty key", | ||
"support": { | ||
"chrome": { | ||
"version_added": "≤105" |
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.
"version_added": "≤105" | |
"version_added": "≤58" |
Co-authored-by: Rob Wu <[email protected]>
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 sync compatibility information for Firefox for Android is incorrect. You have currently set it to 48 (true at the top level), but it should be "53". That mirrors desktop, but we cannot just use "mirror" here because there is a compatibility note that should be kept (i.e. that storage.sync does not sync on Android).
And please fix the Chrome compat information as requested at https://github.com/mdn/browser-compat-data/pull/24272#pullrequestreview-2332762107:>
After applying all of these suggested updates, please do a search and replace in this file, to replace
≤58
with≤18
, since that is the version where these APIs existed; at least where the getBytesInUse method was introduced. Source:
All the Firefox for Android 48 updates here seem to relate to this table: https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/storage/StorageArea#browser_compatibility Your review comment relates to https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/storage/sync#browser_compatibility This PR doesn't actually touch that line (line 445 currently, line 992 in this PR) |
This PR adds BCD entries to every storage namespace, whose version information may differ from where the original version was introduced. With such a large diff/file it is not obvious what would be going on, so I put a quick preview tool together that generates a table based on the input. You can see it at https://jsbin.com/subipeyibo/edit?output and input the latest commit, currently: The rendered table shows incorrect version information for the storage.sync API. |
To recap (plus more specific details):
|
@Rob--W, hopefully, those changes are made; however, I didn't fully follow your request regarding firefox_android in the sync namespace. We don't normally cascade notes down, but did you want the notes about the absence of sync and management of quota on all of the sub-items? Also, for sync.onChanged did you mean 101? |
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.
All looks good! And thanks for catching my typo, it should indeed have been 101 instead of 111.
Since this now includes the changes from #24525, it needs the "KR: Real BCD" label. |
Summary
This change:
"storage.session quota is not enforced" to cover the implementation of
storage.session.getBytesInUse
andstorage.session.QUOTA_BYTES
Related issues
Related content changes for Bug 1908925 or in mdn/content#35629