-
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
Changes from all commits
cc769c8
cee2d96
4440087
cb1d78e
3e0b086
71f998e
27359e5
d5628df
e2ccdd8
ffce3e3
82c3ec7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -145,13 +145,13 @@ <h3>CropTarget Motivation</h3> | |
<section id="crop-target"> | ||
<h3><dfn>CropTarget</dfn> Definition</h3> | ||
<p> | ||
CropTarget is an intentionally empty, opaque identifier that exposes nothing. Its sole | ||
purpose is to be handed to {{BrowserCaptureMediaStreamTrack/cropTo}} as input. | ||
CropTarget is an intentionally empty, opaque identifier. Its purpose is to be handed to | ||
{{BrowserCaptureMediaStreamTrack/cropTo}} as input. | ||
</p> | ||
<pre class="idl"> | ||
[Exposed=(Window,Worker), Serializable] | ||
interface CropTarget { | ||
// Intentionally empty; just an opaque identifier. | ||
[SecureContext] static Promise<CropTarget> fromElement(Element element); | ||
eladalon1983 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}; | ||
</pre> | ||
<div class="note"> | ||
|
@@ -160,7 +160,53 @@ <h3><dfn>CropTarget</dfn> Definition</h3> | |
<a href="https://github.com/w3c/mediacapture-region/issues/18">issue #18</a>. | ||
</p> | ||
</div> | ||
<dl data-link-for="CropTarget" data-dfn-for="CropTarget"></dl> | ||
<div class="note"> | ||
<p> | ||
There is no consensus yet on whether {{CropTarget/fromElement}} should be exposed beyond | ||
secure contexts. | ||
</p> | ||
</div> | ||
<dl data-link-for="CropTarget" data-dfn-for="CropTarget"> | ||
<dt> | ||
<dfn>fromElement()</dfn> | ||
</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 commentThe 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 commentThe 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. |
||
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 commentThe 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 commentThe 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. |
||
{{CropTarget.fromElement()}} in the current [=top-level browsing context=] or any of | ||
its | ||
<a data-cite="HTML#list-of-the-descendant-browsing-contexts" | ||
>descendant browsing contexts</a | ||
>. | ||
</p> | ||
<p> | ||
When {{CropTarget/fromElement}} is called with a given <var>element</var>, the user | ||
agent [=create a CropTarget|creates a CropTarget=] with <var>element</var> as input. | ||
The user agent MUST return a {{Promise}} <var>p</var>. The user agent MUST resolve | ||
<var>p</var> only after it has finished all the necessary internal propagation of | ||
state associated with the new {{CropTarget}}, at which point the user agent MUST be | ||
ready to receive the new {{CropTarget}} as a valid parameter to | ||
{{BrowserCaptureMediaStreamTrack/cropTo}}. | ||
</p> | ||
<p> | ||
When cloning an {{Element}} on which {{CropTarget/fromElement}} was previously called, | ||
the clone is not associated with any {{CropTarget}}. If {{CropTarget/fromElement}} is | ||
later called on the clone, a new {{CropTarget}} will be assigned to it. | ||
</p> | ||
<div class="note"> | ||
<p> | ||
There is no consensus yet on whether producing a {{CropTarget}} | ||
should be done by invoking an asynchronous method like {{CropTarget.fromElement()}}, | ||
or a {{CropTarget}} constructor that accepts an {{Element}} as input. This is | ||
further discussed on | ||
<a href="https://github.com/w3c/mediacapture-region/issues/17">issue #17</a>. | ||
</p> | ||
</div> | ||
</dd> | ||
</dl> | ||
<p> | ||
To <dfn data-export>create a CropTarget</dfn> with <var>element</var> as input, run the | ||
following steps: | ||
|
@@ -209,63 +255,6 @@ <h3><dfn>CropTarget</dfn> Definition</h3> | |
</li> | ||
</ol> | ||
</section> | ||
<section id="producecroptarget-method"> | ||
<h3>MediaDevices.produceCropTarget</h3> | ||
<pre class="idl"> | ||
partial interface MediaDevices { | ||
Promise<CropTarget> | ||
produceCropTarget(Element element); | ||
}; | ||
</pre> | ||
<dl data-link-for="MediaDevices" data-dfn-for="MediaDevices"> | ||
<dt> | ||
<dfn>produceCropTarget()</dfn> | ||
</dt> | ||
<dd> | ||
<p> | ||
Calling {{MediaDevices/produceCropTarget}} on 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 | ||
{{MediaDevices/produceCropTarget()}} in the current [=top-level browsing context=] or | ||
any of its | ||
<a data-cite="HTML#list-of-the-descendant-browsing-contexts" | ||
>descendant browsing contexts</a | ||
>. | ||
</p> | ||
<p> | ||
When {{MediaDevices/produceCropTarget}} is called on a given <var>element</var>, the | ||
user agent [=create a CropTarget|creates a CropTarget=] with <var>element</var> as | ||
input. The user agent MUST return a {{Promise}} <var>p</var>. The user agent MUST | ||
resolve <var>p</var> only after it has finished all the necessary internal propagation | ||
of state associated with the new {{CropTarget}}, at which point the user agent MUST be | ||
ready to receive the new {{CropTarget}} as a valid parameter to | ||
{{BrowserCaptureMediaStreamTrack/cropTo}}. | ||
</p> | ||
<p> | ||
When cloning an {{Element}} on which {{MediaDevices/produceCropTarget}} was previously | ||
called, the clone is not associated with any {{CropTarget}}. If | ||
{{MediaDevices/produceCropTarget}} is later called on the clone, a new {{CropTarget}} | ||
will be assigned to it. | ||
</p> | ||
</dd> | ||
</dl> | ||
<div class="note"> | ||
<p>There is no consensus yet on the following issues:</p> | ||
<ul> | ||
<li> | ||
Whether <code>produceCropTarget()</code> should be exposed on instances of | ||
{{MediaDevices}} or on instances of {{Element}}. This is under discussion in | ||
<a href="https://github.com/w3c/mediacapture-region/issues/11">issue #11</a>. | ||
</li> | ||
<li> | ||
Whether {{MediaDevices/produceCropTarget()}} should return a {{CropTarget}} or a | ||
{{Promise}}<{{CropTarget}}>. This is under discussion in | ||
<a href="https://github.com/w3c/mediacapture-region/issues/17">issue #17</a>. | ||
</li> | ||
</ul> | ||
</div> | ||
</section> | ||
</section> | ||
<section id="cropping-a-track"> | ||
<h2><dfn>Cropping Mechanism</dfn></h2> | ||
|
@@ -410,7 +399,7 @@ <h3>Crop-Session Lifetime</h3> | |
<h4>Crop-Session Definitions</h4> | ||
<p> | ||
We define an {{Element}} for which a {{CropTarget}} was produced (through a call to | ||
{{MediaDevices/produceCropTarget}}) as a <dfn>potential crop-target</dfn>. | ||
{{CropTarget/fromElement}}) as a <dfn>potential crop-target</dfn>. | ||
</p> | ||
<p> | ||
We define a [=potential crop-target=] which is targeted by a successful call to | ||
|
@@ -484,7 +473,7 @@ <h2>Sample Code</h2> | |
<p>Code in the capture-target:</p> | ||
<pre class="javascript"> | ||
const mainContentArea = navigator.getElementById('mainContentArea'); | ||
const cropTarget = await navigator.mediaDevices.produceCropTarget(mainContentArea); | ||
const cropTarget = await CropTarget.fromElement(mainContentArea); | ||
sendCropTarget(cropTarget); | ||
|
||
function sendCropTarget(cropTarget) { | ||
|
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:
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