-
Notifications
You must be signed in to change notification settings - Fork 26
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
Fixes #45, #46 - Fix a few consistency issues #47
Conversation
…ithm The tokens should already have that as part of its name, if relevant.
@@ -244,10 +244,6 @@ if <var>request</var>'s <a for=request>header list</a> | |||
policy-controlled feature</a>, returns <code>false</code>, then skip the next steps and | |||
continue to the next <var>hintName</var>. | |||
[[!PERMISSIONS-POLICY]] [[!CLIENT-HINTS]] | |||
|
|||
<li><p>Set <var>hintName</var> to "Sec-" concatenated with <var>hintName</var>. | |||
<div class=issue>We need to figure out if we really want a `Sec-` prefix, and if so also exempt it from CORS.</div> |
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.
Should we file an issue related to the CORS question, assuming it's still an open issue?
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.
There's an open PR on Fetch.
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.
Might be good to open an issue to track this on our end as well though
This removes the If so, I can do something slightly different and add some language to that effect and special case the set of "legacy" tokens (Save-Data, DRP, Viewport-Width, Width, Device-Memory, Downlink, RTT, ECT). PTAL @yoavweiss, 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
@@ -244,10 +244,6 @@ if <var>request</var>'s <a for=request>header list</a> | |||
policy-controlled feature</a>, returns <code>false</code>, then skip the next steps and | |||
continue to the next <var>hintName</var>. | |||
[[!PERMISSIONS-POLICY]] [[!CLIENT-HINTS]] | |||
|
|||
<li><p>Set <var>hintName</var> to "Sec-" concatenated with <var>hintName</var>. | |||
<div class=issue>We need to figure out if we really want a `Sec-` prefix, and if so also exempt it from CORS.</div> |
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.
Might be good to open an issue to track this on our end as well though
Thanks, @yoavweiss. I filed #49 so feel free to merge here when you have a chance. |
Fixes a few spec inconsistencies, related to UA-CH tokens and prefixes in the Request processing algorithm.