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

[bugzilla-daemon at webkit.org]

Devin Rousso <webkit at devinrousso.com> has denied  review:
Bug 176048: Implement DOMMatrix2DInit for setTransform()/addPath()
https://bugs.webkit.org/show_bug.cgi?id=176048

Attachment 319439: Patch

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




--- Comment #25 from Devin Rousso <webkit at devinrousso.com> ---
Comment on attachment 319439
  --> https://bugs.webkit.org/attachment.cgi?id=319439
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."