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

Make CropTarget serializable #24

Closed
wants to merge 3 commits into from
+36 −1
Closed

Make CropTarget serializable #24

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bcb597f
Serialization
eladalon1983 Mar 8, 2022
83a6675
rm fixme
eladalon1983 Mar 14, 2022
545cc38
Add internal slot
eladalon1983 Mar 18, 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
37 changes: 36 additions & 1 deletion index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,14 @@ <h2>Abstract</h2>
a First Public Working Draft.
</section>
<section id="conformance"></section>
<section id="terminology">
<h2>Terminology</h2>
<p>
The terms
<dfn data-cite="HTML/structured-data.html#serializable-objects">serializable objects</dfn>,
[= serialization steps =], and [= deserialization steps =] are defined in [[!HTML]].
</p>
</section>
<section id="definitions">
<h2>Definitions</h2>
<p>
Expand Down Expand Up @@ -147,12 +155,39 @@ <h3><dfn>CropTarget</dfn> Definition</h3>
purpose is to be handed to {{BrowserCaptureMediaStreamTrack/cropTo}} as input.
</p>
<pre class="idl">
[Exposed = Window]
[Exposed = Window, Serializable]
interface CropTarget {
// Intentionally empty; just an opaque identifier.
};
</pre>
<dl data-link-for="CropTarget" data-dfn-for="CropTarget"></dl>

<p id="serializable-croptarget">
{{CropTarget}} objects are [= serializable objects =] [[!HTML]]. Their [= serialization
steps =], given <var>value</var> and <var>serialized</var>, are:
Comment on lines +166 to +167
Copy link
Member

Choose a reason for hiding this comment

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

I think we want to accept forStorage here as well, and throw an error (not sure which one, DataCloneError maybe?) if it is true.

Copy link
Member Author

Choose a reason for hiding this comment

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

"The introduction of these steps should omit mention of the forStorage argument if it is not relevant to the algorithm."

Copy link
Contributor

Choose a reason for hiding this comment

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

What is the use case for forStorage being set to true?

Copy link
Member

Choose a reason for hiding this comment

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

Do you want this to be able to end up in IDB/pushState/some other things or not, essentially.

Copy link
Member Author

Choose a reason for hiding this comment

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

I want it to be possible for this to be stringified and sent over a REST call, as I know some Web applications that can cooperate either in a single-tab or multi-tab setting, and they use REST (and REST-like) for as much as they can, since that's already set up and works for them. I think it's a valid use-case and there's no reason to forbid it.

Copy link
Member

Choose a reason for hiding this comment

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

You mean through Fetch? Serializable doesn't help with that.

Copy link
Member Author

Choose a reason for hiding this comment

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

If Serializable forStorage=true does not allow conversion to a string, could you please advise what does?

Copy link
Member

Choose a reason for hiding this comment

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

If you want to convert a platform object to a string, you need to define a stringifier for it. That seems like a topic for a new issue though as #21 doesn't cover it.

Copy link
Member Author

Choose a reason for hiding this comment

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

  • I've added a slot for [[Data]].
  • Indeed, stringification is a separate issue. If you happen to know the mechanism, I'd be happy to hear it. At any rate, we can do so as a follow-up.

</p>
<ol>
<li>
Set <var>serialized</var>.[[\Data]] a randomly generated, globally unique {{DOMString}}.
</li>
</ol>
<p>
Their <a>deserialization steps</a>, given <var>serialized</var> and <var>value</var>, are:
</p>
<ol>
<li>
Compare <var>serialized</var>.[[\Data]] against a record of all {{CropTargets}} produced
and serialized thus far in the current top-level [=browsing context=], or in any of its
descendent [=browsing contexts=].
<ol>
<li>
If <var>serialized</var>.[[\Data]] was obtained by serializing previous
{{CropTarget}}, then set <var>value</var> to an equivalent {{CropTarget}}.
Copy link
Contributor

@youennf youennf Mar 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This "equivalent {{CropTarget}}" is not very well defined.
We would be on solid ground by introducing in the spec a direct CropTarget<->Element relationship, through an element reference or a slot storing an element referrence.
{{CropTarget}}s would then be equivalent if referencing the same element.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this comment applies here too; only its second half is relevant, so I'll repeat:
It might be that using weak references is standard procedure and would be well understood by readers. If that is the case, then I do not wish to delay us further. Could you point out an example of a Transferable/Serializable object that stores a weak reference to an Element in the original context, and where the weak reference gets replicated in the new context?

</li>
<li>Otherwise, set <var>value</var> to {{undefined}}.</li>
Copy link
Contributor

Choose a reason for hiding this comment

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

If the serialisation steps are done correctly, we should always successfully deserialise, which does not seem to be the case here if we set value to undefined.
Is the intention to serialise a CropTarget and deserialise it into undefined?
Does this mean serialising a CropTarget from top level page A to top level page B then back to top level page A would not work?

Introducing a direct element <-> CropTarget link, for instance by having CropTarget having an element reference seems a good approach to tighten things.

Copy link
Member Author

@eladalon1983 eladalon1983 Mar 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The otherwise section intended to deal with deserialization of invalid input. However, since we don't have a (legal) way to obtain an invalid CropTarget, or an invalid serialization thereof, I'd gladly drop this line - if that is indeed what you're suggesting...?

</ol>
</li>
</ol>
</section>
<section id="producecroptarget-method">
<h3>MediaDevices.produceCropTarget</h3>
Expand Down