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

SourceBuffer(List) / MediaSource: update event entries #13924

Merged
merged 11 commits into from
Feb 21, 2022

Conversation

Elchi3
Copy link
Member

@Elchi3 Elchi3 commented Dec 9, 2021

Update for new event structure.

Content update: mdn/content#11176

@Elchi3 Elchi3 added the needs content update 📝 This PR needs a corresponding update to mdn/content to update the documentation label Dec 9, 2021
@github-actions github-actions bot added the data:api 🐇 Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API label Dec 9, 2021
@Elchi3 Elchi3 removed the needs content update 📝 This PR needs a corresponding update to mdn/content to update the documentation label Dec 13, 2021
@Elchi3
Copy link
Member Author

Elchi3 commented Dec 13, 2021

Content PR merged. This is ready.

],
"support": {
"chrome": {
"version_added": "53"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Chrome 53 is when onabort and lots of other event handler properties were added:
https://storage.googleapis.com/chromium-find-releases-static/000.html#00003ed19f12e054784ee59b95c863b380361ef4

I would suggest also including MediaSource in this PR, to sort it all out at once.

Unfortunately, we'll have to find the versions when each event was supported.

It also looks like it's the same situation for Edge. Firefox and Safari match the parent feature version though, so those don't need any research.

@foolip
Copy link
Collaborator

foolip commented Jan 5, 2022

@Elchi3 it will be a lot of work to sort this out, and really any time that the event's version doesn't match the parent feature I'd be suspicious and want to check if it's yet another case of the event handler property being missing originally.

@foolip
Copy link
Collaborator

foolip commented Jan 5, 2022

@Elchi3 so my question is, how do you want to handle this? Doing the research as part of PR review is going to slow things down, but can you maintain a list of likely incorrect data in an issue for later review?

@Elchi3 Elchi3 changed the title SourceBuffer(List): update event entries SourceBuffer(List) / MediaSource: update event entries Feb 8, 2022
@Elchi3
Copy link
Member Author

Elchi3 commented Feb 8, 2022

I've added MediaSource, so it is all together in this PR now.

For all events, the question is:

Is it Chrome 53 or Chrome 31 (Chrome 25 with webkit)?
Is it Edge 17 or Edge 12?

SourceBuffer.abort_event
SourceBuffer.error_event
SourceBuffer.update_event
SourceBuffer.updateend_event
SourceBuffer.updatestart_event
SourceBufferList.addsourcebuffer_event
SourceBufferList.removesourcebuffer_event
MediaSource.sourceclose_event
MediaSource.sourceend_event
MediaSource.sourceopen_event

It currently states the later versions.

@foolip
Copy link
Collaborator

foolip commented Feb 9, 2022

@Elchi3 the event handler properties were added in Chromium 53:
https://storage.googleapis.com/chromium-find-releases-static/000.html#00003ed19f12e054784ee59b95c863b380361ef4

So we'll need partial_implementation ranges for the events before that.

The events themselves look complicated. Some of the events themselves were originally prefixed, with https://trac.webkit.org/changeset/123522/webkit being an example of this. (But note how no code was added to actually fire "webkitaddsourcebuffer" or "webkitremovesourcebuffer" there.)

@Elchi3 would you like to capture the prefixed events so that we have data going back to when the interfaces were added? This will require digging in WebKit source for each event. I can show you how I do it if you'd like.

@Elchi3
Copy link
Member Author

Elchi3 commented Feb 10, 2022

I'm happy to add ranges like these:

"chrome": [
  {
    "version_added": "53"
  },
  {
    "version_added": "$versionnumber",
    "partial_implementation": true,
    "notes": "The <code>onsourceclose</code> event handler property is not supported."
 }
],

I have no idea how to find $versionnumber. Pointers appreciated.

The events themselves look complicated. Some of the events themselves were originally prefixed,

I don't know how to find the complete history here and I'm not sure it is worth to go back in time so much?

@Elchi3 Elchi3 requested a review from foolip February 15, 2022 14:30
@foolip
Copy link
Collaborator

foolip commented Feb 17, 2022

OK, so let's ignore the prefixed events and try to get reasonable versions for when the unprefixed events were supported.

api/MediaSource.json Outdated Show resolved Hide resolved
api/MediaSource.json Outdated Show resolved Hide resolved
@Elchi3 Elchi3 requested a review from foolip February 18, 2022 12:29
api/MediaSource.json Outdated Show resolved Hide resolved
api/MediaSource.json Outdated Show resolved Hide resolved
api/MediaSource.json Outdated Show resolved Hide resolved
api/MediaSource.json Outdated Show resolved Hide resolved
api/MediaSource.json Show resolved Hide resolved
Copy link
Collaborator

@foolip foolip left a comment

Choose a reason for hiding this comment

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

Found some more things wrong when trying to do "final" review.

Note that the IE data is also arguably wrong, I'm pretty sure it was also missing the event handler properties. I won't insist on adding that though.

api/SourceBufferList.json Outdated Show resolved Hide resolved
api/SourceBufferList.json Outdated Show resolved Hide resolved
api/SourceBufferList.json Outdated Show resolved Hide resolved
api/MediaSource.json Outdated Show resolved Hide resolved
Copy link
Collaborator

@foolip foolip left a comment

Choose a reason for hiding this comment

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

I started to review again and found more things wrong, but at this point I'm just going to look away :)

@foolip foolip merged commit c0ba7f0 into mdn:main Feb 21, 2022
@Elchi3
Copy link
Member Author

Elchi3 commented Feb 21, 2022

Ha, thanks for merging! This has been quite an advantage in compat archeology.

@Elchi3 Elchi3 deleted the events-sourcebuffer branch February 21, 2022 16:20
@foolip
Copy link
Collaborator

foolip commented Feb 21, 2022

For future archeologists: we still have a mix of 41 and 42 versions for Firefox Android, which is almost certainty wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data:api 🐇 Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants