-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add or condition support. #7
Conversation
@domenic @sisidovski Can I ask you to review the change? |
@@ -1567,6 +1567,8 @@ spec: storage; urlPrefix: https://storage.spec.whatwg.org/ | |||
RequestMode requestMode; | |||
RequestDestination requestDestination; | |||
RunningStatus runningStatus; | |||
|
|||
sequence<RouterCondition> _or; |
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 unsure this has to have a leading underscore. https://webidl.spec.whatwg.org/#idl-names
This underscore in the chromium implementation actually comes from the chromium Web IDL compiler, but Web IDL spec itself doesn't think or
is a reserved keyword?
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 think it is reserved: https://webidl.spec.whatwg.org/#index-prod-UnionType has it in monospaced font, which I think means it's a terminal symbol and thus the tokenizer will pick it up as part of the grammar? I'm pretty rusty on my grammar stuff though.
But we have had to do similar escaping in the past, e.g. note the definition of _any
in https://dom.spec.whatwg.org/#interface-AbortSignal . So this makes sense to me.
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 couldn't find the whole list. That makes sense, thanks!
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 with nits
@@ -1567,6 +1567,8 @@ spec: storage; urlPrefix: https://storage.spec.whatwg.org/ | |||
RequestMode requestMode; | |||
RequestDestination requestDestination; | |||
RunningStatus runningStatus; | |||
|
|||
sequence<RouterCondition> _or; |
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 think it is reserved: https://webidl.spec.whatwg.org/#index-prod-UnionType has it in monospaced font, which I think means it's a terminal symbol and thus the tokenizer will pick it up as part of the grammar? I'm pretty rusty on my grammar stuff though.
But we have had to do similar escaping in the past, e.g. note the definition of _any
in https://dom.spec.whatwg.org/#interface-AbortSignal . So this makes sense to me.
- mutual to mutually. - |or condition| or |or conditions| to |orCondition| or |orConditions|. - simplify the for loop upon WICG/service-worker-static-routing-api#9.
Thanks for the review. I am going to merge this. |
The ServiceWorker static routing API has a or conditional syntax. This is a specification update to support that.
Preview | Diff