[webkit-reviews] review granted: [Bug 191417] SVG2: Use DOMMatrix2DInit for setMatrix and createSVGTransformFromMatrix : [Attachment 354220] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Dirk Schulze <krit at webkit.org>'s
request for review:
Bug 191417: SVG2: Use DOMMatrix2DInit for setMatrix and
createSVGTransformFromMatrix
https://bugs.webkit.org/show_bug.cgi?id=191417

Attachment 354220: Patch

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




--- Comment #3 from Dean Jackson <dino at apple.com> ---
Comment on attachment 354220
  --> https://bugs.webkit.org/attachment.cgi?id=354220
Patch

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

> Source/WebCore/ChangeLog:9
> +	   With SVG 2.0, any DOMPoint2DInit type is supported which inlcudes
dictionaries,

Typo: includes

>> Source/WebCore/svg/SVGSVGElement.cpp:403
>> +Ref<SVGTransform>
SVGSVGElement::createSVGTransformFromMatrix(DOMMatrix2DInit&& matrixInit)
> 
> The specs says this function should take a DOMMatrixReadOnly object.
https://www.w3.org/TR/SVG/struct.html#InterfaceSVGSVGElement.

It should also accept a DOMMatrix2DInit as defined by the Geometry
specification (the init object for a DOMMatrixReadOnly). However I don't know
why we don't have a version that accepts a DOMMatrixReadOnly directly.

>> Source/WebCore/svg/SVGSVGElement.cpp:417
>> +	    transform.setF(matrixInit.f.value());
> 
> Should we add a new constructor to AffineTransform which takes
DOMMatrixReadOnly?

We shouldn't. AffineTransform is in platform.

> Source/WebCore/svg/SVGSVGElement.h:97
> +    static Ref<SVGTransform>
createSVGTransformFromMatrix(DOMMatrix2DInit&&);

Why don't we have a version that also accepts a DOMMatrixReadOnly?

> Source/WebCore/svg/SVGSVGElement.idl:75
> +    [NewObject] SVGTransform createSVGTransformFromMatrix(optional
DOMMatrix2DInit matrix);

I couldn't find this in the latest SVG 2 editor's draft.

>> Source/WebCore/svg/SVGTransform.idl:38
>> +	[MayThrowException] void setMatrix(optional DOMMatrix2DInit matrix);
> 
> The specs says this function takes DOMMatrixReadOnly and it is not optional.

Dirk is updating to the latest version.