make cookie subscription name optional, url default to service worker… #291
Conversation
index.bs
Outdated
| each member is a <dfn>cookie change subscription</dfn>. A [=cookie change subscription=] is | ||
| <span dfn-for="cookie change subscription"> | ||
| a [=tuple=] of <dfn>name</dfn> and <dfn>url</dfn>. | ||
| a [=tuple=] of an optional <dfn>name</dfn> and a <dfn>url</dfn>. |
There was a problem hiding this comment.
This doesn't really exist. I think we need to make name be a string or null or some such.
There was a problem hiding this comment.
tried to add the types to that line
index.bs
Outdated
| 1. Let |subscription list| be |registration|'s associated [=cookie change subscription list=]. | ||
| 1. [=list/For each=] |entry| in |subscriptions|, run these steps: | ||
| 1. Let |name| be |entry|["{{CookieStoreGetOptions/name}}"]. | ||
| 1. Let |name| be the |entry|["{{CookieStoreGetOptions/name}}"] if it [=map/exists=]. |
There was a problem hiding this comment.
If it doesn't exist, what happens in the next step?
There was a problem hiding this comment.
made an if block and only attempt to normalize if name is provided
index.bs
Outdated
| 1. Let |url| be the result of [=basic URL parser|parsing=] |entry|["{{CookieStoreGetOptions/url}}"] with |settings|'s [=environment settings object/API base URL=]. | ||
| 1. Let |url| be |registration|'s [=service worker registration/scope url=]. | ||
| 1. If ["{{CookieStoreGetOptions/url}}"] [=map/exists=], then let |url| be the result of [=basic URL parser|parsing=] |entry|["{{CookieStoreGetOptions/url}}"] with |settings|'s [=environment settings object/API base URL=]. | ||
| 1. If |url| does not start with |registration|'s [=service worker registration/scope url=], |
There was a problem hiding this comment.
added "if |url| is failure, or ..." to the promise rejection in the next step but I can split if thats too long
|
FYI added some additional testing for a missing name and empty subscription web-platform-tests/wpt#56004 |
DCtheTall
left a comment
DCtheTall
left a comment
There was a problem hiding this comment.
LGTM, though I would wait until Anne also approves since he also left a review :)
index.bs
Outdated
| 1. If |url| does not start with |registration|'s [=service worker registration/scope url=], | ||
| then [=reject=] |p| with a {{TypeError}} and abort these steps. | ||
| 1. Let |name| be null. | ||
| 1. If ["{{CookieStoreGetOptions/name}}"] [=map/exists=]: |
There was a problem hiding this comment.
You omitted |entry| here and in several cases below (also with different keys).
index.bs
Outdated
| 1. [=Normalize=] |name|. | ||
| 1. Let |url| be |registration|'s [=service worker registration/scope url=]. | ||
| 1. If ["{{CookieStoreGetOptions/url}}"] [=map/exists=], then set |url| to the result of [=basic URL parser|parsing=] |entry|["{{CookieStoreGetOptions/url}}"] with |settings|'s [=environment settings object/API base URL=]. | ||
| 1. If |url| is failure, or if |url| does not start with |registration|'s [=service worker registration/scope url=], then [=reject=] |p| with a {{TypeError}} and abort these steps. |
There was a problem hiding this comment.
No need for the comma before "or" since we only have two conditions.
There was a problem hiding this comment.
gotcha, removed it
annevk
left a comment
annevk
left a comment
There was a problem hiding this comment.
We should also consider linking "start with" to https://infra.spec.whatwg.org/#string-starts-with
| then [=iteration/continue=]. | ||
| 1. Let |cookieName| be the result of running [=UTF-8 decode without BOM=] on |cookie|'s [=cookie/name=]. | ||
| 1. If |cookieName| equals |subscription|'s [=cookie change subscription/name=], | ||
| 1. If |subscription|'s [=cookie change subscription/name=] is null, or if |cookieName| equals |subscription|'s [=cookie change subscription/name=], |
There was a problem hiding this comment.
| 1. If |subscription|'s [=cookie change subscription/name=] is null, or if |cookieName| equals |subscription|'s [=cookie change subscription/name=], | |
| 1. If |subscription|'s [=cookie change subscription/name=] is null or |cookieName| is |subscription|'s [=cookie change subscription/name=], |
| 1. [=Normalize=] |name|. | ||
| 1. Let |url| be |registration|'s [=service worker registration/scope url=]. | ||
| 1. If |entry|["{{CookieStoreGetOptions/url}}"] [=map/exists=], then set |url| to the result of [=basic URL parser|parsing=] |entry|["{{CookieStoreGetOptions/url}}"] with |settings|'s [=environment settings object/API base URL=]. | ||
| 1. If |url| is failure or if |url| does not start with |registration|'s [=service worker registration/scope url=], then [=reject=] |p| with a {{TypeError}} and abort these steps. |
There was a problem hiding this comment.
| 1. If |url| is failure or if |url| does not start with |registration|'s [=service worker registration/scope url=], then [=reject=] |p| with a {{TypeError}} and abort these steps. | |
| 1. If |url| is failure or |url| does not start with |registration|'s [=service worker registration/scope url=], then [=reject=] |p| with a {{TypeError}} and abort these steps. |
|
Note that before landing you'll need to adjust the commit message a bit to meet the guidelines. |
3 participants
… scope
To fix #284 and #236 make the name optional and match to every available cookie in the service worker scope if it is not provided as an argument. The url of the subscription should default to be the service worker's scope url if omitted
Will add some WPTs beyond the missing/invalid url tests in https://github.com/web-platform-tests/wpt/blob/master/cookiestore/cookieStore_subscribe_arguments.https.any.js
Preview | Diff