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

Hyperlink Auditing: Add Content-Type 'text/ping' #7481

Merged
merged 1 commit into from
Feb 17, 2022

Conversation

fowl2
Copy link
Contributor

@fowl2 fowl2 commented Jan 9, 2022

fixes #7477


/links.html ( diff )

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

This looks pretty good to me. Technically to be fully rigorous we'd want to assemble the full header list (including Ping-To / Ping-From) before this line, but the less-precise way in which they're currently specified is a preexisting problem, and I don't think this change makes that issue worse.

I'll merge tomorrow unless anyone else has any comments before then.

@Kaiido
Copy link
Member

Kaiido commented Jan 11, 2022

Minor nits, but shouldn't <code>text/ping</code> be wrapped in back-ticks too?
Also this doesn't seem to respect the 100 chars line wrapping.

@domenic
Copy link
Member

domenic commented Jan 11, 2022

@Kaiido again, I'd ask that you leave review to the editors :). We're happy to provide such fixups to first-time contributors ourselves before merging, as was my plan here.

@Kaiido
Copy link
Member

Kaiido commented Jan 11, 2022

Alright, just didn't want to let this missed and took the "anyone" to mean it was fine, just like in the other PR I took the "cc @ whatwg/canvas" to mean "please help review".

@domenic
Copy link
Member

domenic commented Jan 11, 2022

Hmm, I realized that we actually don't have any tests for this in the web platform tests repository. @fowl2, would you be up for writing them?

@domenic domenic added needs tests Moving the issue forward requires someone to write tests topic: hyperlink labels Jan 11, 2022
@annevk
Copy link
Member

annevk commented Jan 12, 2022

FWIW, I think it was fine that @Kaiido commented here as it wasn't clear the wrapping and backticks were noticed.

@fowl2
Copy link
Contributor Author

fowl2 commented Jan 13, 2022

I copied the formatting of the content type literal from somewhere else in the document, so if you're fixing it here it might want to be fixed there too :)

I've never done a web platform test before but sounds fun, I'll see if I have some time to give it a go on the weekend.

domenic added a commit to web-platform-tests/wpt that referenced this pull request Feb 17, 2022
@domenic
Copy link
Member

domenic commented Feb 17, 2022

I worked on some tests in web-platform-tests/wpt#32887 which should unblock landing this.

@domenic domenic removed the needs tests Moving the issue forward requires someone to write tests label Feb 17, 2022
@domenic domenic merged commit cf367a9 into whatwg:main Feb 17, 2022
domenic added a commit to web-platform-tests/wpt that referenced this pull request Feb 18, 2022
DanielRyanSmith pushed a commit to DanielRyanSmith/wpt that referenced this pull request Feb 28, 2022
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Mar 22, 2022
Automatic update from web-platform-tests
Test hyperlink auditing headers

For whatwg/html#7481.

--

wpt-commits: fd7bc357005c4f85fb73872fd841086c98f75cb3
wpt-pr: 32887
aosmond pushed a commit to aosmond/gecko that referenced this pull request Mar 26, 2022
Automatic update from web-platform-tests
Test hyperlink auditing headers

For whatwg/html#7481.

--

wpt-commits: fd7bc357005c4f85fb73872fd841086c98f75cb3
wpt-pr: 32887
@fowl2 fowl2 deleted the ping-content-type branch June 14, 2022 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Hyperlink Auditing (<a ping>) doesn't specify Content-Type to use for pings
4 participants