-
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
DO NOT MERGE: Technical review for Shared Storage API docs #28051
Conversation
Preview URLs (30 pages)
Flaws (81)Note! 1 document with no flaws that don't need to be listed. 🎉 URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
URL:
External URLs (18)URL:
URL:
URL:
URL:
URL:
URL:
URL:
(comment last updated: 2023-11-21 09:00:41) |
Co-authored-by: sideshowbarker <mike@w3.org>
Co-authored-by: sideshowbarker <mike@w3.org>
Co-authored-by: sideshowbarker <mike@w3.org>
Co-authored-by: sideshowbarker <mike@w3.org>
…nt into shared-storage-api
- : Thrown if: | ||
- The worklet module has not yet been added with {{domxref("Worklet.addModule", "addModule()")}}. | ||
- `key` exceeds the browser-defined maximum length. | ||
- A key/value with the specified `key` is not found in the shared storage. |
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.
If the key is not found, the operation doesn't throw/reject. Instead, it resolves to undefined
.
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.
OK, I've removed that sentence, and updated the return value description to:
A {{jsxref("Promise")}} that fulfills with a string equal to the value of the retrieved key/value pair, or
undefined
if the specifiedkey
is not found in the shared storage.
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.
Does this one throw if shared storage is disabled, like set()
, append()
, etc.?
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.
Yes.
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 of the methods do.)
files/en-us/web/api/workletsharedstorage/remainingbudget/index.md
Outdated
Show resolved
Hide resolved
{{APIRef("Shared Storage API")}}{{SeeCompatTable}} | ||
|
||
The **`remainingBudget()`** method of the | ||
{{domxref("WorkletSharedStorage")}} interface returns the remaining navigation budget for the current origin. |
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.
See comment.
The **`remainingBudget()`** method of the | ||
{{domxref("WorkletSharedStorage")}} interface returns the remaining navigation budget for the current origin. | ||
|
||
The navigation budget is the number of bits of entropy permitted inside a {{htmlelement("fencedframe")}} as a result of {{domxref("WindowSharedStorage.selectURL()")}} calls, per origin, per 24-hour time period. This is not the same as the number of navigations; rather it is based on the number of potential navigations in each call. Each time a `selectURL()` navigation occurs, an amount equal to logarithm base 2 of the number of URL choices is deducted from the corresponding origin's budget. |
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.
Ditto. Soon origin --> site.
Apologies, I had forgotten about this. Took another look and left some more comments. Looks good apart from these. |
@pythagoraskitty thanks so much for having another look; I really appreciate your time. I've made fixes for all the 2nd round review comments; there are just a couple more queries for you/bits you might want to check. |
This pull request has merge conflicts that must be resolved before it can be merged. |
- The `Promise` rejects with a {{jsxref("TypeError")}} if `key` exceeds the browser-defined maximum length. | ||
|
||
> **Note:** If the key/value pair doesn't exist in the shared storage, no error is thrown — the operation still fulfills with `undefined`. | ||
|
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.
Did you want to put another note here like to match the notes in the three other setter/deleter methods, e.g.
> **Note:** In the case of {{domxref("WindowSharedStorage")}}, if the `delete()` operation doesn't successfully write to the database for a reason other than shared storage not being available, no error is thrown — the operation still fulfills with `undefined`.
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.
@pythagoraskitty Yes, good idea. I have done so.
Can you let me know if there are any other changes you think this PR needs? If not, give me an "LGTM", and I will consider it signed off on technical review, after which I can get someone on the MDN content team to give it a final editorial review.
Thanks!
@pythagoraskitty Hi there! I just added enrollment information to the PR, as that was missing before. Have I presented an accurate picture of how the API fails to work if the calling site is not enrolled as appropriate? (The actual enrollment page is not part of the PR; I added it to the Topics API, and it should be merged by the time this PR is merged; see https://pr30107.content.dev.mdn.mozit.cloud/en-US/docs/Web/Privacy/Privacy_sandbox/Enrollment for a preview) |
- The {{domxref("WorkletSharedStorage.remainingBudget()")}} method | ||
- : The promise returned by `remainingBudget()` rejects with a `TypeError` {{domxref("DOMException")}}. | ||
- The {{domxref("WindowSharedStorage.run()")}} and {{domxref("WindowSharedStorage.selectURL()")}} methods | ||
- : Promises returned by run and URL selection operations will reject with a `TypeError` {{domxref("DOMException")}}. |
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.
Actually, all of the shared storage methods required enrollment, not just the ones listed here, but it's a little buried in the spec.
If you look at the references to https://wicg.github.io/shared-storage/#determine-whether-shared-storage-is-allowed, you will see among them https://wicg.github.io/shared-storage/#ref-for-determine-whether-shared-storage-is-allowed%E2%91%A0 in https://wicg.github.io/shared-storage/#obtain-a-shared-storage-shelf.
The latter is referenced at https://wicg.github.io/shared-storage/#ref-for-obtain-a-shared-storage-shelf in https://wicg.github.io/shared-storage/#obtain-a-shared-storage-bottle-map, which in turn is referenced in all of the methods except those you've already listed here.
(Since the methods you've listed here don't need a bottle map, they reference the https://wicg.github.io/shared-storage/#determine-whether-shared-storage-is-allowed directly.)
So perhaps instead of listed the methods individually here, say that all shared storage sub-features will fail? Or else list them all rejecting with a TypeError if you prefer?
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.
Also note that https://wicg.github.io/shared-storage/#check-if-addmodule-is-allowed-and-update-status references https://wicg.github.io/shared-storage/#determine-whether-shared-storage-is-allowed as well, so addModule()
also requires enrollment.
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.
Ah, thanks for helping me understand this. I have updated the comment on the API landing page and added exception information to all API methods, as advised.
Let me know if this PR needs anything else, or give me an "LGTM" if it looks OK. Thanks!
@@ -43,6 +43,7 @@ A {{jsxref("Promise")}} that fulfills with `undefined`. | |||
- : Thrown if: | |||
- The worklet module has not yet been added with {{domxref("Worklet.addModule", "addModule()")}}. | |||
- Shared storage is disabled (for example via a browser setting). | |||
- The calling site does not have the Shared Storage API included in a successful [privacy sandbox enrollment process](/en-US/docs/Web/Privacy/Privacy_sandbox/Enrollment). |
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.
Perhaps add this same item to all methods?
Will look good if you address my comments above. Thanks for adding this! |
@@ -37,6 +37,7 @@ None (`undefined`). | |||
- An operation has already been registered with the specified name. | |||
- The `operationCtor` is not a valid constructor. | |||
- The class does not contain a valid `run()` method. | |||
- The calling site does not have the Shared Storage API included in a successful [privacy sandbox enrollment process](/en-US/docs/Web/Privacy/Privacy_sandbox/Enrollment). |
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.
We actually don't check for register()
, which won't get to run in any case if addModule()
is rejected due to lack of enrollment. So you can leave this one out.
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.
OK, removed.
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 mod the comment I made for register()
.
Great, thank you @pythagoraskitty! The next stage is to close this technical review PR, and open up a new PR based on the same branch to contain the editorial review. |
Note: This technical review is now completed and approved. For the follow-on editorial review, see #30427
Description
The Shared Storage API is an integral part of Google's privacy sandbox technologies. Many parts of this set are being made available by default in Chrome 115 (depending on a gradual ramp-up to 100% of userbase over the 115 release period).
This PR adds content for Shared Storage. See my research document for information on exactly what features are being added.
Motivation
Additional details
Related issues and pull requests