-
Notifications
You must be signed in to change notification settings - Fork 22.4k
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
making fixes for wolenetz review comments #22341
Conversation
Preview URLs
Flaws (7)Note! 3 documents with no flaws that don't need to be listed. 🎉 URL:
URL:
(this comment was updated 2022-11-18 07:38:00.143534) |
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.
Looks pretty good. Thank you for addressing the feedback. While the number of nits I have seems large on this PR, they are mostly duplicates due to the example usage text is duplicated so much (aside: is there a way to inline a shared example set of text, or is this copy/paste of example usage text/script unfortunately required?)
Note, I can only "comment", not "request changes" in the review dialog, but please do address these nits. Thanks again for your work!
Thanks @wolenetz ! I think I fixed 'em all.
It would be nice to have a robust include/component type system to do this, but unfortunately, MDN doesn't really have one right now. I think there is still a macro in existence that allows you to include a section from one page on another, but IIRC it is somewhat fragile. |
@@ -34,7 +34,7 @@ The **`MediaSource`** interface of the {{domxref("Media Source Extensions API", | |||
- {{domxref("MediaSource.duration")}} | |||
- : Gets and sets the duration of the current media being presented. | |||
- {{domxref("MediaSource.handle")}} {{ReadOnlyInline}} {{Experimental_Inline}} | |||
- : Returns a {{domxref("MediaSourceHandle")}} object, a proxy for the `MediaSource` that can be transferred from a dedicated worker back to the main thread and attached to a media element via its {{domxref("HTMLMediaElement.srcObject")}} property. | |||
- : Inside a dedicated worker, returns a {{domxref("MediaSourceHandle")}} object, a proxy for the `MediaSource` that can be transferred from the worker back to the main thread and attached to a media element via its {{domxref("HTMLMediaElement.srcObject")}} property. |
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 should have mentioned this previously - my apologies. It might be a good idea to also mention that this handle getter will always return the same MediaSourceHandle instance specific to the dedicated worker MediaSource instance. In particular, if the handle has already been transferred to the main thread using postMessage, the handle instance in the worker is technically detached and no longer can be transferred again. --- or is this too much detail for MDN? It is definitely in the MSE spec, in the Chrome implementation as of 108, and also in the interop web-platform-test suite for MSE.
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.
This isn't too much detail — this is a really useful bit of information to include. I've included a note to explain this on both the MediaSourceHandle
and MediaSource.handle
pages.
Ack. |
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.
Looks good to me (the comment I made today about the specifics around the [SameObject] semantic can be addressed in a later PR or not at all if you deem that to be too much detail for the handle getter's MDN documentation.)
* making fixes for wolenetz review comments * respond to latest wolenetz comments * add note about mediasourcehandle instances and transfers
Description
This PR makes fixes for the review comments by @wolenetz included in #22327.
Specifically, I have:
srcObject
page to fix the note at the top, making it accurate for the current situationhandle
is only accessible from dedicated workers.Motivation
Additional details
Related issues and pull requests
Fixes #22327