I open a master bug for a future implementation of DOMMatrix. See link.
Created attachment 310221 [details] WIP Patch
Created attachment 310261 [details] WIP Patch
Created attachment 310262 [details] WIP Patch
Created attachment 310269 [details] WIP Patch
Comment on attachment 310269 [details] WIP Patch Attachment 310269 [details] did not pass bindings-ews (mac): Output: http://webkit-queues.webkit.org/results/3751479 New failing tests: (JS) JSTestObj.cpp (JS) JSTestTypedefs.cpp
Created attachment 310290 [details] WIP Patch
Created attachment 310291 [details] Patch
Comment on attachment 310291 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=310291&action=review > Source/WebCore/css/DOMMatrixInit.h:55 > + std::optional<double> is2D; I think this should probably be astd::optional<bool>.
Comment on attachment 310291 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=310291&action=review >> Source/WebCore/css/DOMMatrixInit.h:55 >> + std::optional<double> is2D; > > I think this should probably be astd::optional<bool>. Yes indeed :/ Thanks for catching this.
Sam, please take a look at the bindings generator change. I am a bit outdated when it comes to our typing mechanism in the bindings.
Created attachment 310295 [details] Patch
(In reply to Chris Dumez from comment #10) > Sam, please take a look at the bindings generator change. I am a bit > outdated when it comes to our typing mechanism in the bindings. For the constructor, previous it was generating: auto init = state->argument(0).isUndefined() ? std::optional<UNION*>() : convert<IDLUnion<IDLDOMString, IDLSequence<IDLUnrestrictedDouble>>>(*state, state->uncheckedArgument(0)); now it generates: auto init = state->argument(0).isUndefined() ? std::optional<Converter<IDLUnion<IDLDOMString, IDLSequence<IDLUnrestrictedDouble>>>::ReturnType>() : std::optional<Converter<IDLUnion<IDLDOMString, IDLSequence<IDLUnrestrictedDouble>>>::ReturnType>(convert<IDLUnion<IDLDOMString, IDLSequence<IDLUnrestrictedDouble>>>(*state, state->uncheckedArgument(0)));
Comment on attachment 310291 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=310291&action=review > Source/WebCore/css/DOMMatrix.cpp:50 > + auto result = DOMMatrix::fromMatrix(WTFMove(other)); > + if (result.hasException()) Seems wrong to call this "result" because it's not the result. > Source/WebCore/css/DOMMatrix.cpp:52 > + Ref<DOMMatrix> otherObject = result.releaseReturnValue(); auto? > Source/WebCore/css/DOMMatrix.h:64 > + Ref<DOMMatrix> rotateSelf(double rotX = 0, std::optional<double> rotY = std::nullopt, std::optional<double> rotZ = std::nullopt); > + Ref<DOMMatrix> rotateFromVectorSelf(double x = 0, double y = 0); > + Ref<DOMMatrix> rotateAxisAngleSelf(double x = 0, double y = 0, double z = 0, double angle = 0); > + Ref<DOMMatrix> skewXSelf(double sx = 0); > + Ref<DOMMatrix> skewYSelf(double sy = 0); Would be nice if we had a "typedef double Degrees" or a Degrees class like Seconds, so we always knew what units angles were in. At least add a comment, or rename the parameters. > Source/WebCore/css/DOMMatrix.h:91 > + void setM13(double f) { m_matrix.setM13(f); m_is2D = false; } > + void setM14(double f) { m_matrix.setM14(f); m_is2D = false; } > + void setM21(double f) { m_matrix.setM21(f); } > + void setM22(double f) { m_matrix.setM22(f); } > + void setM23(double f) { m_matrix.setM23(f); m_is2D = false; } > + void setM24(double f) { m_matrix.setM24(f); m_is2D = false; } > + void setM31(double f) { m_matrix.setM31(f); m_is2D = false; } > + void setM32(double f) { m_matrix.setM32(f); m_is2D = false; } > + void setM33(double f) { m_matrix.setM33(f); m_is2D = false; } > + void setM34(double f) { m_matrix.setM34(f); m_is2D = false; } > + void setM41(double f) { m_matrix.setM41(f); } > + void setM42(double f) { m_matrix.setM42(f); } > + void setM43(double f) { m_matrix.setM43(f); m_is2D = false; } > + void setM44(double f) { m_matrix.setM44(f); m_is2D = false; } You should only set m_is2D to false if the new value is not 0 (or 1 depending on the property). > Source/WebCore/css/DOMMatrix.idl:37 > + // These attributes are simple aliases for certain elements of the 4x4 matrix I wish "certain elements" were spelled out. Maybe you can put a comment next to each one? > Source/WebCore/css/DOMMatrix.idl:88 > + DOMMatrix rotateSelf(optional unrestricted double rotX = 0, > + optional unrestricted double rotY, > + optional unrestricted double rotZ); > + DOMMatrix rotateFromVectorSelf(optional unrestricted double x = 0, > + optional unrestricted double y = 0); > + DOMMatrix rotateAxisAngleSelf(optional unrestricted double x = 0, > + optional unrestricted double y = 0, > + optional unrestricted double z = 0, > + optional unrestricted double angle = 0); > + DOMMatrix skewXSelf(optional unrestricted double sx = 0); > + DOMMatrix skewYSelf(optional unrestricted double sy = 0); Would be nice if the IDL told me angles were in degrees. > Source/WebCore/css/DOMMatrixReadOnly.cpp:41 > + , m_is2D(is2D == Is2D::Yes) Shouldn't we check if the TransformationMatrix is 2D or not with isAffine()? > Source/WebCore/css/DOMMatrixReadOnly.cpp:46 > +ExceptionOr<void> DOMMatrixReadOnly::validateAndFixup(DOMMatrixInit& other) "other" is a bad name for the argument. It makes me thing this is an operator or member function, but it's just a static checker. > Source/WebCore/css/DOMMatrixReadOnly.cpp:54 > + return Exception { TypeError, ASCIILiteral("Mismatch between 2D and 3D matrix values") }; Would be nice if the error said exactly which thing was wrong. > Source/WebCore/css/DOMMatrixReadOnly.cpp:59 > + return Exception { TypeError, ASCIILiteral("m13 / m14 / m23 / m24 / m31 / m32 / m34 / m43 should be 0 for a 2D matrix") }; Would be nice if the error said exactly which thing was wrong. > Source/WebCore/css/DOMMatrixReadOnly.cpp:61 > + return Exception { TypeError, ASCIILiteral("m33 / m44 should be 1 for a 2D matrix") }; Would be nice if the error said exactly which thing was wrong. > Source/WebCore/css/DOMMatrixReadOnly.cpp:86 > +ExceptionOr<Ref<DOMMatrixReadOnly>> DOMMatrixReadOnly::fromMatrix(DOMMatrixInit&& other) "other" bad here too. > Source/WebCore/css/DOMMatrixReadOnly.cpp:146 > +return Exception { TypeError }; Indentation. > Source/WebCore/css/DOMMatrixReadOnly.cpp:166 > + auto matrix = DOMMatrix::create(m_matrix, m_is2D ? Is2D::Yes : Is2D::No); Maybe there should be a helper for DOMMatrix::create(m_matrix, m_is2D ? Is2D::Yes : Is2D::No); since it's repeated. > Source/WebCore/css/DOMMatrixReadOnly.cpp:229 > + return Exception { INVALID_STATE_ERR, ASCIILiteral("Matrix contains non-finite values") }; Bonus points for saying which entries are non-finite. > Source/WebCore/css/DOMMatrixReadOnly.idl:37 > + // These attributes are simple aliases for certain elements of the 4x4 matrix Clarify "certain" here. > Source/WebCore/css/DOMMatrixReadOnly.idl:89 > + [NewObject] DOMMatrix rotate(optional unrestricted double rotX = 0, > + optional unrestricted double rotY, > + optional unrestricted double rotZ); > + [NewObject] DOMMatrix rotateFromVector(optional unrestricted double x = 0, > + optional unrestricted double y = 0); > + [NewObject] DOMMatrix rotateAxisAngle(optional unrestricted double x = 0, > + optional unrestricted double y = 0, > + optional unrestricted double z = 0, > + optional unrestricted double angle = 0); > + [NewObject] DOMMatrix skewX(optional unrestricted double sx = 0); > + [NewObject] DOMMatrix skewY(optional unrestricted double sy = 0); Would be nice if the IDL told me angles were in degrees. > LayoutTests/imported/w3c/ChangeLog:41 > + do not support this for other types in this spec either (e.g. DOMPoint). Can you file a bug on this? > LayoutTests/imported/w3c/web-platform-tests/css/geometry-1/DOMMatrix-001-expected.txt:30 > +FAIL new DOMMatrix("translateX(5rem)") assert_throws: function "function () { new self[constr](string); }" did not throw > +FAIL new DOMMatrix("translateX(5vw)") assert_throws: function "function () { new self[constr](string); }" did not throw > +FAIL new DOMMatrix("translateX(5vh)") assert_throws: function "function () { new self[constr](string); }" did not throw > +FAIL new DOMMatrix("translateX(5vmin)") assert_throws: function "function () { new self[constr](string); }" did not throw > +FAIL new DOMMatrix("translateX(5vmax)") assert_throws: function "function () { new self[constr](string); }" did not throw These should throw with your patch, right?
Comment on attachment 310291 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=310291&action=review >> Source/WebCore/css/DOMMatrixReadOnly.cpp:41 >> + , m_is2D(is2D == Is2D::Yes) > > Shouldn't we check if the TransformationMatrix is 2D or not with isAffine()? Do you mean an assertion?
(In reply to Chris Dumez from comment #14) > Comment on attachment 310291 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=310291&action=review > > >> Source/WebCore/css/DOMMatrixReadOnly.cpp:41 > >> + , m_is2D(is2D == Is2D::Yes) > > > > Shouldn't we check if the TransformationMatrix is 2D or not with isAffine()? > > Do you mean an assertion? No, something like m_is2D = matrix.isAffine() ? Is2D::Yes : Is2D::No. Because you might be creating a DOMMatrixReadOnly with an arbitrary TransformationMatrix, which can have 3D in it.
(In reply to Simon Fraser (smfr) from comment #15) > (In reply to Chris Dumez from comment #14) > > Comment on attachment 310291 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=310291&action=review > > > > >> Source/WebCore/css/DOMMatrixReadOnly.cpp:41 > > >> + , m_is2D(is2D == Is2D::Yes) > > > > > > Shouldn't we check if the TransformationMatrix is 2D or not with isAffine()? > > > > Do you mean an assertion? > > No, something like m_is2D = matrix.isAffine() ? Is2D::Yes : Is2D::No. > > Because you might be creating a DOMMatrixReadOnly with an arbitrary > TransformationMatrix, which can have 3D in it. I think this should be an assertion to make sure the passed in is2D is adequate for the passed in Matrix. Let me explain why. In the spec, the DOMMatrix maintains an Is2D flag. separate from the Matrix. Relying on the matrix to determine DOMMatrix.is2D is not adequate. Consider this example: const matrix = DOMMatrixReadonly.fromMatrix({is2D: false}); assert_equals(String(matrix), "matrix3d(1, 0, 0, 0, 0, 1, 0, 0, 0, 0, 1, 0, 0, 0, 0, 1)"); With my code, we pass this check. if I make the change you suggest (if I understood correctly), we get: "matrix(1, 0, 0, 1, 0, 0)"
Created attachment 310301 [details] Patch
Generator change looks fine. We should consider switching the NativeType stuff over to just using IDLTypes directly, rather than hardcoding some types, but that can be left to another day.
Oh, could you add a bindings tests with a union in the constructor like you have as well?
(In reply to Sam Weinig from comment #18) > Generator change looks fine. We should consider switching the NativeType > stuff over to just using IDLTypes directly, rather than hardcoding some > types, but that can be left to another day. Ok, thanks for checking! And Yes, I'll add a bindings test.
(In reply to Chris Dumez from comment #16) > (In reply to Simon Fraser (smfr) from comment #15) > > (In reply to Chris Dumez from comment #14) > > > Comment on attachment 310291 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=310291&action=review > > > > > > >> Source/WebCore/css/DOMMatrixReadOnly.cpp:41 > > > >> + , m_is2D(is2D == Is2D::Yes) > > > > > > > > Shouldn't we check if the TransformationMatrix is 2D or not with isAffine()? > > > > > > Do you mean an assertion? > > > > No, something like m_is2D = matrix.isAffine() ? Is2D::Yes : Is2D::No. > > > > Because you might be creating a DOMMatrixReadOnly with an arbitrary > > TransformationMatrix, which can have 3D in it. > > I think this should be an assertion to make sure the passed in is2D is > adequate for the passed in Matrix. Let me explain why. That seems OK. I was basing my comment simply on a function that constructed a DOMMatrixReadonly with an arbitrary TransformationMatrix and an is2D flag, with no checking that they agree on 2D-ness.
Created attachment 310305 [details] Patch
(In reply to Chris Dumez from comment #22) > Created attachment 310305 [details] > Patch Added the bindings test.
Comment on attachment 310305 [details] Patch Giving Simon some time to comment before I land.
I've r+'d this patch twice and got conflicts every time.
Committed r216959: <http://trac.webkit.org/changeset/216959>
(In reply to Simon Fraser (smfr) from comment #25) > I've r+'d this patch twice and got conflicts every time. Sorry about that.
Looks like some test results haven't been updated for this new DOM object. Here's *all* of the places where LayoutTests/js/dom/global-constructors-attributes.html has results: $ find LayoutTests -name global-constructors-attributes-expected.txt LayoutTests/js/dom/global-constructors-attributes-expected.txt LayoutTests/platform/gtk/js/dom/global-constructors-attributes-expected.txt LayoutTests/platform/mac/js/dom/global-constructors-attributes-expected.txt LayoutTests/platform/mac-elcapitan/js/dom/global-constructors-attributes-expected.txt LayoutTests/platform/mac-wk1/js/dom/global-constructors-attributes-expected.txt LayoutTests/platform/mac-yosemite/js/dom/global-constructors-attributes-expected.txt LayoutTests/platform/win/js/dom/global-constructors-attributes-expected.txt LayoutTests/platform/wpe/js/dom/global-constructors-attributes-expected.txt
(In reply to David Kilzer (:ddkilzer) from comment #28) > Looks like some test results haven't been updated for this new DOM object. > > Here's *all* of the places where > LayoutTests/js/dom/global-constructors-attributes.html has results: > > $ find LayoutTests -name global-constructors-attributes-expected.txt > LayoutTests/js/dom/global-constructors-attributes-expected.txt > LayoutTests/platform/gtk/js/dom/global-constructors-attributes-expected.txt > LayoutTests/platform/mac/js/dom/global-constructors-attributes-expected.txt > LayoutTests/platform/mac-elcapitan/js/dom/global-constructors-attributes- > expected.txt > LayoutTests/platform/mac-wk1/js/dom/global-constructors-attributes-expected. > txt > LayoutTests/platform/mac-yosemite/js/dom/global-constructors-attributes- > expected.txt > LayoutTests/platform/win/js/dom/global-constructors-attributes-expected.txt > LayoutTests/platform/wpe/js/dom/global-constructors-attributes-expected.txt Some examples: https://build.webkit.org/results/Apple%20Sierra%20Debug%20WK2%20(Tests)/r216971%20(1047)/js/dom/global-constructors-attributes-diff.txt https://build.webkit.org/results/Apple%20Sierra%20Release%20WK1%20(Tests)/r216971%20(1683)/js/dom/global-constructors-attributes-diff.txt https://build.webkit.org/results/Apple%20Sierra%20Debug%20WK1%20(Tests)/r216971%20(1265)/js/dom/global-constructors-attributes-diff.txt I'm sure why some bots don't hit this failure, though, unless the test is skipped: https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=js%2Fdom%2Fglobal-constructors-attributes.html
(In reply to David Kilzer (:ddkilzer) from comment #29) > I'm sure why some bots don't hit this failure, though, unless the test is > skipped: > > https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=js%2Fdom%2Fglobal-constructors-attributes.html Skipped or marked as failing (inconsistently): $ grep -l -r global-constructors-attributes LayoutTests/TestExpectations LayoutTests/platform LayoutTests/platform/gtk/TestExpectations LayoutTests/platform/ios/TestExpectations LayoutTests/platform/ios-wk2/TestExpectations LayoutTests/platform/mac-wk1/TestExpectations LayoutTests/platform/mac-wk2/TestExpectations LayoutTests/platform/win/TestExpectations
(In reply to David Kilzer (:ddkilzer) from comment #28) > Looks like some test results haven't been updated for this new DOM object. > > Here's *all* of the places where > LayoutTests/js/dom/global-constructors-attributes.html has results: > > $ find LayoutTests -name global-constructors-attributes-expected.txt > LayoutTests/js/dom/global-constructors-attributes-expected.txt > LayoutTests/platform/gtk/js/dom/global-constructors-attributes-expected.txt > LayoutTests/platform/mac/js/dom/global-constructors-attributes-expected.txt > LayoutTests/platform/mac-elcapitan/js/dom/global-constructors-attributes- > expected.txt > LayoutTests/platform/mac-wk1/js/dom/global-constructors-attributes-expected. > txt > LayoutTests/platform/mac-yosemite/js/dom/global-constructors-attributes- > expected.txt > LayoutTests/platform/win/js/dom/global-constructors-attributes-expected.txt > LayoutTests/platform/wpe/js/dom/global-constructors-attributes-expected.txt Ok, I will fix this morning, thanks. For what it’s worth, EWS was happy.
(In reply to Chris Dumez from comment #31) > (In reply to David Kilzer (:ddkilzer) from comment #28) > > Looks like some test results haven't been updated for this new DOM object. > > > > Here's *all* of the places where > > LayoutTests/js/dom/global-constructors-attributes.html has results: > > > > $ find LayoutTests -name global-constructors-attributes-expected.txt > > LayoutTests/js/dom/global-constructors-attributes-expected.txt > > LayoutTests/platform/gtk/js/dom/global-constructors-attributes-expected.txt > > LayoutTests/platform/mac/js/dom/global-constructors-attributes-expected.txt > > LayoutTests/platform/mac-elcapitan/js/dom/global-constructors-attributes- > > expected.txt > > LayoutTests/platform/mac-wk1/js/dom/global-constructors-attributes-expected. > > txt > > LayoutTests/platform/mac-yosemite/js/dom/global-constructors-attributes- > > expected.txt > > LayoutTests/platform/win/js/dom/global-constructors-attributes-expected.txt > > LayoutTests/platform/wpe/js/dom/global-constructors-attributes-expected.txt > > Ok, I will fix this morning, thanks. For what it’s worth, EWS was happy. Test was rebaselined in <http://trac.webkit.org/changeset/216976>.