Summary: | Remove the SVG tear off objects for SVGMatrix, SVGTransfrom, SVGTransformList and SVGAnimatedTransformList | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Said Abou-Hallawa <sabouhallawa> | ||||||||
Component: | SVG | Assignee: | 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
Said Abou-Hallawa
2019-03-21 10:19:48 PDT
Created attachment 365788 [details]
Patch
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 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 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 on attachment 365788 [details]
Patch
This needs to be extracted from the SVGMatrix patch.
Created attachment 366429 [details]
Patch
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" Created attachment 366449 [details]
Patch
Comment on attachment 366449 [details] Patch Clearing flags on attachment: 366449 Committed r243730: <https://trac.webkit.org/changeset/243730> All reviewed patches have been landed. Closing bug. |