Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 reporting and report-only mode. #529
Add reporting and report-only mode. #529
Changes from 3 commits
5cf4f9c
1db7935
30a6371
d99fb6f
63c4a10
e51cd15
62d9823
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Lowercase
T
for "true". Same goes for "False" in other instances below. See https://infra.spec.whatwg.org/#booleans. If the spec does this elsewhere, we can maybe take care of this in a separate PR (or not at all, if you prefer, it isn't a big deal really, just a nit).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.
Will address in a separate cleanup as well. 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.
Sometimes this spec uses
"report-to"
(code formatting) and other times (like this case) it doesn't. I'd aim to be consistent (probably using code formatting everywhere).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.
Can you link to "permissions policy" here?
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.
One problem here is that we always pass in
group
, its just sometimes it is the actual value null. In that case, I think we should either: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'll make it mandatory, and react to null; in earlier iterations of reporting, there was no way to configure the group, and so it was effectively always "default" (we actually never called it from this spec, the intention at the time was to call this from other specs instead, and maybe they would choose a different endpoint group).