Bug 49311 - Convert SVGMatrix/SVGTransform/SVGTransformList to the new SVGPropertyTearOff concept
Summary: Convert SVGMatrix/SVGTransform/SVGTransformList to the new SVGPropertyTearOff...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
Depends on:
Blocks: 47905
  Show dependency treegraph
 
Reported: 2010-11-10 02:15 PST by Nikolas Zimmermann
Modified: 2010-11-11 03:21 PST (History)
4 users (show)

See Also:


Attachments
Patch (210.02 KB, patch)
2010-11-10 05:26 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch v2 (606.21 KB, patch)
2010-11-10 12:09 PST, Nikolas Zimmermann
krit: review+
Details | Formatted Diff | Diff

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