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

[geometry] DOMMatrix constructor is a performance and code portability footgun #346

Open
bzbarsky opened this issue Jun 18, 2019 · 15 comments

Comments

@bzbarsky
Copy link

commented Jun 18, 2019

Consider what happens if one does:

new DOMMatrix(new DOMMatrix())

(or in general passes a DOMMatrix instance to the DOMMatrix constructor). The answer is that, since DOMMatrix is not iterable, it will get converted to a string of the form "matrix(stuff)", per https://drafts.fxtf.org/geometry/#dommatrixreadonly-stringification-behavior and then the constructor will either re-parse that string to create a DOMMatrix or throw, depending on whether it's running in a Window or a worker. This seems like a code portability (between windows and workers) footgun, as well as a performance footgun in the Window case.

Why, exactly, was the overload of the constructor that takes DOMMatrixReadOnly removed? It seems to me that we should reinstate it and either make it consistently work (so people can use it) or consistently throw (so people know to not use it and use the fromMatrix thing instead)... The current setup where it kinda works, but only in Window scopes doesn't seem great.

@zcorpan do you recall why this was set up like this?

@saschanaz

@zcorpan

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

This was part of the change that introduced static methods to avoid overloading as much as possible. The string argument in the constructor was necessary for compat with web content using WebKitCSSMatrix().

62b9cb9

cc @domenic

@bzbarsky

This comment has been minimized.

Copy link
Author

commented Jun 18, 2019

OK, but was the goal to remove "overloading" in the sense of using Web IDL overloads, or to reduce the set of things that could be passed in? Because we can easily add DOMMatrixReadOnly to the union, right?

(Note that this situation is quite different from the one for DOMPoint, DOMRect, etc.)

@zcorpan

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

The goal was to reduce WebIDL overloads.

That said, if people do new DOMMatrix(new DOMMatrix()) (or variants), then I think it makes sense to add a DOMMatrixReadOnly overload. The only problem I think would be reduced consistency with other geometry APIs, but as you point out the current setup is a bit problematic.

@bzbarsky

This comment has been minimized.

Copy link
Author

commented Jun 18, 2019

It's increased consistency on workers (where you can pass in a DOMPoint to the DOMPoint constructor and have it work, for example).

@zcorpan

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

The new point would have x: NaN, y: 0, z: 0, w: 1, regardless of the passed point's properties, I believe. So it wouldn't throw, but also not really consistent?

@bzbarsky

This comment has been minimized.

Copy link
Author

commented Jun 18, 2019

Hmm, so it would. I thought for some reason that we still had a DOMPointInit constructor version for DOMPoint. In that case, this is just an unfortunate interaction with the toString bit and the string constructor....

@saschanaz

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

That said, if people do new DOMMatrix(new DOMMatrix())

@bzbarsky should we add a use counter for this, or do you think we should add the overload regardless of use count?

If we are to add a counter, is it possible to add a counter for each overload on Firefox?

@bzbarsky

This comment has been minimized.

Copy link
Author

commented Jun 26, 2019

You can add a counter to each overload in Firefox, yes. You just have to do it manually, not via the IDL annotation.

I'm not sure a use counter is all that important here; the inconsistency between workers and mainthread bothers me more.

@tabatkins

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

I'm curious - why did we want to reduce IDL overloads?

@tabatkins

This comment has been minimized.

Copy link
Member

commented Jun 27, 2019

Ah, right! Thanks for digging up those references!

Okay, so it looks like we were just a bit overzealous here, then; we were already mentally committed to having the constructor take a DOMMatrixInit (which you can't distinguish from any other object), and so moved the entire chunk of argument-space over to a static method. Whereas we could have left DOMMatrixReadonly in the constructor's union, satisfying bz's concern in this thread and still fixing the original problem.

That would just mean that you could pass a literal DOMMatrix either in the constructor or the .fromMatrix() method and get the same result, but that seems fine.

@zcorpan

This comment has been minimized.

Copy link
Member

commented Jun 27, 2019

Agree that that seems fine

@dirkschulze

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2019

@bz @zcorpan @tabatkins So are you in agreement that we should re-add the overload for DOMMatrixReadOnly to the DOMMatrix constructor?

@tabatkins

This comment has been minimized.

Copy link
Member

commented Jun 28, 2019

Yeah, I think it makes sense, and solves bz's very reasonable use-case.

@dirkschulze dirkschulze added the Agenda+ label Jul 1, 2019

@css-meeting-bot

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

The CSS Working Group just discussed DOMMatrix constructor is a performance and code portability footgun, and agreed to the following:

  • RESOLVED: Add back in the overflow for DOMMatrix readonly to the DOMMatric constructor
The full IRC log of that discussion <dael> Topic: DOMMatrix constructor is a performance and code portability footgun
<dael> github: https://github.com//issues/346
<dael> astearns: krit sent regrets. TabAtkins said makes sense.
<dael> chris: [missed] said he didn't need to be in discussion and happy with what we agreed
<dael> astearns: Proposal was [reads]
<dael> AmeliaBR: I don't entirely understand it either. Right now the constructor allows either a string or an iterrable object. In certain env if you pass another matrix object as a param it gets serialized to a string and reparsed and act like it works. In others it won't happens and it thorws. Concern this is bad
<dael> AmeliaBR: Not sure why sometimes serialized and not others
<dael> AmeliaBR: Request is support another constructor overload that you can construct any matrix by copying values of another matrix object
<dael> heycam: Don't understand why it is worker throws exception
<dael> dbaron: I suspect some serialization or parsing code not exposed to workers.
<dbaron> yeah, the stringifier is Window-only
<dael> AmeliaBR: If behavior in a window is it serializes and parse back again it doesn't sound efficient. Direct cloning is more efficient
<dael> dbaron: Diff is because stringifier is DOM only.
<dael> dbaron: There's consensus in issue. Asking WG to decalre that
<dael> astearns: I don't see anyone saying it's a bad idea to revert the change
<dael> dbaron: And it's part of a change being reverted, not the entire
<dael> heycam: I also don't understand underlying motivation
<dael> astearns: Prop: Add back in the overflow for DOMMatrix readonly to the DOMMatric constructor
<dael> astearns: Objections?
<heycam> s/motivation of removing overloads in general/
<heycam> s/motivation/motivation of removing overloads in general/
<dael> AmeliaBR: Question- do we have multi-impl consensus?
<dael> astearns: I see blink and gecko in comments
<dael> AmeliaBR: Prob okay unless someone from WK objects
<dael> RESOLVED: Add back in the overflow for DOMMatrix readonly to the DOMMatric constructor
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.