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
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
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
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 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
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
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
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 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 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
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
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 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.
2017-08-29 20:43 PDT, Sam Weinig
2017-08-29 20:45 PDT, Sam Weinig
2017-08-29 21:54 PDT, Build Bot
2017-08-29 21:59 PDT, Build Bot
2017-08-29 22:07 PDT, Build Bot
2017-08-29 22:24 PDT, Build Bot
2017-08-30 13:03 PDT, Sam Weinig
2017-08-30 14:12 PDT, Build Bot
2017-08-30 14:17 PDT, Build Bot
2017-08-30 14:29 PDT, Build Bot
2017-08-30 14:41 PDT, Build Bot
2017-08-30 19:51 PDT, Sam Weinig
2017-08-30 20:41 PDT, Sam Weinig
2017-08-31 10:23 PDT, Sam Weinig
2017-08-31 10:36 PDT, Sam Weinig
2017-08-31 11:30 PDT, Build Bot
2017-08-31 11:40 PDT, Build Bot
2017-08-31 11:47 PDT, Build Bot
2017-08-31 11:50 PDT, Sam Weinig