Bug 49311

Summary: Convert SVGMatrix/SVGTransform/SVGTransformList to the new SVGPropertyTearOff concept
Product: WebKit Reporter: Nikolas Zimmermann <zimmermann>
Component: SVGAssignee: Nikolas Zimmermann <zimmermann>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, krit, mdelaney7, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 47905    
Attachments:
Description Flags
Patch
none
Patch v2 krit: review+

Description Nikolas Zimmermann 2010-11-10 02:15:42 PST
Unfortunately there was no way to split the patches up, w/o adding new "hybrid wrappers" that support an "old style" parent (SVGTransform) with a "new style" child (SVGMatrix)
SVGTransform contains "readonly attribute SVGMatrix matrix", so I had to switch them over to the new system in one go.
Unfortunately SVGTransformList/SVGAnimatedTransformList had to be converted as well, but those require a new SVGPropertyTearOff classes, that deals with the two extra methods that SVGTransformList provides (consolidate, createSVGTransformFromMatrix)

So the patch is a much larger than I hoped. Anyhow, after this patch there's only SVGPathSegList left to do! (And the cleanup patches removing the old JS/V8SVGPODTypeWrappers, and all the Deprecated* files).
Comment 1 Nikolas Zimmermann 2010-11-10 05:26:30 PST
Created attachment 73492 [details]
Patch
Comment 2 Eric Seidel (no email) 2010-11-10 06:37:02 PST
Attachment 73492 [details] did not build on mac:
Build output: http://queues.webkit.org/results/5506085
Comment 3 Eric Seidel (no email) 2010-11-10 06:53:49 PST
Attachment 73492 [details] did not build on mac:
Build output: http://queues.webkit.org/results/5438080
Comment 4 Nikolas Zimmermann 2010-11-10 12:09:18 PST
Created attachment 73522 [details]
Patch v2

Not sure why mac build failed, probably an unused variable. If it fails again, I'll try a release build.

This patch is significantly larger, as it contains several new testcases.
Comment 5 Eric Seidel (no email) 2010-11-10 13:26:20 PST
Attachment 73522 [details] did not build on mac:
Build output: http://queues.webkit.org/results/5613002
Comment 6 Eric Seidel (no email) 2010-11-10 13:37:53 PST
Attachment 73522 [details] did not build on mac:
Build output: http://queues.webkit.org/results/5501087
Comment 7 Nikolas Zimmermann 2010-11-11 00:14:00 PST
(In reply to comment #6)
> Attachment 73522 [details] did not build on mac:
> Build output: http://queues.webkit.org/results/5501087

The error is real, I tried a build from scratch, and it turns out I forgot to add SVGAnimatedTransformList.h to Xcode, no works fine. Not sure if it's worth uploading a new version just for that though
Comment 8 Dirk Schulze 2010-11-11 03:07:38 PST
Comment on attachment 73522 [details]
Patch v2

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

We already talked about some issues on IRC, just add some notes to bugzilla review as well. I may forgot to note some change wishes here :-) Niko already fixed Mac.

Great Patch Niko! Looking forward to see the same work on SVGPathSegList :-D

r=me

> WebCore/svg/SVGTransformList.cpp:66
> +    return String::format("matrix(%f %f %f %f %f %f)", matrix.a(), matrix.b(), matrix.c(), matrix.d(), matrix.e(), matrix.f());

Don't use String::format here ;-) We talked about this on IRC right now.

> WebCore/svg/SVGTransformable.cpp:192
> +        list.append(t);

Can you rename t to transform please? And please fix the indentation as well on switch .. case later in the code.
Comment 9 Nikolas Zimmermann 2010-11-11 03:19:18 PST
Fixed all issues, landing.
Comment 10 Nikolas Zimmermann 2010-11-11 03:21:16 PST
Committed r71802: <http://trac.webkit.org/changeset/71802>