Bug 110001 – Implement DOMMatrix / DOMMatrixReadOnly

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Dirk Schulze]

I open a master bug for a future implementation of DOMMatrix. See link.

[Chris Dumez]

Created attachment 310221 [details]
WIP Patch

[Chris Dumez]

Created attachment 310261 [details]
WIP Patch

[Chris Dumez]

Created attachment 310262 [details]
WIP Patch

[Chris Dumez]

Created attachment 310269 [details]
WIP Patch

[Build Bot]

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

[Chris Dumez]

Created attachment 310290 [details]
WIP Patch

[Chris Dumez]

Created attachment 310291 [details]
Patch

[Sam Weinig]

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

[Chris Dumez]

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.

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 310295 [details]
Patch

[Chris Dumez]

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

[Simon Fraser (smfr)]

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?

[Chris Dumez]

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?

[Simon Fraser (smfr)]

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

[Chris Dumez]

(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)"

[Chris Dumez]

Created attachment 310301 [details]
Patch

[Sam Weinig]

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.

[Sam Weinig]

Oh, could you add a bindings tests with a union in the constructor like you have as well?

[Chris Dumez]

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

[Simon Fraser (smfr)]

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

[Chris Dumez]

Created attachment 310305 [details]
Patch

[Chris Dumez]

(In reply to Chris Dumez from comment #22)
> Created attachment 310305 [details]
> Patch

Added the bindings test.

[Chris Dumez]

Comment on attachment 310305 [details]
Patch

Giving Simon some time to comment before I land.

[Simon Fraser (smfr)]

I've r+'d this patch twice and got conflicts every time.

[Chris Dumez]

Committed r216959: <http://trac.webkit.org/changeset/216959>

[Chris Dumez]

(In reply to Simon Fraser (smfr) from comment #25)
> I've r+'d this patch twice and got conflicts every time.

Sorry about that.

[David Kilzer (:ddkilzer)]

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

[David Kilzer (:ddkilzer)]

(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

[David Kilzer (:ddkilzer)]

(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

[Chris Dumez]

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

[Chris Dumez]

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