-
Notifications
You must be signed in to change notification settings - Fork 10
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
MediaDevices.produceCropTarget -> CropTarget.fromElement #50
Conversation
Change the point of exposure of token-minting from: MediaDevices.produceCropTarget() To: CropTarget.fromElement()
@jan-ivar has requested that Chrome should hold off on shipping Region Capture so that we may keep discussing. Both @jan-ivar and @youennf have been extremely responsive in interacting with the discussion on the various issues of this API in the last few weeks. I would be gratified if you could also be similarly responsive in reviewing PRs that solidify the consensus we have worked so hard to achieve. |
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 believe the sample code at https://w3c.github.io/mediacapture-region/#sample-code should be using CropTarget.fromElement()
instead of mediaDevices.produceCropTarget()
.
The README.md file would also benefit from these changes for consistency
@@ -151,7 +151,7 @@ <h3><dfn>CropTarget</dfn> Definition</h3> | |||
<pre class="idl"> |
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.
The text above should be edited as well. It currently says:
CropTarget is an intentionally empty, opaque identifier that exposes nothing. Its sole
purpose is to be handed to {{BrowserCaptureMediaStreamTrack/cropTo}} as input.
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.
That much seems still correct, given that the only method is static. We'd probably not have created this CropTarget as an exposure point for fromElement
if we went, say, with the UUID-based solution.
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.
The "exposes nothing" part was what triggered me as CropTarget.fromElement()
is something to me.
Your call 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.
I could remove "exposes nothing". Would that make things clear enough, or would "intentionally empty" be confusing?
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.
Removing exposes nothing
would work for me.
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.
Done.
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.
To increase visibility of this change to other reviewers: I've also removed "sole".
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.
LGTM
Co-authored-by: François Beaufort <[email protected]>
Thanks for catching!
I'll send a separate PR once this one is merged. I think it would require a bit more prose to be updated, so it might take a few more iterations, whereas here I hope we're mostly done. |
@@ -151,7 +151,7 @@ <h3><dfn>CropTarget</dfn> Definition</h3> | |||
<pre class="idl"> |
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.
LGTM
</dt> | ||
<dd> | ||
<p> | ||
Calling {{CropTarget/fromElement}} with an {{Element}} of a supported type associates |
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.
"supported type" here carries no definition, and the algorithm specifies no type validation, which means it is just confusing and should be removed.
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 is prose was moved unchanged. The status quo of the document has not been altered. Let's have a separate discussion for this.
Calling {{CropTarget/fromElement}} with an {{Element}} of a supported type associates | ||
that {{Element}} with a {{CropTarget}}. This {{CropTarget}} may be used as input to | ||
{{BrowserCaptureMediaStreamTrack/cropTo}}. We define a | ||
<dfn>valid CropTarget</dfn> as one returned by a previous call to |
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 doesn't belong here which I've stated in #29 (comment), but since this is in some ways a code-move, I'll accept a separate PR to remove it, in order to break down changes into smaller PRs.
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 is prose was moved unchanged. The status quo of the document has not been altered. Let's have a separate discussion for this.
Co-authored-by: Jan-Ivar Bruaroey <[email protected]>
Co-authored-by: Jan-Ivar Bruaroey <[email protected]>
The current PR would still be an improvement over
Let's press onwards. Many productive issues await us. |
Change the point of exposure of token-minting from:
MediaDevices.produceCropTarget()
To:
CropTarget.fromElement()
Everything else is kept as-is, to be debated and improved in subsequent PRs.
Preview | Diff