Bug 176048 – Implement DOMMatrix2DInit for setTransform()/addPath()

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Simon Pieters (:zcorpan)]

In https://bugs.webkit.org/show_bug.cgi?id=174278 setTransform(optional DOMMatrixInit transform) was added. This has now been specified using a 2D variant dictionary where "validate and fixup" ignores 3D members.

https://github.com/whatwg/html/pull/2926
https://github.com/w3c/fxtf-drafts/pull/211
https://github.com/w3c/web-platform-tests/pull/7007

[Sam Weinig]

Created attachment 319327 [details]
WIP

[Sam Weinig]

Created attachment 319329 [details]
Patch

[Build Bot]

Comment on attachment 319329 [details]
Patch

Attachment 319329 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4407984

New failing tests:
fast/canvas/canvas-path-addPath.html

[Build Bot]

Created attachment 319338 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 319329 [details]
Patch

Attachment 319329 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4408004

New failing tests:
fast/canvas/canvas-path-addPath.html

[Build Bot]

Created attachment 319339 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 319329 [details]
Patch

Attachment 319329 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4407989

New failing tests:
fast/canvas/canvas-path-addPath.html

[Build Bot]

Created attachment 319340 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 319329 [details]
Patch

Attachment 319329 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4408035

New failing tests:
fast/canvas/canvas-path-addPath.html

[Build Bot]

Created attachment 319343 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Sam Weinig]

Created attachment 319390 [details]
Patch

[Build Bot]

Comment on attachment 319390 [details]
Patch

Attachment 319390 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4412064

New failing tests:
imported/w3c/web-platform-tests/css/geometry-1/historical.html
imported/w3c/web-platform-tests/css/geometry-1/DOMMatrix-css-string.worker.html
imported/w3c/web-platform-tests/css/geometry-1/DOMMatrix-001.html

[Build Bot]

Created attachment 319397 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 319390 [details]
Patch

Attachment 319390 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4412078

New failing tests:
imported/w3c/web-platform-tests/css/geometry-1/historical.html
imported/w3c/web-platform-tests/css/geometry-1/DOMMatrix-css-string.worker.html
imported/w3c/web-platform-tests/css/geometry-1/DOMMatrix-001.html

[Build Bot]

Created attachment 319399 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 319390 [details]
Patch

Attachment 319390 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4412084

New failing tests:
imported/w3c/web-platform-tests/css/geometry-1/historical.html
imported/w3c/web-platform-tests/css/geometry-1/DOMMatrix-css-string.worker.html
imported/w3c/web-platform-tests/css/geometry-1/DOMMatrix-001.html

[Build Bot]

Created attachment 319402 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 319390 [details]
Patch

Attachment 319390 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4412136

New failing tests:
imported/w3c/web-platform-tests/css/geometry-1/historical.html
imported/w3c/web-platform-tests/css/geometry-1/DOMMatrix-css-string.worker.html
imported/w3c/web-platform-tests/css/geometry-1/DOMMatrix-001.html

[Build Bot]

Created attachment 319407 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Sam Weinig]

Created attachment 319436 [details]
Patch

[Sam Weinig]

Created attachment 319439 [details]
Patch

[Devin Rousso]

Comment on attachment 319439 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=319439&action=review

I was actually trying to implement this earlier today.  Didn't realize that this bug already existed :(

It looks fine to me, but I'm not sure if I'm allowed to review things in WebCore.  If so, I'd he happy to provide an r+ :)

> Source/WebCore/css/DOMMatrixReadOnly.cpp:81
> +ExceptionOr<void> DOMMatrixReadOnly::validateAndFixup(DOMMatrixInit& init)

The spec lists an `ignore3D` parameter that controls what checks are done when passed a DOMMatrixInit.  I realize that this is mostly covered by splitting `validateAndFixup` into two functions (one for 2D and one for 3D), but it might be worth adding it here to conform to the spec.

[Simon Pieters (:zcorpan)]

Having two validateAndFixup functions is perfectly conforming, so long as the end result is equivalent.

[Chris Dumez]

Comment on attachment 319439 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=319439&action=review

r=me

> LayoutTests/imported/w3c/web-platform-tests/css/geometry-1/DOMMatrixInit-validate-fixup-expected.txt:30
> +FAIL {f: NaN, m42: NaN} (2d) init.f and init.m42 do not match

Why this failure?

> LayoutTests/imported/w3c/web-platform-tests/css/geometry-1/DOMMatrixInit-validate-fixup-expected.txt:31
> +FAIL {f: NaN, m42: NaN, is2D: true} (2d) init.f and init.m42 do not match

Why this failure?

[Devin Rousso]

Comment on attachment 319439 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=319439&action=review

> Source/WebCore/css/DOMMatrixReadOnly.cpp:62
> +ExceptionOr<void> DOMMatrixReadOnly::validateAndFixup(DOMMatrix2DInit& init)

I took another look at the spec this morning and I think this might be slightly off.  Steps 2-7 of validate and fixup <https://drafts.fxtf.org/geometry/#matrix-validate-and-fixup> should still be performed on a DOMMatrix2DInit.  As such, I think these two validateAndFixup functions should be reworked as such:

    ExceptionOr<void> DOMMatrixReadOnly::validateAndFixup(DOMMatrix2DInit& init)
    {
        // FIXME: Should be using SameValueZero rather than c-equality.
        if (init.a && init.m11 && init.a.value() != init.m11.value())
            return Exception { TypeError, ASCIILiteral("init.a and init.m11 do not match") };
        if (init.b && init.m12 && init.b.value() != init.m12.value())
            return Exception { TypeError, ASCIILiteral("init.b and init.m12 do not match") };
        if (init.c && init.m21 && init.c.value() != init.m21.value())
            return Exception { TypeError, ASCIILiteral("init.c and init.m21 do not match") };
        if (init.d && init.m22 && init.d.value() != init.m22.value())
            return Exception { TypeError, ASCIILiteral("init.d and init.m22 do not match") };
        if (init.e && init.m41 && init.e.value() != init.m41.value())
            return Exception { TypeError, ASCIILiteral("init.e and init.m41 do not match") };
        if (init.f && init.m42 && init.f.value() != init.m42.value())
            return Exception { TypeError, ASCIILiteral("init.f and init.m42 do not match") };

        if (!init.m11)
            init.m11 = init.a.value_or(1);
        if (!init.m12)
            init.m12 = init.b.value_or(0);
        if (!init.m21)
            init.m21 = init.c.value_or(0);
        if (!init.m22)
            init.m22 = init.d.value_or(1);
        if (!init.m41)
            init.m41 = init.e.value_or(0);
        if (!init.m42)
            init.m42 = init.f.value_or(0);

        return { };
    }

    ExceptionOr<void> DOMMatrixReadOnly::validateAndFixup(DOMMatrixInit& init)
    {
        if (init.is2D && init.is2D.value()) {
            if (init.m13)
                return Exception { TypeError, ASCIILiteral("m13 should be 0 for a 2D matrix") };
            if (init.m14)
                return Exception { TypeError, ASCIILiteral("m14 should be 0 for a 2D matrix") };
            if (init.m23)
                return Exception { TypeError, ASCIILiteral("m23 should be 0 for a 2D matrix") };
            if (init.m24)
                return Exception { TypeError, ASCIILiteral("m24 should be 0 for a 2D matrix") };
            if (init.m31)
                return Exception { TypeError, ASCIILiteral("m31 should be 0 for a 2D matrix") };
            if (init.m32)
                return Exception { TypeError, ASCIILiteral("m32 should be 0 for a 2D matrix") };
            if (init.m34)
                return Exception { TypeError, ASCIILiteral("m34 should be 0 for a 2D matrix") };
            if (init.m43)
                return Exception { TypeError, ASCIILiteral("m43 should be 0 for a 2D matrix") };
            if (init.m33 != 1)
                return Exception { TypeError, ASCIILiteral("m33 should be 1 for a 2D matrix") };
            if (init.m44 != 1)
                return Exception { TypeError, ASCIILiteral("m44 should be 1 for a 2D matrix") };
        }

        auto validate2D = validateAndFixup(static_cast<DOMMatrix2DInit&>(init));
        if (validate2D.hasException())
            return validate2D.releaseException();

        if (!init.is2D) {
            if (init.m13 || init.m14 || init.m23 || init.m24 || init.m31 || init.m32 || init.m34 || init.m43 || init.m33 != 1 || init.m44 != 1)
                init.is2D = false;
            else
                init.is2D = true;
        }

        return { };
    }

Moving the `init.is2D && init.is2D.value()` if statement before the `validate2D`, while awkward, still conforms to the text "If if at least one of the following conditions are true for dict, throw a TypeError exception and abort these steps."

[Sam Weinig]

(In reply to Chris Dumez from comment #24)
> Comment on attachment 319439 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=319439&action=review
> 
> r=me
> 
> > LayoutTests/imported/w3c/web-platform-tests/css/geometry-1/DOMMatrixInit-validate-fixup-expected.txt:30
> > +FAIL {f: NaN, m42: NaN} (2d) init.f and init.m42 do not match
> 
> Why this failure?
> 
> > LayoutTests/imported/w3c/web-platform-tests/css/geometry-1/DOMMatrixInit-validate-fixup-expected.txt:31
> > +FAIL {f: NaN, m42: NaN, is2D: true} (2d) init.f and init.m42 do not match
> 
> Why this failure?

I think these are due to same issue the FIXME I added is about:

"// FIXME: Should be using SameValueZero rather than c-equality."

[Sam Weinig]

Created attachment 319473 [details]
Patch

[Devin Rousso]

Comment on attachment 319473 [details]
Patch

r=me

[Sam Weinig]

Created attachment 319475 [details]
Patch

[Build Bot]

Comment on attachment 319475 [details]
Patch

Attachment 319475 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4417586

New failing tests:
fast/canvas/canvas-path-addPath.html
fast/canvas/2d.setTransform.matrix.html
fast/canvas/2d.getTransform.modification.html

[Build Bot]

Created attachment 319486 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 319475 [details]
Patch

Attachment 319475 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4417615

New failing tests:
fast/canvas/canvas-path-addPath.html
fast/canvas/2d.setTransform.matrix.html
fast/canvas/2d.getTransform.modification.html

[Build Bot]

Created attachment 319487 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 319475 [details]
Patch

Attachment 319475 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4417614

New failing tests:
fast/canvas/canvas-path-addPath.html
fast/canvas/2d.setTransform.matrix.html
fast/canvas/2d.getTransform.modification.html

[Build Bot]

Created attachment 319491 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Sam Weinig]

Created attachment 319492 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 319492 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=319492&action=review

> Source/WebCore/css/DOMMatrixReadOnly.cpp:64
> +    // FIXME: Should be using SameValueZero rather than c-equality.

Does this need a bug to be filed?

> Source/WebCore/html/canvas/DOMPath.cpp:47
> +    m_path.addPath(path.path(), { matrixInit.a.value_or(1), matrixInit.b.value_or(0), matrixInit.c.value_or(0), matrixInit.d.value_or(1), matrixInit.e.value_or(0), matrixInit.f.value_or(0) });

Shouldn't this consult m11-m42 as well? It's really unclear how aliasing is supposed to be handled.

> LayoutTests/imported/w3c/web-platform-tests/css/geometry-1/DOMMatrix-css-string.worker-expected.txt:3
> +FAIL DOMMatrix constructor with empty string argument in worker assert_true: DOMMatrix should exist expected true got false

Why the fail?

> LayoutTests/imported/w3c/web-platform-tests/css/geometry-1/DOMMatrix-css-string.worker-expected.txt:8
> +FAIL DOMMatrixReadOnly constructor with empty string argument in worker assert_true: DOMMatrixReadOnly should exist expected true got false

Why the fail?

> LayoutTests/imported/w3c/web-platform-tests/css/geometry-1/DOMMatrix-css-string.worker.js:16
> +    assert_true(constr in self, `${constr} should exist`);
> +    assert_throws(new TypeError(), () => new self[constr]('') );
> +  }, `${constr} constructor with empty string argument in worker`);
> +
> +  test(() => {

What's the upstreaming plan?

[Sam Weinig]

(In reply to Simon Fraser (smfr) from comment #37)
> Comment on attachment 319492 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=319492&action=review
> 
> > Source/WebCore/css/DOMMatrixReadOnly.cpp:64
> > +    // FIXME: Should be using SameValueZero rather than c-equality.
> 
> Does this need a bug to be filed?
> 

I'm going to handle it shortly.

> > Source/WebCore/html/canvas/DOMPath.cpp:47
> > +    m_path.addPath(path.path(), { matrixInit.a.value_or(1), matrixInit.b.value_or(0), matrixInit.c.value_or(0), matrixInit.d.value_or(1), matrixInit.e.value_or(0), matrixInit.f.value_or(0) });
> 
> Shouldn't this consult m11-m42 as well? It's really unclear how aliasing is
> supposed to be handled.

validateAndFixup should throw if they differ.

> 
> > LayoutTests/imported/w3c/web-platform-tests/css/geometry-1/DOMMatrix-css-string.worker-expected.txt:3
> > +FAIL DOMMatrix constructor with empty string argument in worker assert_true: DOMMatrix should exist expected true got false
> 
> Why the fail?

We don't expose DOMMatrix in workers yet.

> 
> > LayoutTests/imported/w3c/web-platform-tests/css/geometry-1/DOMMatrix-css-string.worker-expected.txt:8
> > +FAIL DOMMatrixReadOnly constructor with empty string argument in worker assert_true: DOMMatrixReadOnly should exist expected true got false
> 
> Why the fail?

We don't expose DOMMatrix in workers yet.

> 
> > LayoutTests/imported/w3c/web-platform-tests/css/geometry-1/DOMMatrix-css-string.worker.js:16
> > +    assert_true(constr in self, `${constr} should exist`);
> > +    assert_throws(new TypeError(), () => new self[constr]('') );
> > +  }, `${constr} constructor with empty string argument in worker`);
> > +
> > +  test(() => {
> 
> What's the upstreaming plan?

This was a pull. No need to upstream.

[WebKit Commit Bot]

Comment on attachment 319492 [details]
Patch

Clearing flags on attachment: 319492

Committed r221462: <http://trac.webkit.org/changeset/221462>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34694074>