Bug 196086

Summary: Remove the SVG tear off objects for SVGMatrix, SVGTransfrom, SVGTransformList and SVGAnimatedTransformList
Product: WebKit Reporter: Said Abou-Hallawa <sabouhallawa>
Component: SVGAssignee: Said Abou-Hallawa <sabouhallawa>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, dino, jonlee, rniwa, simon.fraser, thorton, webkit-bug-importer, zimmermann
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 196263    
Bug Blocks: 191237    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Said Abou-Hallawa 2019-03-21 10:19:48 PDT
This is part of removing the tear off objects of the SVG properties.
Comment 1 Radar WebKit Bug Importer 2019-03-21 14:01:17 PDT
<rdar://problem/49122913>
Comment 2 Said Abou-Hallawa 2019-03-22 18:20:19 PDT
Created attachment 365788 [details]
Patch
Comment 3 Simon Fraser (smfr) 2019-03-25 12:48:24 PDT
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.
Comment 4 Dean Jackson 2019-03-25 12:49:45 PDT
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?
Comment 5 Said Abou-Hallawa 2019-03-26 11:45:05 PDT
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
Comment 6 Simon Fraser (smfr) 2019-04-01 11:06:48 PDT
Comment on attachment 365788 [details]
Patch

This needs to be extracted from the SVGMatrix patch.
Comment 7 Said Abou-Hallawa 2019-04-01 15:15:35 PDT
Created attachment 366429 [details]
Patch
Comment 8 Simon Fraser (smfr) 2019-04-01 15:37:34 PDT
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"
Comment 9 Said Abou-Hallawa 2019-04-01 17:31:45 PDT
Created attachment 366449 [details]
Patch
Comment 10 WebKit Commit Bot 2019-04-01 18:32:01 PDT
Comment on attachment 366449 [details]
Patch

Clearing flags on attachment: 366449

Committed r243730: <https://trac.webkit.org/changeset/243730>
Comment 11 WebKit Commit Bot 2019-04-01 18:32:03 PDT
All reviewed patches have been landed.  Closing bug.