Bug 110001

Summary: Implement DOMMatrix / DOMMatrixReadOnly
Product: WebKit Reporter: Dirk Schulze <krit>
Component: WebCore Misc.Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, cdumez, dbates, ddkilzer, dino, oliver, sam, simon.fraser, syoichi, zcorpan
Priority: P2 Keywords: WebExposed
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
URL: https://drafts.fxtf.org/geometry/#DOMMatrix
Bug Depends on: 50633, 110048, 84006, 110002    
Bug Blocks: 163505, 172191    
Attachments:
Description Flags
WIP Patch
none
WIP Patch
none
WIP Patch
none
WIP Patch
buildbot: commit-queue-
WIP Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Dirk Schulze 2013-02-15 20:28:28 PST
I open a master bug for a future implementation of DOMMatrix. See link.
Comment 1 Chris Dumez 2017-05-15 21:42:19 PDT
Created attachment 310221 [details]
WIP Patch
Comment 2 Chris Dumez 2017-05-16 08:11:18 PDT
Created attachment 310261 [details]
WIP Patch
Comment 3 Chris Dumez 2017-05-16 08:23:03 PDT
Created attachment 310262 [details]
WIP Patch
Comment 4 Chris Dumez 2017-05-16 10:16:00 PDT
Created attachment 310269 [details]
WIP Patch
Comment 5 Build Bot 2017-05-16 10:19:39 PDT
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
Comment 6 Chris Dumez 2017-05-16 11:56:22 PDT
Created attachment 310290 [details]
WIP Patch
Comment 7 Chris Dumez 2017-05-16 12:20:29 PDT
Created attachment 310291 [details]
Patch
Comment 8 Sam Weinig 2017-05-16 13:31:53 PDT
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 9 Chris Dumez 2017-05-16 13:33:16 PDT
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.
Comment 10 Chris Dumez 2017-05-16 13:34:14 PDT
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.
Comment 11 Chris Dumez 2017-05-16 13:38:30 PDT
Created attachment 310295 [details]
Patch
Comment 12 Chris Dumez 2017-05-16 13:39:58 PDT
(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 13 Simon Fraser (smfr) 2017-05-16 13:48:10 PDT
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 14 Chris Dumez 2017-05-16 14:23:21 PDT
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?
Comment 15 Simon Fraser (smfr) 2017-05-16 14:38:19 PDT
(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.
Comment 16 Chris Dumez 2017-05-16 14:48:48 PDT
(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)"
Comment 17 Chris Dumez 2017-05-16 14:49:23 PDT
Created attachment 310301 [details]
Patch
Comment 18 Sam Weinig 2017-05-16 15:06:26 PDT
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.
Comment 19 Sam Weinig 2017-05-16 15:06:59 PDT
Oh, could you add a bindings tests with a union in the constructor like you have as well?
Comment 20 Chris Dumez 2017-05-16 15:07:35 PDT
(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.
Comment 21 Simon Fraser (smfr) 2017-05-16 15:09:23 PDT
(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.
Comment 22 Chris Dumez 2017-05-16 15:10:51 PDT
Created attachment 310305 [details]
Patch
Comment 23 Chris Dumez 2017-05-16 15:11:03 PDT
(In reply to Chris Dumez from comment #22)
> Created attachment 310305 [details]
> Patch

Added the bindings test.
Comment 24 Chris Dumez 2017-05-16 15:59:20 PDT
Comment on attachment 310305 [details]
Patch

Giving Simon some time to comment before I land.
Comment 25 Simon Fraser (smfr) 2017-05-16 16:18:42 PDT
I've r+'d this patch twice and got conflicts every time.
Comment 26 Chris Dumez 2017-05-16 16:45:19 PDT
Committed r216959: <http://trac.webkit.org/changeset/216959>
Comment 27 Chris Dumez 2017-05-16 16:45:51 PDT
(In reply to Simon Fraser (smfr) from comment #25)
> I've r+'d this patch twice and got conflicts every time.

Sorry about that.
Comment 28 David Kilzer (:ddkilzer) 2017-05-17 05:12:12 PDT
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
Comment 29 David Kilzer (:ddkilzer) 2017-05-17 05:22:29 PDT
(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
Comment 30 David Kilzer (:ddkilzer) 2017-05-17 05:25:21 PDT
(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
Comment 31 Chris Dumez 2017-05-17 07:05:32 PDT
(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.
Comment 32 Chris Dumez 2017-05-17 09:23:28 PDT
(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>.