Update spec for enrollment #105
Conversation
Update spec for enrollment
spec.bs
Outdated
| @@ -928,6 +929,12 @@ On the other hand, methods for getting data from the [=shared storage database=] | |||
| 1. If |worklet|'s [=global scopes|list of global scopes=] is [=list/empty=], then return a [=promise rejected=] with a {{TypeError}}. | |||
| 1. [=Assert=] that |worklet|'s [=global scopes|list of global scopes=] [=list/contains=] a single {{SharedStorageWorkletGlobalScope}}. | |||
| 1. If the result of running [=SharedStorageWorkletGlobalScope/check whether addModule is finished=] for |worklet|'s {{SharedStorageWorkletGlobalScope}} is false, return a [=promise rejected=] with a {{TypeError}}. | |||
| 1. Optionally, return a [=promise rejected=] with a {{TypeError}}. | |||
There was a problem hiding this comment.
@xyaoinum @pythagoraskitty I wasn't sure if this is the right place for returning an error from selectUrl and the right error type, can you help confirm if this aligns with the implementation or should this return be at an earlier point like in addModule, thanks!
There was a problem hiding this comment.
There seems to be two issues:
- In this spec, we only check the user settings during storage getters/setters, but not for addModule()/selectURL()/run(). We should check the user settings for those as well.
- In the spec, the Error is always "TypeError". In the code, the error type is sometimes OperationError.
I can send a patch to fix (1) to unblock this patch (or @shivanigithub do you prefer doing the fix along with this patch?). And problem 2 can be addressed separately as it's not only relevant to user setting errors.
spec.bs
Outdated
| @@ -376,7 +376,8 @@ The Shared Storage API will integrate into the [=Storage Model|Storage API=] as | |||
|
|
|||
| 1. Let |origin| be |environment|'s [=url/origin=]. | |||
| 1. If |origin| is an [=opaque origin=], then return failure. | |||
| 1. If the user has disabled [=shared storage=], then return failure. | |||
| 1. If the user has disabled [=shared storage=] or operation is denied due to other user agent defined mechanism e.g. |origin|'s site not being | |||
There was a problem hiding this comment.
More precisely, I think this should be |origin|'s [=origin/host=]'s [=host/registrable domain=]?
There was a problem hiding this comment.
Since you add the link to multiple sites, it seems better to add the link to <pre class="anchors"> section at the top, and reference it via [=enrolled=] in the main text.
html; urlPrefix: https://github.com/privacysandbox/attestation
type: dfn
text: enrolled
Also, I just noticed it's the schemeful site. So, this can be e.g. the result of [=obtaining a site=] with |origin| not being [=enrolled=].
There was a problem hiding this comment.
(FYI I also made a PR to add the settings check for addModule/selectURL/run: #106)
There was a problem hiding this comment.
Edited to add this link.
There was a problem hiding this comment.
And used something similar to the result of [=obtaining a site=] with |origin| not being [=enrolled=]...
Could probably still be further edited. xyaoinum@ what do you think?
spec.bs
Outdated
| @@ -928,6 +929,12 @@ On the other hand, methods for getting data from the [=shared storage database=] | |||
| 1. If |worklet|'s [=global scopes|list of global scopes=] is [=list/empty=], then return a [=promise rejected=] with a {{TypeError}}. | |||
| 1. [=Assert=] that |worklet|'s [=global scopes|list of global scopes=] [=list/contains=] a single {{SharedStorageWorkletGlobalScope}}. | |||
| 1. If the result of running [=SharedStorageWorkletGlobalScope/check whether addModule is finished=] for |worklet|'s {{SharedStorageWorkletGlobalScope}} is false, return a [=promise rejected=] with a {{TypeError}}. | |||
| 1. Optionally, return a [=promise rejected=] with a {{TypeError}}. | |||
There was a problem hiding this comment.
There seems to be two issues:
- In this spec, we only check the user settings during storage getters/setters, but not for addModule()/selectURL()/run(). We should check the user settings for those as well.
- In the spec, the Error is always "TypeError". In the code, the error type is sometimes OperationError.
I can send a patch to fix (1) to unblock this patch (or @shivanigithub do you prefer doing the fix along with this patch?). And problem 2 can be addressed separately as it's not only relevant to user setting errors.
|
Couldn't figure out how to reply inline to your comment above, @xyaoinum, i.e.
Anyway, I just approved and merged your fix to (1.) As for (2.), that's due to feedback from my spec mentor, @wanderview, who had recommended that we not delve too deeply into categorizing various exceptions and just use TypeErrors. But I'm happy to revisit that. |
Maybe the solution is to fix the code to throw TypeError to be in sync with the spec? |
Update to streamline
Combine 2 lines
tweaks
Remove transition paragraph
SHA: 0d7bffe Reason: push, by pythagoraskitty Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Update spec for enrollment
Preview | Diff