Bug 196086 - Remove the SVG tear off objects for SVGMatrix, SVGTransfrom, SVGTransformList and SVGAnimatedTransformList
Summary: Remove the SVG tear off objects for SVGMatrix, SVGTransfrom, SVGTransformList...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on: 196263
Blocks: 191237
  Show dependency treegraph
 
Reported: 2019-03-21 10:19 PDT by Said Abou-Hallawa
Modified: 2019-04-01 18:32 PDT (History)
8 users (show)

See Also:


Attachments
Patch (150.66 KB, patch)
2019-03-22 18:20 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (204.86 KB, patch)
2019-04-01 15:15 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (213.88 KB, patch)
2019-04-01 17:31 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.