Bug 191417 – [SVG2]: Use DOMMatrix2DInit for setMatrix and createSVGTransformFromMatrix

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Dirk Schulze]

Following previous changes that made SVG APIs use DOMPointInit (instead of SVGPoint, later DOMPointReadOnly), changing APIs that take SVGMatrix (DOMMatrixReadOnly) to use DOMMatrix2DInit.

Spec change is in review here: https://github.com/w3c/svgwg/issues/326

Affected: SVGTransform::setMatrix, SVGTransformList::createSVGTransformFromMatrix and SVGSVGElement::createSVGTransformFromMatrix.

[Dirk Schulze]

Created attachment 354220 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 354220 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        DOMMatrix, DOMMatrixReadOnly and SVGMatrix (alias of DOMMatrix).

Can you please add a link to the specs?

> 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.

> Source/WebCore/svg/SVGSVGElement.cpp:417
> +    AffineTransform transform;
> +    if (matrixInit.a.has_value())
> +        transform.setA(matrixInit.a.value());
> +    if (matrixInit.b.has_value())
> +        transform.setB(matrixInit.b.value());
> +    if (matrixInit.c.has_value())
> +        transform.setC(matrixInit.c.value());
> +    if (matrixInit.d.has_value())
> +        transform.setD(matrixInit.d.value());
> +    if (matrixInit.e.has_value())
> +        transform.setE(matrixInit.e.value());
> +    if (matrixInit.f.has_value())
> +        transform.setF(matrixInit.f.value());

Should we add a new constructor to AffineTransform which takes DOMMatrixReadOnly?

> Source/WebCore/svg/SVGSVGElement.cpp:418
> +    return SVGTransform::create(SVGTransformValue { transform });

If we add the AffineTransform constructor, I think this can be replaced by 

return SVGTransform::create(SVGTransformValue { matrix });

> Source/WebCore/svg/SVGTransform.h:66
> +    ExceptionOr<void> setMatrix(DOMMatrix2DInit&& matrixInit)

The specs says this function should take DOMMatrixReadOnly, https://www.w3.org/TR/SVG/coords.html#InterfaceSVGTransform.

> Source/WebCore/svg/SVGTransform.h:84
> +        AffineTransform transform;
> +        if (matrixInit.a.has_value())
> +            transform.setA(matrixInit.a.value());
> +        if (matrixInit.b.has_value())
> +            transform.setB(matrixInit.b.value());
> +        if (matrixInit.c.has_value())
> +            transform.setC(matrixInit.c.value());
> +        if (matrixInit.d.has_value())
> +            transform.setD(matrixInit.d.value());
> +        if (matrixInit.e.has_value())
> +            transform.setE(matrixInit.e.value());
> +        if (matrixInit.f.has_value())
> +            transform.setF(matrixInit.f.value());
> +        propertyReference().setMatrix(transform);

Should be a single line if the new AffineTransform constructor is added.

> Source/WebCore/svg/SVGTransform.idl:38
> +    [MayThrowException] void setMatrix(optional DOMMatrix2DInit matrix);

The specs says this function takes DOMMatrixReadOnly and it is not optional.

> Source/WebCore/svg/SVGTransformList.idl:38
> +    [MayThrowException, NewObject] SVGTransform createSVGTransformFromMatrix(optional DOMMatrix2DInit matrix);

Ditto.

[Dean Jackson]

Comment on attachment 354220 [details]
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.

[Dirk Schulze]

Created attachment 381515 [details]
Patch for landing

[Dirk Schulze]

Created attachment 381531 [details]
Patch for landing

[Dirk Schulze]

Created attachment 381532 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 381532 [details]
Patch for landing

Clearing flags on attachment: 381532

Committed r251427: <https://trac.webkit.org/changeset/251427>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/56494093>