SVGMatrix.IDL methods do not conform to the specs

[Said Abou-Hallawa]

The SVG 1.1 specs link is: https://www.w3.org/TR/SVG11/coords.html#InterfaceSVGMatrix.

The specs of the methods is a little bit vague. For example the multiply method does not clearly state whether the operation should change the SVGMatrix object i.e. "thisMatrix *= secondMatrix; return thisMatrix;". Or it is read only operation i.e., "return thisMatrix * secondMatrix".

But you can notice in the specs that SVGMatrix methods do not raise the exception NO_MODIFICATION_ALLOWED_ERR if the object is read only. Notice also setting the attribute 'a' for example may raise this exception. Therefore, I think the specs wanted to make these operations read-only. None of the methods should raise the exception NO_MODIFICATION_ALLOWED_ERR.

[Said Abou-Hallawa]

Created attachment 365978 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/49283958>

[Simon Fraser (smfr)]

Comment on attachment 365978 [details]
Patch

This needs a test (preferably a WPT so that we can share with other browsers).

[Said Abou-Hallawa]

Created attachment 366020 [details]
Patch

[Said Abou-Hallawa]

Created attachment 366022 [details]
Patch

[Said Abou-Hallawa]

(In reply to Simon Fraser (smfr) from comment #3)
> Comment on attachment 365978 [details]
> Patch
> 
> This needs a test (preferably a WPT so that we can share with other
> browsers).

LayoutTests/svg/dom/SVGMatrix.html tests only the cases where invalid arguments are passed to the SVGMatrix methods. I cleaned the test and added few cases where valid arguments are passed.

Notice that the new test passes in WebKit with and without this patch. SVGMatrix had two problems:

-- The SVGMatrix.IDL was written incorrectly.
-- The SVGMatrix methods were calling commitChange() even though the underlying SVGMatrixValue was not changing.

[Simon Fraser (smfr)]

(In reply to Said Abou-Hallawa from comment #6)
> (In reply to Simon Fraser (smfr) from comment #3)
> > Comment on attachment 365978 [details]
> > Patch
> > 
> > This needs a test (preferably a WPT so that we can share with other
> > browsers).
> 
> LayoutTests/svg/dom/SVGMatrix.html tests only the cases where invalid
> arguments are passed to the SVGMatrix methods. I cleaned the test and added
> few cases where valid arguments are passed.
> 
> Notice that the new test passes in WebKit with and without this patch.
> SVGMatrix had two problems:
> 
> -- The SVGMatrix.IDL was written incorrectly.
> -- The SVGMatrix methods were calling commitChange() even though the
> underlying SVGMatrixValue was not changing.

Do we mate Chrome/FF behavior on this new test?

[Simon Fraser (smfr)]

^match

[Said Abou-Hallawa]

Chrome and FF almost match the WebKit behavior. You can see these differences if you open LayoutTests/svg/dom/SVGMatrix.html in Chrome or FireFox.

-- FireFox throws exceptions with all invalid attribute values or invalid arguments.
-- The exceptions' messages in both FF and Chrome are different from the WebKit messages.
-- The calculation for tan(90) in FF is different from WebKit and Chrome.

[Simon Fraser (smfr)]

Comment on attachment 366022 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=366022&action=review

> LayoutTests/svg/dom/SVGMatrix.html:110
> +shouldBeEqualToString("matrixToString(matrix.translate(10, 20))", "{ a: 2, b: 0, c: 0, d: 1, e: 20, f: 220}");
> +shouldBeEqualToString("matrixToString(matrix.scale(5))", "{ a: 10, b: 0, c: 0, d: 5, e: 0, f: 200}");
> +shouldBeEqualToString("matrixToString(matrix.scaleNonUniform(2, 3))", "{ a: 4, b: 0, c: 0, d: 3, e: 0, f: 200}");
> +shouldBeEqualToString("matrixToString(matrix.skewX(90))", "{ a: 2, b: 0, c: 32662478706390740, d: 1, e: 0, f: 200}");
> +shouldBeEqualToString("matrixToString(matrix.skewY(90))", "{ a: 2, b: 16331239353195370, c: 0, d: 1, e: 0, f: 200}");

Don't we need a test that's like:

let matrix = { a: 2, b: 0, c: 0, d: 1, e: 0, f: 200} // or whatever
matrix.scale(2)
shouldBeEqualToString("matrixToString(matrix, "{ a: 10, b: 0, c: 0, d: 5, e: 0, f: 200}");

[Said Abou-Hallawa]

Comment on attachment 366022 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=366022&action=review

>> LayoutTests/svg/dom/SVGMatrix.html:110
>> +shouldBeEqualToString("matrixToString(matrix.skewY(90))", "{ a: 2, b: 16331239353195370, c: 0, d: 1, e: 0, f: 200}");
> 
> Don't we need a test that's like:
> 
> let matrix = { a: 2, b: 0, c: 0, d: 1, e: 0, f: 200} // or whatever
> matrix.scale(2)
> shouldBeEqualToString("matrixToString(matrix, "{ a: 10, b: 0, c: 0, d: 5, e: 0, f: 200}");

Sorry but I do not understand your suggestion. Did you mean we need to confirm that matrix was not changed by the previous calls? If this is the case, the following statements already do that:

debug("");
debug("Check that the matrix is still containing the correct values");
shouldBeEqualToString("matrixToString(matrix)", "{ a: 2, b: 0, c: 0, d: 1, e: 0, f: 200}");

[Said Abou-Hallawa]

Created attachment 366410 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 366410 [details]
Patch

Clearing flags on attachment: 366410

Committed r243703: <https://trac.webkit.org/changeset/243703>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.