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.

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

Is CropTarget name too generic? #18

Closed
youennf opened this issue Feb 2, 2022 · 18 comments · Fixed by #73
Closed

Is CropTarget name too generic? #18

youennf opened this issue Feb 2, 2022 · 18 comments · Fixed by #73

Comments

@youennf
Copy link
Contributor

youennf commented Feb 2, 2022

As it is, CropTarget is a generic term that could be applied to any region of a video frame, say for instance a head region from a camera feed, or a window in an application getDisplayMedia track.

As currently defined, CropTarget is more something like an element viewport.
In that sense, it might be good to think of renaming it to something more specific, for instance 'Viewport'.

@eladalon1983
Copy link
Member

eladalon1983 commented Feb 2, 2022

CropTarget is not an element viewport, it's a token. Previously it was called CropID. This name was changed at the suggestion of multiple parties during the WebRTC WG Interim of November 2021, under the argument that this is likely to be preferred by TAG. I'd be happy with either CropTarget or CropID, and I'd welcome even more suggestions. I don't think "Viewport" makes sense, unless you mean "CropViewport". Even then, I think it's misleading. It makes it even harder to figure out that this thing is a token.

@youennf
Copy link
Contributor Author

youennf commented Feb 4, 2022

Thinking further, CropTarget is nothing but a way to get live element bounding rect.
Renaming cropTarget to layoutBox would make sense with that regards.
Then you could write something that reads naturally: track.cropTo(element.layoutBox).

@youennf
Copy link
Contributor Author

youennf commented Feb 4, 2022

That also begs the question of the actual definition of CropTarget in the spec, it should probably relate to existing HTML/CSS structures.

@eladalon1983
Copy link
Member

  1. I think developers will be surprised to learn that when they produce a LayoutBox of an Element, they can't do anything with it other than use it for cropping. They wouldn't be able to get (x, y, width, height) out of it, etc. I think the name CropTarget is much clearer.
  2. You have made compelling arguments for furnishing the CropTarget interface with additional fields such as name and origin. I think those fields are much more sensible when exposed on a "CropTarget" than when exposed on a "LayoutBox".

@youennf
Copy link
Contributor Author

youennf commented Mar 8, 2022

Another name: ElementProxy. then cropTo would take a Element or ElementProxy.
The intent is clear and it is not surprising to not have all getters of an Element.

@eladalon1983
Copy link
Member

eladalon1983 commented Mar 8, 2022

Consider a Web-developer who is not interest in cropping at all.

  1. While inspecting Element, this developer comes across ElementProxy. They cannot immediately understand what this is for, and might needlessly spend time reading up on it.
  2. While inspecting Element, this developer comes across CropTarget. Knowing that they are not interested in anything cropping-related, they quickly move on.
  3. While inspecting Element, this developer never comes across this feature, because we expose it on MediaDevices.produceCropTarget. We never waste even a second of this developer's time.

I think that 3 > 2 > 1.

@eladalon1983
Copy link
Member

I don't think we've landed on a name better than CropTarget. I hope you have found my objections to "layoutBox" and "ElementProxy" compelling. Do you have any other suggestions, @youennf? Or shall we close this issue?

@youennf
Copy link
Contributor Author

youennf commented Mar 21, 2022

I am not sure looking at it from a web developer inspecting each property is the right angle here.
A better angle might be to look at what might come next.
Basically, either we decide to name this object according its current use (a reference used for cropping) or according what it represents (an element reference).

Either we augment CropTarget to expose more of Elements (say its name) and it is better to have a name that clarifies that this is tied to an element. It could for instance be reused in other APIs (dom element capture track can be exposed to workers and has an API to get its element for instance).

Or we might augment CropTarget API to allow cropping for various sources using the same CropTarget.
For instance, if we want in the future to allow cropping of a screen or a window, would we reuse CropTarget or something else?
My understanding from past discussions with you is that we would not reuse CropTarget here, hence why a name focused on what the object is (ElementReference, ElementProxy...) seems more appropriate at the moment.

Of course, who knows the future. But in any case, these are the kind of discussion I'd like to have to validate the name selection.

@eladalon1983
Copy link
Member

I have not yet heard a suggestion for a name better than CropTarget. I've added the documentation of our lack of consensus to PR #27. You're welcome to continue this discussion and bring up additional suggestions, but I hope this won't block us making more progress toward FPWD and beyond.

@lghall
Copy link

lghall commented Mar 23, 2022

I agree that CropTarget is the best name suggestion I've seen so far for this field. We have been using the region capture API at Google (I represent the Google Docs editors) and CropTarget would be a natural fit for how this object is used in code. Thanks!

@eladalon1983
Copy link
Member

eladalon1983 commented Mar 28, 2022

For instance, if we want in the future to allow cropping of a screen or a window, would we reuse CropTarget or something else?

I don't think we have a robust way of cropping windows wrt changes to window-internal layout. But should we find something in the future, that future mechanism could rely on a token that is marked as a sub-class of CropTarget. Or CropTarget could be extended to support that adjacent use-case too. I don't think the name will be an issue.

or according what it represents (an element reference).
...
hence why a name focused on what the object is (ElementReference, ElementProxy...) seems more appropriate at the moment.

  1. I don't think it's good to think of the token as a reference/proxy to the Element - see here.
  2. I don't see another use-case for a "weak reference" to an Element in another document. What, other than cropping, could we do? Why do we need a name that fits hypothetical future uses too?

Of course, who knows the future. But in any case, these are the kind of discussion I'd like to have to validate the name selection.

Please let me know if I have failed to address any of your points, if you still think we should go with any of the ideas you've already brought up, or if you have new naming ideas that you'd like to discuss.

@eladalon1983
Copy link
Member

@youennf, I'd like to conclude this discussion. Nobody else has stepped forth who is interested in changing the current naming scheme. May we close this issue?

@eladalon1983
Copy link
Member

Either we augment CropTarget to expose more of Elements (say its name) and it is better to have a name that clarifies that this is tied to an element. It could for instance be reused in other APIs

Consider a Web-developer who today writes and ships code that sends a CropTarget over postMessage(). That developer assumes that CropTarget exposes nothing, and can only be used for cropping (shaving off information). This greatly simplifies how that developer can reason about the safety of posting CropTarget around. If we were to reuse CropTarget in the future, it would run the risk of breaking that developer's reasonable assumption. CropTarget is intentionally crop-specific. Reuse for other APIs is not intended, and so we don't need a name that lends itself to arbitrary reuse.

@eladalon1983
Copy link
Member

@youennf, there been no activity here for a while, and you have yourself suggested CropTarget.fromElement(). May we close this discussion now?

@eladalon1983
Copy link
Member

Ping @youennf, let's please close this.

@youennf
Copy link
Contributor Author

youennf commented Jun 2, 2022

Discussed at today's editor's call.
Let's try to present it at next interim (next Tuesday) and have a WG decision there.

@eladalon1983
Copy link
Member

I'm not sure I can make the next interim. I'll let you know.

@alvestrand
Copy link

Discussed during interim meeting of June 2022.
Youenn's slides on the matter are here.
The consensus was to keep the name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants