Bug 125670 – SVG animatable bindings are improperly being generated with "fastGetAttribute"

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Brent Fulgham]

The changes in Bug 118845 caused the 'fastGetAttribute' case to be called when animated attributes are involved.

> Source/WebCore/bindings/scripts/CodeGenerator.pm:575
> +        } elsif ($generator->IsSVGAnimatedType($attribute)) {

This is wrong. $attribute in this context is a hash, not a string that can be compared.  I think this test now always fails, causing us to use "fastGetAttribute" in some cases where it is not safe.

This should have been something like:

"my $type = $attribute->signature->type; 
if ($generator->IsSVGAniatedType($type)) {
... do stuff

I found this while correcting a second problem, where SVGScriptElementType was being used with "fastGetAttribute". This is also animatable, and should not go through that path.

[Brent Fulgham]

Created attachment 219139 [details]
Patch

[Chris Dumez]

Comment on attachment 219139 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGenerator.pm:576
> +        } elsif ($generator->IsSVGAnimatedType($attributeType)) {

This change looks good.

> Source/WebCore/bindings/scripts/CodeGenerator.pm:578
> +        } elsif (($interfaceName =~ /SVGScriptElement/) && ($attribute->signature->name =~ /type/)) {

I don't understand this one. The "type" attribute on SVGScriptElement is not animatable in the specification (svg 1.1 or 2):
http://www.w3.org/TR/SVG2/script.html#ScriptElementTypeAttribute

Also, the attribute type in our IDL is a simple DOMString, not a SVGAnimatedString.

[Darin Adler]

Comment on attachment 219139 [details]
Patch

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

>> Source/WebCore/bindings/scripts/CodeGenerator.pm:578
>> +        } elsif (($interfaceName =~ /SVGScriptElement/) && ($attribute->signature->name =~ /type/)) {
> 
> I don't understand this one. The "type" attribute on SVGScriptElement is not animatable in the specification (svg 1.1 or 2):
> http://www.w3.org/TR/SVG2/script.html#ScriptElementTypeAttribute
> 
> Also, the attribute type in our IDL is a simple DOMString, not a SVGAnimatedString.

Patch in bug 125527 corrects things so that SVGScriptElement’s type attribute is correctly identified as not animatable, so we should land that first, and omit this change.

[Brent Fulgham]

(In reply to comment #3)
> (From update of attachment 219139 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=219139&action=review
> 
> >> Source/WebCore/bindings/scripts/CodeGenerator.pm:578
> >> +        } elsif (($interfaceName =~ /SVGScriptElement/) && ($attribute->signature->name =~ /type/)) {
> > 
> > I don't understand this one. The "type" attribute on SVGScriptElement is not animatable in the specification (svg 1.1 or 2):
> > http://www.w3.org/TR/SVG2/script.html#ScriptElementTypeAttribute
> > 
> > Also, the attribute type in our IDL is a simple DOMString, not a SVGAnimatedString.
> 
> Patch in bug 125527 corrects things so that SVGScriptElement’s type attribute is correctly identified as not animatable, so we should land that first, and omit this change.

Ah! That makes great sense, I'll revise the patch and  wait for 125527 to land. Thanks for looking it over.

[Brent Fulgham]

Created attachment 219151 [details]
Updated to take advantage of fixes in Bug 125527.

[WebKit Commit Bot]

Comment on attachment 219151 [details]
Updated to take advantage of fixes in Bug 125527.

Clearing flags on attachment: 219151

Committed r160578: <http://trac.webkit.org/changeset/160578>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.