Bug 196086 – Remove the SVG tear off objects for SVGMatrix, SVGTransfrom, SVGTransformList and SVGAnimatedTransformList

• Component: SVG
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

This is part of removing the tear off objects of the SVG properties.

[Radar WebKit Bug Importer]

<rdar://problem/49122913>

[Said Abou-Hallawa]

Created attachment 365788 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 365788 [details]
Patch

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

> Source/WebCore/ChangeLog:22
> +            I think there was a misconception about these functions.
> +
> +            The specs link is 
> +                https://www.w3.org/TR/SVG11/coords.html#InterfaceSVGMatrix.
> +
> +            Notice that the specs does not state that the multiply() method should
> +            raise the exception NO_MODIFICATION_ALLOWED_ERR if the object is read
> +            only. Reading through the specs, it is clear that these methods should
> +            not affect the object but rather create new objects for the resulting
> +            matrix.

That's a pretty fundamental behavior change, and I think we should do it in a separate patch. I'm amazed that the SVG spec is vague enough to not make it clear what the expected behavior is, and that SVG tests don't exercise the behavior.

[Dean Jackson]

Comment on attachment 365788 [details]
Patch

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

>> Source/WebCore/ChangeLog:22
>> +            matrix.
> 
> That's a pretty fundamental behavior change, and I think we should do it in a separate patch. I'm amazed that the SVG spec is vague enough to not make it clear what the expected behavior is, and that SVG tests don't exercise the behavior.

Definitely should be a separate patch. I'd also like to know if this has any affect on d3 - do they use SVGMatrix?

[Said Abou-Hallawa]

Comment on attachment 365788 [details]
Patch

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

>>> Source/WebCore/ChangeLog:22
>>> +            matrix.
>> 
>> That's a pretty fundamental behavior change, and I think we should do it in a separate patch. I'm amazed that the SVG spec is vague enough to not make it clear what the expected behavior is, and that SVG tests don't exercise the behavior.
> 
> Definitely should be a separate patch. I'd also like to know if this has any affect on d3 - do they use SVGMatrix?

https://bugs.webkit.org/show_bug.cgi?id=196263

[Simon Fraser (smfr)]

Comment on attachment 365788 [details]
Patch

This needs to be extracted from the SVGMatrix patch.

[Said Abou-Hallawa]

Created attachment 366429 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 366429 [details]
Patch

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

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:20571
> +		725AB46F225295E700C73CF5 /* Recovered References */ = {
> +			isa = PBXGroup;
> +			children = (
> +				08CA3D4312894A3800FFF260 /* SVGMatrixTearOff.h */,
> +			);
> +			name = "Recovered References";
> +			sourceTree = "<group>";
> +		};

I don't think you want this project change.

> Source/WebCore/svg/SVGTransformValue.h:117
> +    FloatPoint translate() const

I wish this were called "translation"

[Said Abou-Hallawa]

Created attachment 366449 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 366449 [details]
Patch

Clearing flags on attachment: 366449

Committed r243730: <https://trac.webkit.org/changeset/243730>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.