Bug 176048

Summary: Implement DOMMatrix2DInit for setTransform()/addPath()
Product: WebKit Reporter: Simon Pieters (:zcorpan) <simon>
Component: CanvasAssignee: Sam Weinig <sam>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, cdumez, commit-queue, dino, drousso, rniwa, sam, simon.fraser, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
WIP
none
Patch
none
Archive of layout-test-results from ews102 for mac-elcapitan
none
Archive of layout-test-results from ews105 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews114 for mac-elcapitan
none
Archive of layout-test-results from ews122 for ios-simulator-wk2
none
Patch
none
Archive of layout-test-results from ews103 for mac-elcapitan
none
Archive of layout-test-results from ews106 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews112 for mac-elcapitan
none
Archive of layout-test-results from ews124 for ios-simulator-wk2
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews102 for mac-elcapitan
none
Archive of layout-test-results from ews104 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews112 for mac-elcapitan
none
Patch none

Description Simon Pieters (:zcorpan) 2017-08-29 00:08:15 PDT
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
Comment 1 Sam Weinig 2017-08-29 20:43:57 PDT
Created attachment 319327 [details]
WIP
Comment 2 Sam Weinig 2017-08-29 20:45:33 PDT
Created attachment 319329 [details]
Patch
Comment 3 Build Bot 2017-08-29 21:54:36 PDT Comment hidden (obsolete)
Comment 4 Build Bot 2017-08-29 21:54:37 PDT Comment hidden (obsolete)
Comment 5 Build Bot 2017-08-29 21:59:24 PDT Comment hidden (obsolete)
Comment 6 Build Bot 2017-08-29 21:59:25 PDT Comment hidden (obsolete)
Comment 7 Build Bot 2017-08-29 22:07:54 PDT Comment hidden (obsolete)
Comment 8 Build Bot 2017-08-29 22:07:56 PDT Comment hidden (obsolete)
Comment 9 Build Bot 2017-08-29 22:24:06 PDT Comment hidden (obsolete)
Comment 10 Build Bot 2017-08-29 22:24:07 PDT Comment hidden (obsolete)
Comment 11 Sam Weinig 2017-08-30 13:03:25 PDT Comment hidden (obsolete)
Comment 12 Build Bot 2017-08-30 14:12:27 PDT Comment hidden (obsolete)
Comment 13 Build Bot 2017-08-30 14:12:28 PDT Comment hidden (obsolete)
Comment 14 Build Bot 2017-08-30 14:17:48 PDT Comment hidden (obsolete)
Comment 15 Build Bot 2017-08-30 14:17:50 PDT Comment hidden (obsolete)
Comment 16 Build Bot 2017-08-30 14:29:08 PDT Comment hidden (obsolete)
Comment 17 Build Bot 2017-08-30 14:29:09 PDT Comment hidden (obsolete)
Comment 18 Build Bot 2017-08-30 14:41:23 PDT Comment hidden (obsolete)
Comment 19 Build Bot 2017-08-30 14:41:24 PDT Comment hidden (obsolete)
Comment 20 Sam Weinig 2017-08-30 19:51:33 PDT Comment hidden (obsolete)
Comment 21 Sam Weinig 2017-08-30 20:41:35 PDT
Created attachment 319439 [details]
Patch
Comment 22 Devin Rousso 2017-08-30 23:13:05 PDT
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.
Comment 23 Simon Pieters (:zcorpan) 2017-08-31 06:11:24 PDT
Having two validateAndFixup functions is perfectly conforming, so long as the end result is equivalent.
Comment 24 Chris Dumez 2017-08-31 09:26:45 PDT
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?
Comment 25 Devin Rousso 2017-08-31 09:46:27 PDT
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."
Comment 26 Sam Weinig 2017-08-31 09:58:52 PDT
(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."
Comment 27 Sam Weinig 2017-08-31 10:23:25 PDT
Created attachment 319473 [details]
Patch
Comment 28 Devin Rousso 2017-08-31 10:24:37 PDT
Comment on attachment 319473 [details]
Patch

r=me
Comment 29 Sam Weinig 2017-08-31 10:36:29 PDT
Created attachment 319475 [details]
Patch
Comment 30 Build Bot 2017-08-31 11:30:54 PDT
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
Comment 31 Build Bot 2017-08-31 11:30:56 PDT
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
Comment 32 Build Bot 2017-08-31 11:40:03 PDT
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
Comment 33 Build Bot 2017-08-31 11:40:05 PDT
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
Comment 34 Build Bot 2017-08-31 11:47:51 PDT
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
Comment 35 Build Bot 2017-08-31 11:47:52 PDT
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
Comment 36 Sam Weinig 2017-08-31 11:50:37 PDT
Created attachment 319492 [details]
Patch
Comment 37 Simon Fraser (smfr) 2017-08-31 16:13:07 PDT
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?
Comment 38 Sam Weinig 2017-08-31 16:29:00 PDT
(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.
Comment 39 WebKit Commit Bot 2017-08-31 17:22:03 PDT
Comment on attachment 319492 [details]
Patch

Clearing flags on attachment: 319492

Committed r221462: <http://trac.webkit.org/changeset/221462>
Comment 40 WebKit Commit Bot 2017-08-31 17:22:05 PDT
All reviewed patches have been landed.  Closing bug.
Comment 41 Radar WebKit Bug Importer 2017-09-27 12:49:11 PDT
<rdar://problem/34694074>