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.

Sign up for GitHub

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

Jump to bottom

MediaDevices.produceCropTarget -> CropTarget.fromElement #50

Merged
merged 11 commits into from
Jun 2, 2022
Merged

MediaDevices.produceCropTarget -> CropTarget.fromElement #50

Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
cc769c8
x
eladalon1983 May 20, 2022
cee2d96
MediaDevices.produceCropTarget -> CropTarget.fromElement
eladalon1983 May 20, 2022
4440087
Code review pass #1
eladalon1983 May 24, 2022
cb1d78e
Update index.html
eladalon1983 May 24, 2022
3e0b086
Re-run formatter
eladalon1983 May 24, 2022
71f998e
Update sample code
eladalon1983 May 24, 2022
27359e5
Code review pass #2
eladalon1983 May 24, 2022
d5628df
Update index.html
eladalon1983 May 27, 2022
e2ccdd8
Update index.html
eladalon1983 May 27, 2022
ffce3e3
Add note about SecureContext
eladalon1983 May 31, 2022
82c3ec7
Merge branch 'crop_to' of https://github.com/eladalon1983/mediacaptur…
eladalon1983 May 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
102 changes: 42 additions & 60 deletions index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ <h3><dfn>CropTarget</dfn> Definition</h3>
<pre class="idl">
Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

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 ;)

Copy link
Member Author

@eladalon1983 eladalon1983 May 24, 2022 •
edited
Loading

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?

Copy link
Contributor

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member Author

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".

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM

[Exposed=(Window,Worker), Serializable]
interface CropTarget {
// Intentionally empty; just an opaque identifier.
[SecureContext] static Promise&lt;CropTarget&gt; fromElement(Element element);
eladalon1983 marked this conversation as resolved.
Show resolved Hide resolved
};
</pre>
<div class="note">
Expand All @@ -160,7 +160,46 @@ <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>
<dl data-link-for="CropTarget" data-dfn-for="CropTarget">
<dt>
<dfn>fromElement()</dfn>
</dt>
<dd>
<p>
Calling {{CropTarget/fromElement}} on an {{Element}} of a supported type associates
eladalon1983 marked this conversation as resolved.
Show resolved Hide resolved
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
Copy link
Member

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.

Copy link
Member Author

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.

{{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 on a given <var>element</var>, the user
eladalon1983 marked this conversation as resolved.
Show resolved Hide resolved
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 the following issue: Whether
{{CropTarget.fromElement()}} should return a {{CropTarget}} or a
eladalon1983 marked this conversation as resolved.
Show resolved Hide resolved
{{Promise}}&lt;{{CropTarget}}&gt;. This is under discussion in
<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:
Expand Down Expand Up @@ -209,63 +248,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&lt;CropTarget&gt;
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}}&lt;{{CropTarget}}&gt;. 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>
Expand Down Expand Up @@ -410,7 +392,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
Expand Down