125670 2013-12-12 18:09:40 -0800 SVG animatable bindings are improperly being generated with "fastGetAttribute" 2013-12-13 17:50:13 -0800 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 118845 125527 1 bfulgham bfulgham bfulgham cdumez commit-queue darin haraken rniwa sam simon.fraser thorton oldest_to_newest 959431 0 bfulgham 2013-12-12 18:09:40 -0800 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. 959435 1 219139 bfulgham 2013-12-12 18:24:11 -0800 Created attachment 219139 Patch 959436 2 219139 cdumez 2013-12-12 18:58:53 -0800 Comment on attachment 219139 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. 959450 3 219139 darin 2013-12-12 20:31:57 -0800 Comment on attachment 219139 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. 959461 4 bfulgham 2013-12-12 21:08:25 -0800 (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. 959470 5 219151 bfulgham 2013-12-12 21:31:45 -0800 Created attachment 219151 Updated to take advantage of fixes in Bug 125527. 959719 6 219151 commit-queue 2013-12-13 17:50:09 -0800 Comment on attachment 219151 Updated to take advantage of fixes in Bug 125527. Clearing flags on attachment: 219151 Committed r160578: <http://trac.webkit.org/changeset/160578> 959720 7 commit-queue 2013-12-13 17:50:13 -0800 All reviewed patches have been landed. Closing bug. 219139 2013-12-12 18:24:11 -0800 2013-12-12 21:31:42 -0800 Patch bug-125670-20131212182406.patch text/plain 2779 bfulgham SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDE2MDUyMykKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDIxIEBACisyMDEzLTEyLTEyICBCcmVudCBG dWxnaGFtICA8YmZ1bGdoYW1AYXBwbGUuY29tPgorCisgICAgICAgIFNWRyBiaW5kaW5ncyBhcmUg aW1wcm9wZXJseSBiZWluZyBnZW5lcmF0ZWQgd2l0aCAiZmFzdEdldEF0dHJpYnV0ZSIKKyAgICAg ICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTEyNTY3MAorCisgICAg ICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIEEgYnVnIHdhcyBpbnRy b2R1Y2VkIGluIHIxNTI4NDUgdGhhdCBpbXByb3Blcmx5IGNhbGxlZCB0aGUKKyAgICAgICAgSXNT VkdBbmltYXRlZFR5cGUgdXNpbmcgdGhlICRhdHRyaWJ1dGUgaGFzaCwgcmF0aGVyIHRoYW4gdGhl CisgICAgICAgIGV4cGVjdGVkICRhdHRyaWJ1dGUtPnNpZ25hdHVyZS0+dHlwZWkuCisKKyAgICAg ICAgQWxzbywgdGhlIFNWR1NjcmlwdEVsZW1lbnQgJ3R5cGUnIGF0dHJpYnV0ZSBpcyBhbHNvCisg ICAgICAgIGFuaW1hdGFibGUsIGFuZCBzaG91bGQgbm90IHVzZSB0aGUgZmFzdEdldEF0dHJpYnV0 ZSBwYXRoLiAKKworICAgICAgICAqIGJpbmRpbmdzL3NjcmlwdHMvQ29kZUdlbmVyYXRvci5wbToK KyAgICAgICAgKEdldHRlckV4cHJlc3Npb24pOiBDbGVhbiB1cCBhdHRyaWJ1dGUgdHlwZSBjb25m dXNpb24sIGFuZAorICAgICAgICBjaGVjayBmb3IgY2FsbHMgb24gU1ZHU2NyaXB0RWxlbWVudCdz ICd0eXBlJyBhdHRyaWJ1dGUuCisKIDIwMTMtMTItMTIgIFRpbSBIb3J0b24gIDx0aW1vdGh5X2hv cnRvbkBhcHBsZS5jb20+CiAKICAgICAgICAgW3drMl0gSGFuZGxlIHBpbmNoLXRvLXpvb20gZ2Vz dHVyZQpJbmRleDogU291cmNlL1dlYkNvcmUvYmluZGluZ3Mvc2NyaXB0cy9Db2RlR2VuZXJhdG9y LnBtCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJDb3JlL2JpbmRpbmdzL3NjcmlwdHMvQ29kZUdl bmVyYXRvci5wbQkocmV2aXNpb24gMTYwNTA4KQorKysgU291cmNlL1dlYkNvcmUvYmluZGluZ3Mv c2NyaXB0cy9Db2RlR2VuZXJhdG9yLnBtCSh3b3JraW5nIGNvcHkpCkBAIC01NTUsMTQgKzU1NSwx NiBAQCBzdWIgR2V0dGVyRXhwcmVzc2lvbgogICAgICAgICByZXR1cm4gKCRnZW5lcmF0b3ItPldL X2xjZmlyc3QoJGdlbmVyYXRvci0+QXR0cmlidXRlTmFtZUZvckdldHRlckFuZFNldHRlcigkYXR0 cmlidXRlKSkpOwogICAgIH0KIAorICAgIG15ICRhdHRyaWJ1dGVUeXBlID0gJGF0dHJpYnV0ZS0+ c2lnbmF0dXJlLT50eXBlOworCiAgICAgbXkgJGZ1bmN0aW9uTmFtZTsKICAgICBpZiAoJGF0dHJp YnV0ZS0+c2lnbmF0dXJlLT5leHRlbmRlZEF0dHJpYnV0ZXMtPnsiVVJMIn0pIHsKICAgICAgICAg JGZ1bmN0aW9uTmFtZSA9ICJnZXRVUkxBdHRyaWJ1dGUiOwotICAgIH0gZWxzaWYgKCRhdHRyaWJ1 dGUtPnNpZ25hdHVyZS0+dHlwZSBlcSAiYm9vbGVhbiIpIHsKKyAgICB9IGVsc2lmICgkYXR0cmli dXRlVHlwZSBlcSAiYm9vbGVhbiIpIHsKICAgICAgICAgJGZ1bmN0aW9uTmFtZSA9ICJmYXN0SGFz QXR0cmlidXRlIjsKLSAgICB9IGVsc2lmICgkYXR0cmlidXRlLT5zaWduYXR1cmUtPnR5cGUgZXEg ImxvbmciKSB7CisgICAgfSBlbHNpZiAoJGF0dHJpYnV0ZVR5cGUgZXEgImxvbmciKSB7CiAgICAg ICAgICRmdW5jdGlvbk5hbWUgPSAiZ2V0SW50ZWdyYWxBdHRyaWJ1dGUiOwotICAgIH0gZWxzaWYg KCRhdHRyaWJ1dGUtPnNpZ25hdHVyZS0+dHlwZSBlcSAidW5zaWduZWQgbG9uZyIpIHsKKyAgICB9 IGVsc2lmICgkYXR0cmlidXRlVHlwZSBlcSAidW5zaWduZWQgbG9uZyIpIHsKICAgICAgICAgJGZ1 bmN0aW9uTmFtZSA9ICJnZXRVbnNpZ25lZEludGVncmFsQXR0cmlidXRlIjsKICAgICB9IGVsc2Ug ewogICAgICAgICBpZiAoJGNvbnRlbnRBdHRyaWJ1dGVOYW1lIGVxICJXZWJDb3JlOjpIVE1MTmFt ZXM6OmlkQXR0ciIpIHsKQEAgLTU3MSw3ICs1NzMsOSBAQCBzdWIgR2V0dGVyRXhwcmVzc2lvbgog ICAgICAgICB9IGVsc2lmICgkY29udGVudEF0dHJpYnV0ZU5hbWUgZXEgIldlYkNvcmU6OkhUTUxO YW1lczo6bmFtZUF0dHIiKSB7CiAgICAgICAgICAgICAkZnVuY3Rpb25OYW1lID0gImdldE5hbWVB dHRyaWJ1dGUiOwogICAgICAgICAgICAgJGNvbnRlbnRBdHRyaWJ1dGVOYW1lID0gIiI7Ci0gICAg ICAgIH0gZWxzaWYgKCRnZW5lcmF0b3ItPklzU1ZHQW5pbWF0ZWRUeXBlKCRhdHRyaWJ1dGUpKSB7 CisgICAgICAgIH0gZWxzaWYgKCRnZW5lcmF0b3ItPklzU1ZHQW5pbWF0ZWRUeXBlKCRhdHRyaWJ1 dGVUeXBlKSkgeworICAgICAgICAgICAgJGZ1bmN0aW9uTmFtZSA9ICJnZXRBdHRyaWJ1dGUiOwor ICAgICAgICB9IGVsc2lmICgoJGludGVyZmFjZU5hbWUgPX4gL1NWR1NjcmlwdEVsZW1lbnQvKSAm JiAoJGF0dHJpYnV0ZS0+c2lnbmF0dXJlLT5uYW1lID1+IC90eXBlLykpIHsKICAgICAgICAgICAg ICRmdW5jdGlvbk5hbWUgPSAiZ2V0QXR0cmlidXRlIjsKICAgICAgICAgfSBlbHNlIHsKICAgICAg ICAgICAgICRmdW5jdGlvbk5hbWUgPSAiZmFzdEdldEF0dHJpYnV0ZSI7Cg== 219151 2013-12-12 21:31:45 -0800 2013-12-13 17:50:09 -0800 Updated to take advantage of fixes in Bug 125527. bug-125670-20131212213137.patch text/plain 2430 bfulgham SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDE2MDUyMykKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE3IEBACisyMDEzLTEyLTEyICBCcmVudCBG dWxnaGFtICA8YmZ1bGdoYW1AYXBwbGUuY29tPgorCisgICAgICAgIFNWRyBiaW5kaW5ncyBhcmUg aW1wcm9wZXJseSBiZWluZyBnZW5lcmF0ZWQgd2l0aCAiZmFzdEdldEF0dHJpYnV0ZSIKKyAgICAg ICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTEyNTY3MAorCisgICAg ICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIEEgYnVnIHdhcyBpbnRy b2R1Y2VkIGluIHIxNTI4NDUgdGhhdCBpbXByb3Blcmx5IGNhbGxlZCB0aGUKKyAgICAgICAgSXNT VkdBbmltYXRlZFR5cGUgdXNpbmcgdGhlICRhdHRyaWJ1dGUgaGFzaCwgcmF0aGVyIHRoYW4gdGhl CisgICAgICAgIGV4cGVjdGVkICRhdHRyaWJ1dGUtPnNpZ25hdHVyZS0+dHlwZS4KKworICAgICAg ICAqIGJpbmRpbmdzL3NjcmlwdHMvQ29kZUdlbmVyYXRvci5wbToKKyAgICAgICAgKEdldHRlckV4 cHJlc3Npb24pOiBDbGVhbiB1cCBhdHRyaWJ1dGUgdHlwZSBjb25mdXNpb24uCisKIDIwMTMtMTIt MTIgIFRpbSBIb3J0b24gIDx0aW1vdGh5X2hvcnRvbkBhcHBsZS5jb20+CiAKICAgICAgICAgW3dr Ml0gSGFuZGxlIHBpbmNoLXRvLXpvb20gZ2VzdHVyZQpJbmRleDogU291cmNlL1dlYkNvcmUvYmlu ZGluZ3Mvc2NyaXB0cy9Db2RlR2VuZXJhdG9yLnBtCj09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJD b3JlL2JpbmRpbmdzL3NjcmlwdHMvQ29kZUdlbmVyYXRvci5wbQkocmV2aXNpb24gMTYwNTA4KQor KysgU291cmNlL1dlYkNvcmUvYmluZGluZ3Mvc2NyaXB0cy9Db2RlR2VuZXJhdG9yLnBtCSh3b3Jr aW5nIGNvcHkpCkBAIC01NTUsMTQgKzU1NSwxNiBAQCBzdWIgR2V0dGVyRXhwcmVzc2lvbgogICAg ICAgICByZXR1cm4gKCRnZW5lcmF0b3ItPldLX2xjZmlyc3QoJGdlbmVyYXRvci0+QXR0cmlidXRl TmFtZUZvckdldHRlckFuZFNldHRlcigkYXR0cmlidXRlKSkpOwogICAgIH0KIAorICAgIG15ICRh dHRyaWJ1dGVUeXBlID0gJGF0dHJpYnV0ZS0+c2lnbmF0dXJlLT50eXBlOworCiAgICAgbXkgJGZ1 bmN0aW9uTmFtZTsKICAgICBpZiAoJGF0dHJpYnV0ZS0+c2lnbmF0dXJlLT5leHRlbmRlZEF0dHJp YnV0ZXMtPnsiVVJMIn0pIHsKICAgICAgICAgJGZ1bmN0aW9uTmFtZSA9ICJnZXRVUkxBdHRyaWJ1 dGUiOwotICAgIH0gZWxzaWYgKCRhdHRyaWJ1dGUtPnNpZ25hdHVyZS0+dHlwZSBlcSAiYm9vbGVh biIpIHsKKyAgICB9IGVsc2lmICgkYXR0cmlidXRlVHlwZSBlcSAiYm9vbGVhbiIpIHsKICAgICAg ICAgJGZ1bmN0aW9uTmFtZSA9ICJmYXN0SGFzQXR0cmlidXRlIjsKLSAgICB9IGVsc2lmICgkYXR0 cmlidXRlLT5zaWduYXR1cmUtPnR5cGUgZXEgImxvbmciKSB7CisgICAgfSBlbHNpZiAoJGF0dHJp YnV0ZVR5cGUgZXEgImxvbmciKSB7CiAgICAgICAgICRmdW5jdGlvbk5hbWUgPSAiZ2V0SW50ZWdy YWxBdHRyaWJ1dGUiOwotICAgIH0gZWxzaWYgKCRhdHRyaWJ1dGUtPnNpZ25hdHVyZS0+dHlwZSBl cSAidW5zaWduZWQgbG9uZyIpIHsKKyAgICB9IGVsc2lmICgkYXR0cmlidXRlVHlwZSBlcSAidW5z aWduZWQgbG9uZyIpIHsKICAgICAgICAgJGZ1bmN0aW9uTmFtZSA9ICJnZXRVbnNpZ25lZEludGVn cmFsQXR0cmlidXRlIjsKICAgICB9IGVsc2UgewogICAgICAgICBpZiAoJGNvbnRlbnRBdHRyaWJ1 dGVOYW1lIGVxICJXZWJDb3JlOjpIVE1MTmFtZXM6OmlkQXR0ciIpIHsKQEAgLTU3MSw3ICs1NzMs NyBAQCBzdWIgR2V0dGVyRXhwcmVzc2lvbgogICAgICAgICB9IGVsc2lmICgkY29udGVudEF0dHJp YnV0ZU5hbWUgZXEgIldlYkNvcmU6OkhUTUxOYW1lczo6bmFtZUF0dHIiKSB7CiAgICAgICAgICAg ICAkZnVuY3Rpb25OYW1lID0gImdldE5hbWVBdHRyaWJ1dGUiOwogICAgICAgICAgICAgJGNvbnRl bnRBdHRyaWJ1dGVOYW1lID0gIiI7Ci0gICAgICAgIH0gZWxzaWYgKCRnZW5lcmF0b3ItPklzU1ZH QW5pbWF0ZWRUeXBlKCRhdHRyaWJ1dGUpKSB7CisgICAgICAgIH0gZWxzaWYgKCRnZW5lcmF0b3It PklzU1ZHQW5pbWF0ZWRUeXBlKCRhdHRyaWJ1dGVUeXBlKSkgewogICAgICAgICAgICAgJGZ1bmN0 aW9uTmFtZSA9ICJnZXRBdHRyaWJ1dGUiOwogICAgICAgICB9IGVsc2UgewogICAgICAgICAgICAg JGZ1bmN0aW9uTmFtZSA9ICJmYXN0R2V0QXR0cmlidXRlIjsK