Bug 48469

Summary: Convert SVGAnimatedBoolean to the new SVGAnimatedPropertyTearOff concept
Product: WebKit Reporter: Nikolas Zimmermann <zimmermann>
Component: SVGAssignee: Nikolas Zimmermann <zimmermann>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, eric, krit, mdelaney7, webkit-ews, webkit.review.bot, 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
none
Patch v3
none
Patch v4 krit: review+

Nikolas Zimmermann
Reported 2010-10-27 14:35:59 PDT
Converting SVGAnimatedBoolean is trivial, but it needs a new type: SVGAnimatedStaticPropertyTearOff, which is used for properties that don't create tear offs when accessing baseVal/animVal. For example with SVGLength you can do: var foo = rect.x.baseVal; // SVGRectElement -> SVGAnimatedLength -> SVGLength foo.value = 50; In this case a SVGAnimatedPropertyTearOff is created, and a SVGPropertyTearOff to represent the SVGLength. For SVGExternalResourcesRequired (one of the two SVGAnimatedBoolean types), this is not possible, as baseVal returns a POD type (boolean). var blub = rect.externalResourcesRequired.baseVal; // blub is a boolean blub = false; // does NOT effect the rect.externalResourcesRequired object The old concept used JSSVGStaticPODTypeWrapper for these types, we need something similar in the new concept, called SVGAnimatedStaticPropertyTearOff.
Attachments
Patch (59.50 KB, patch)
2010-10-27 15:30 PDT, Nikolas Zimmermann 		no flags
DetailsFormatted DiffDiff
Patch v2 (59.61 KB, patch)
2010-10-27 15:36 PDT, Nikolas Zimmermann 		no flags
DetailsFormatted DiffDiff
Patch v3 (62.55 KB, patch)
2010-10-28 03:48 PDT, Nikolas Zimmermann 		no flags
DetailsFormatted DiffDiff
Patch v4 (63.35 KB, patch)
2010-10-28 05:33 PDT, Nikolas Zimmermann 		krit: review+
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Nikolas Zimmermann
Comment 1 2010-10-27 15:30:18 PDT
Created attachment 72096 [details] Patch Did not test on a local chrome build, let's wait for EWS to see whether it builds. But the CodeGenerator changes are trivial this time, so the likelihood is large, that it may just work.
WebKit Review Bot
Comment 2 2010-10-27 15:34:16 PDT
Attachment 72096 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/svg/SVGScriptElement.h:26: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 52 files If any of these errors are false positives, please file a bug against check-webkit-style.
Nikolas Zimmermann
Comment 3 2010-10-27 15:36:47 PDT
Created attachment 72097 [details] Patch v2 Fix style issue.
Eric Seidel (no email)
Comment 4 2010-10-27 19:20:06 PDT
Attachment 72097 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4850045
WebKit Review Bot
Comment 5 2010-10-27 23:19:14 PDT
Attachment 72097 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4789056
Nikolas Zimmermann
Comment 6 2010-10-28 03:48:38 PDT
Created attachment 72167 [details] Patch v3 Need to apply the same fix as ObjC received to V8, to avoid including "V8boolean.h".
Early Warning System Bot
Comment 7 2010-10-28 04:19:57 PDT
Attachment 72167 [details] did not build on qt: Build output: http://queues.webkit.org/results/4837054
Nikolas Zimmermann
Comment 8 2010-10-28 05:33:18 PDT
Created attachment 72174 [details] Patch v4 Oops, fix some last minute changes that broke the build. Reattaching a working version, tested a local mac leopard debug build, zero regressions.
Eric Seidel (no email)
Comment 9 2010-10-28 06:09:13 PDT
Attachment 72167 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4846067
Eric Seidel (no email)
Comment 10 2010-10-28 06:27:01 PDT
Attachment 72167 [details] did not build on mac: Build output: http://queues.webkit.org/results/4800064
Dirk Schulze
Comment 11 2010-10-28 07:16:55 PDT
(In reply to comment #10) > Attachment 72167 [details] did not build on mac: > Build output: http://queues.webkit.org/results/4800064 But you build it without regressions on Mac ;-)
Dirk Schulze
Comment 12 2010-10-28 07:25:09 PDT
Comment on attachment 72174 [details] Patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=72174&action=review Looks great, waiting for mac and chromium bots before I review it. (If no one else beats me) > WebCore/svg/properties/SVGAnimatedProperty.h:-88 > - RefPtr<SVGProperty> m_baseVal; > - RefPtr<SVGProperty> m_animVal; I guess you moved this out, because animatedStaticPro doesn't need it.
Nikolas Zimmermann
Comment 13 2010-10-28 13:00:04 PDT
(In reply to comment #12) > (From update of attachment 72174 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=72174&action=review > > Looks great, waiting for mac and chromium bots before I review it. (If no one else beats me) > > > WebCore/svg/properties/SVGAnimatedProperty.h:-88 > > - RefPtr<SVGProperty> m_baseVal; > > - RefPtr<SVGProperty> m_animVal; > > I guess you moved this out, because animatedStaticPro doesn't need it. Yes, exactly - mac bot worked fine, and cr-mac as well.
Nikolas Zimmermann
Comment 14 2010-10-28 13:01:41 PDT
(In reply to comment #11) > (In reply to comment #10) > > Attachment 72167 [details] [details] did not build on mac: > > Build output: http://queues.webkit.org/results/4800064 > > But you build it without regressions on Mac ;-) Yes, if you look closer, that's the result from the last patch v3, not patch v4.
Dirk Schulze
Comment 15 2010-10-28 13:27:13 PDT
Comment on attachment 72174 [details] Patch v4 r=me
Nikolas Zimmermann
Comment 16 2010-10-29 02:55:16 PDT
Landed in r70857. Waiting for bots before closing bug.
Nikolas Zimmermann
Comment 17 2010-10-29 03:32:59 PDT
(In reply to comment #16) > Landed in r70857. Waiting for bots before closing bug. Forgot to remove an alert() from LayoutTests/svg/dom/SVGScriptElement/resources/script-load4.js before landing, fixed in r70860.
WebKit Review Bot
Comment 18 2010-10-29 03:43:37 PDT
http://trac.webkit.org/changeset/70857 might have broken Qt Linux Release
Note You need to log in before you can comment on or make changes to this bug.