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
Created attachment 319327 [details] WIP
Created attachment 319329 [details] Patch
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
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
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
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
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
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
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
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
Created attachment 319390 [details] Patch
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
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
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
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
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
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
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
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
Created attachment 319436 [details] Patch
Created attachment 319439 [details] Patch
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.
Having two validateAndFixup functions is perfectly conforming, so long as the end result is equivalent.
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 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."
(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."
Created attachment 319473 [details] Patch
Comment on attachment 319473 [details] Patch r=me
Created attachment 319475 [details] Patch
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
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 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
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 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
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
Created attachment 319492 [details] Patch
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?
(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 on attachment 319492 [details] Patch Clearing flags on attachment: 319492 Committed r221462: <http://trac.webkit.org/changeset/221462>
All reviewed patches have been landed. Closing bug.
<rdar://problem/34694074>