Skip to content
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

[editorial] Make algorithm headers clickable #621

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

antosart
Copy link
Member

@antosart antosart commented Sep 6, 2023

Adding the dfn attribute to algorithm headers makes them clickable and makes it possible to find their references.

A couple algorithms where defined both in headers and in prose, and we just remove the in-prose-definition for consistency.

Fixes #618.

index.bs Outdated
parsed, the object's [=policy/directive set=] will be empty.
Given a [=string=] |serialized|, a [=policy/source=] |source|, and a [=policy/disposition=]
|disposition|, the following algorithm returns a parsed [=Content Security Policy object=].
If |serialized| could not be parsed, the object's [=policy/directive set=] will be empty.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd have expected the shift to go the other way, moving from treating the header as the definition to explicitly adding a <dfn> to each algorithm. That's what I was suggesting in #618 (comment).

That said, you're doing the work, and I'm fine with this approach if it works...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I misread your comment then. Putting all definitions in prose is a bit more work, since there are quite a few algorithms. It will also require fixing links from other specs. I can do it, it's just a bit more work.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's my preference, but I'll defer to you on whether it's worth doing the work.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I finally found the time to make the change. Unfortunately it ended up in a quite huge diff (CSP defines a lot of algorithms!)

Let me know what you think when you have the time to have a look.

index.bs Outdated
@@ -467,16 +467,13 @@ spec: WebRTC; urlPrefix: https://www.w3.org/TR/webrtc/
; in section 2.1 of this document.
</pre>

<h4 id="parse-serialized-policy" algorithm>
<h4 id="parse-serialized-policy" algorithm dfn>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes the type from abstract-op to a dfn. I'm a little surprised that didn't break any links.

@antosart
Copy link
Member Author

antosart commented Oct 5, 2023

Given the discussion on whatwg/fetch#1714 I made two additional changes. I replaced "should be" with "is" in the various algorithms and I made the exported algorithms return booleans instead of "Allowed"/"Blocked". The changes are in separate commits for easier reviews (I can also split them in different PRs).

Copy link
Member

@mikewest mikewest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very large diff indeed. :)

I very quickly skimmed it, and the kinds of changes you're making look right. I'll go over it again later this morning to see if anything jumps out at me as looking weird.

@oasissbasinessltd
Copy link

oasissbasinessltd commented May 18, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Algorithms should be <dfn> in prose instead of linked to headers
3 participants