make cookie subscription name optional, url default to service worker…#291
make cookie subscription name optional, url default to service worker…#291annevk merged 7 commits intowhatwg:mainfrom
Conversation
| 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
| 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
| 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 :)
| 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).
| 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.
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
|
Note that before landing you'll need to adjust the commit message a bit to meet the guidelines. |
Co-authored-by: Anne van Kesteren <annevk@annevk.nl>
DCtheTall
left a comment
DCtheTall
left a comment
There was a problem hiding this comment.
Still LGTM, one formatting nit
|
@aamuley does MDN document this already? If so, we should file an issue against MDN too. |
|
ok just did that, mdn/content#43382 |
3 participants
… scope
To fix mdn/mdn#284 and mdn/mdn#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
(See WHATWG Working Mode: Changes for more details.)
Preview | Diff