[webkit-reviews] review granted: [Bug 176048] Implement DOMMatrix2DInit for setTransform()/addPath() : [Attachment 319492] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Sam Weinig
<sam at webkit.org>'s request for review:
Bug 176048: Implement DOMMatrix2DInit for setTransform()/addPath()
https://bugs.webkit.org/show_bug.cgi?id=176048

Attachment 319492: Patch

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




--- Comment #37 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 319492
  --> https://bugs.webkit.org/attachment.cgi?id=319492
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?