-
Notifications
You must be signed in to change notification settings - Fork 2k
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 missing urls for Trusted* (and toJSON() entries) #11063
Conversation
For the record, these, including the |
@@ -46,8 +48,60 @@ | |||
"deprecated": false | |||
} | |||
}, | |||
"toJSON": { | |||
"__compat": { | |||
"mdn_url": "https://developer.mozilla.org/docs/Web/API/TrustedHTML/toJSON", |
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.
Wow, https://developer.mozilla.org/docs/Web/API/TrustedHTML/toJSON actually exists :)
Commented on something at mdn/content#2978 (review)...
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.
We have a few toJSON and toString (stringifier) pages. Like for the iterable pages, they all have a close structure that make them fairly quick to create.
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.
IMHO, for them to be useful, they would have to say which attributes are actually serialized in the resulting JSON, since that's often defined by prose in specs and isn't really guessable from looking at https://developer.mozilla.org/en-US/docs/Web/API/TrustedHTML, for example.
api/TrustedHTML.json
Outdated
"support": { | ||
"chrome": { | ||
"version_added": false, | ||
"notes": "Currently Chrome Canary only" |
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.
Very few things are enabled in Chrome Canary only in the same way that Firefox does, unless perhaps we count all of the "experimental web platform features" flag in this bucket.
Anyway, how did you determine this is in Canary only? Judging by https://mdn-bcd-collector.appspot.com/tests/api/TrustedHTML/toJSON it's already shipped to stable in Chrome 90, and that happened in this commit:
https://storage.googleapis.com/chromium-find-releases-static/0db.html#0dbce1408181f002902cba22fa275f658e8d9c18
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 trusted @rachelandrew comment in mdn/content#297, and it could have changed in the meantime!
Should we put 90?
(Sidenote: I need to dig into mdn-bcd-collector :-) )
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.
Yeah, I'd use Chrome 90 and then npm run mirror
to update the rest.
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 tried to do it. My first run wiith npm run mirror
, so I hope I did well. Fix in next commit
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 didn't try running it myself, but npm run mirror edge
should have mirrored the Edge data, but you have to run it once for each file you care about so it's very tedious. #10645 will make it less tedious.
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 tried, but I think it mirrored it to false as it took IE as source. Maybe I did it wrong, though.
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.
Ah right, #6582 hasn't been merged yet :)
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.
Missing some Edge data, but I'll apply my own suggestions and merge.
Added the mdn_url and spec_url to
TrustedHTML
,TrustedScript
,TrustedScriptURL
,TrustedTypePolicy
, andTrustedTypePolicyFactor
(both on the interface and the children).Added the
TrustedHTML.toJSON()
,TrustedScript.toJSON()
, andTrustedScriptURL.toJSON()
compat data. They are only in Canary, so I put a note there.