Bug 48469 – Convert SVGAnimatedBoolean to the new SVGAnimatedPropertyTearOff concept

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Nikolas Zimmermann]

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.

[Nikolas Zimmermann]

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]

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]

Created attachment 72097 [details]
Patch v2

Fix style issue.

[Eric Seidel (no email)]

Attachment 72097 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4850045

[WebKit Review Bot]

Attachment 72097 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4789056

[Nikolas Zimmermann]

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]

Attachment 72167 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4837054

[Nikolas Zimmermann]

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)]

Attachment 72167 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4846067

[Eric Seidel (no email)]

Attachment 72167 [details] did not build on mac:
Build output: http://queues.webkit.org/results/4800064

[Dirk Schulze]

(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 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]

(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]

(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 on attachment 72174 [details]
Patch v4

r=me

[Nikolas Zimmermann]

Landed in r70857. Waiting for bots before closing bug.

[Nikolas Zimmermann]

(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]

http://trac.webkit.org/changeset/70857 might have broken Qt Linux Release