Bug 191417 - [SVG2]: Use DOMMatrix2DInit for setMatrix and createSVGTransformFromMatrix
Summary: [SVG2]: Use DOMMatrix2DInit for setMatrix and createSVGTransformFromMatrix
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dirk Schulze
URL:
Keywords: InRadar
Depends on:
Blocks: 191292 200143
  Show dependency treegraph
 
Reported: 2018-11-08 01:24 PST by Dirk Schulze
Modified: 2019-10-22 02:44 PDT (History)
14 users (show)

See Also:


Attachments
Patch (19.63 KB, patch)
2018-11-08 01:31 PST, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch for landing (17.89 KB, patch)
2019-10-21 23:58 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch for landing (20.54 KB, patch)
2019-10-22 01:57 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch for landing (20.58 KB, patch)
2019-10-22 01:59 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Schulze 2018-11-08 01:24:16 PST
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.
Comment 1 Dirk Schulze 2018-11-08 01:31:25 PST
Created attachment 354220 [details]
Patch
Comment 2 Said Abou-Hallawa 2018-11-08 10:03:41 PST
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.
Comment 3 Dean Jackson 2018-11-08 12:28:46 PST
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.
Comment 4 Dirk Schulze 2019-10-21 23:58:21 PDT
Created attachment 381515 [details]
Patch for landing
Comment 5 Dirk Schulze 2019-10-22 01:57:16 PDT
Created attachment 381531 [details]
Patch for landing
Comment 6 Dirk Schulze 2019-10-22 01:59:36 PDT
Created attachment 381532 [details]
Patch for landing
Comment 7 WebKit Commit Bot 2019-10-22 02:43:55 PDT
Comment on attachment 381532 [details]
Patch for landing

Clearing flags on attachment: 381532

Committed r251427: <https://trac.webkit.org/changeset/251427>
Comment 8 WebKit Commit Bot 2019-10-22 02:43:57 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2019-10-22 02:44:38 PDT
<rdar://problem/56494093>