86004 2012-05-09 10:19:48 -0700 Cleanup SVGElement.cpp 2012-05-09 18:09:17 -0700 1 1 1 Unclassified WebKit SVG 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 rwlbuis rwlbuis eric zimmermann oldest_to_newest 619615 0 rwlbuis 2012-05-09 10:19:48 -0700 SVGElement.cpp has potential for cleanup since a lot of code moved in and out over time. 619624 1 140970 rwlbuis 2012-05-09 10:25:43 -0700 Created attachment 140970 Patch 619831 2 140970 eric 2012-05-09 14:28:14 -0700 Comment on attachment 140970 Patch OK. 619833 3 140970 eric 2012-05-09 14:29:15 -0700 Comment on attachment 140970 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140970&action=review > Source/WebCore/svg/svgattrs.in:-198 > -style This has a null namespace and prefix, correct? There is no differnece between this and HTMLNames::styleAttr? 619837 4 eric 2012-05-09 14:30:18 -0700 It appears there is! DEFINE_GLOBAL(QualifiedName, styleAttr, nullAtom, "style", xhtmlNamespaceURI) I'm not sure if that's intentional or not. 619843 5 140970 eric 2012-05-09 14:36:42 -0700 Comment on attachment 140970 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140970&action=review > Source/WebCore/svg/SVGElement.cpp:434 > - if (attr->name() != HTMLNames::styleAttr) > + if (attr->name() != styleAttr) What happens when we end up in a SVG document and the style attribute is in the SVG namespace? Seems this will do the wrong thing. Seems we want to just compare hte localName() here. Unless attr->name() is already just an AtomicString insetad of a QualifiedName? > Source/WebCore/svg/SVGElement.cpp:530 > DEFINE_STATIC_LOCAL(HashSet<QualifiedName>, animatableAttributes, ()); I suspect this doesn't work right either. At least not for the class attribute, which will be in a differnet namespace than expected? 619847 6 140970 eric 2012-05-09 14:39:59 -0700 Comment on attachment 140970 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140970&action=review >> Source/WebCore/svg/SVGElement.cpp:434 >> + if (attr->name() != styleAttr) > > What happens when we end up in a SVG document and the style attribute is in the SVG namespace? Seems this will do the wrong thing. Seems we want to just compare hte localName() here. > > Unless attr->name() is already just an AtomicString insetad of a QualifiedName? IT's a QualifiedName. :( http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Attribute.h I believe this has a noticable behavioral difference when the attribute is explcitly created with the sVG namespace. Which is possible manually, as well as by having an xml file or a .svg file, I think. > Source/WebCore/svg/SVGElement.cpp:533 > + animatableAttributes.add(classAttr); Similarly, I believe this code is observable in the same manner. We won't treat "class" attributes in the SVG namespace as animatable. I'm not sure if we should or not. 619850 7 eric 2012-05-09 14:40:21 -0700 You're not changing the code, I understand,b ut I believe you may be stumbling on two bugs here. 619857 8 rwlbuis 2012-05-09 14:45:33 -0700 I need to think about this :) Seems tricky stuff. Slightly related, what would happen with this <rect style="fill:red;" svg:style="green">. But I think this is more of a XML question... 620130 9 rwlbuis 2012-05-09 18:09:17 -0700 The style attribute change was not committed, this needs more investigation. Fixed in r 116569. 140970 2012-05-09 10:25:43 -0700 2012-05-09 14:39:59 -0700 Patch bug-86004-20120509132542.patch text/plain 3668 rwlbuis U3VidmVyc2lvbiBSZXZpc2lvbjogMTE2NDU1CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggOTQxMGJkYjlhYjYyNzQ2 NGViYWZmM2M4ZmVmM2YzYjAwN2FlNTczOS4uMTc2NDJlZTRiNzlkMTQ5OWJlZDU3YTFlZTJiYjc4 ZDgxMmI4NWI0NyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE5IEBACisyMDEyLTA1LTA5ICBSb2Ig QnVpcyAgPHJidWlzQHJpbS5jb20+CisKKyAgICAgICAgQ2xlYW51cCBTVkdFbGVtZW50LmNwcAor ICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9ODYwMDQKKwor ICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBSZW1vdmUgdW5u ZWVkZWQgaW5jbHVkZXMuIEFsc28gcmVtb3ZlIFNWR05hbWVzOjpzdHlsZSwganVzdCB1c2UgSFRN TE5hbWVzOjpzdHlsZS4KKyAgICAgICAgV2UgZG8gbm90IG5lZWQgdG8gY2hlY2sgYXR0ciBpbiBT VkdFbGVtZW50OjphdHRyaWJ1dGVDaGFuZ2VkLCBsb3dlciBsYXllcnMgYXNzdW1lCisgICAgICAg IGl0IGlzIG5vbi1udWxsIGFuZCB3ZSBkbyBub3QgY2FsbCBhdHRyaWJ1dGVDaGFuZ2VkIGluIFNW Ry4KKworICAgICAgICAqIHN2Zy9TVkdFbGVtZW50LmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OlNW R0VsZW1lbnQ6OmF0dHJpYnV0ZUNoYW5nZWQpOgorICAgICAgICAoV2ViQ29yZTo6U1ZHRWxlbWVu dDo6aXNBbmltYXRhYmxlQXR0cmlidXRlKToKKyAgICAgICAgKiBzdmcvc3ZnYXR0cnMuaW46CisK IDIwMTItMDUtMDggIFJhcGhhZWwgS3VibyBkYSBDb3N0YSAgPHJha3Vjb0B3ZWJraXQub3JnPgog CiAgICAgICAgIFtDTWFrZV0gUmV3cml0ZSBGaW5kR1N0cmVhbWVyLmNtYWtlLgpkaWZmIC0tZ2l0 IGEvU291cmNlL1dlYkNvcmUvc3ZnL1NWR0VsZW1lbnQuY3BwIGIvU291cmNlL1dlYkNvcmUvc3Zn L1NWR0VsZW1lbnQuY3BwCmluZGV4IDYyOGVhNWEwMjhkOWJlNGUzYzYzYTVhZmY0NGUwODIyZmEy NTQyODguLjE2YzYxNzExNjVhNDIwNzZhY2I5NmU0NWUxNjE3OWEwZjRmMjBlZjIgMTAwNjQ0Ci0t LSBhL1NvdXJjZS9XZWJDb3JlL3N2Zy9TVkdFbGVtZW50LmNwcAorKysgYi9Tb3VyY2UvV2ViQ29y ZS9zdmcvU1ZHRWxlbWVudC5jcHAKQEAgLTI2LDE5ICsyNiwxMyBAQAogI2lmIEVOQUJMRShTVkcp CiAjaW5jbHVkZSAiU1ZHRWxlbWVudC5oIgogCi0jaW5jbHVkZSAiQXR0cmlidXRlLmgiCiAjaW5j bHVkZSAiQ1NTQ3Vyc29ySW1hZ2VWYWx1ZS5oIgogI2luY2x1ZGUgIkRPTUltcGxlbWVudGF0aW9u LmgiCiAjaW5jbHVkZSAiRG9jdW1lbnQuaCIKICNpbmNsdWRlICJFdmVudC5oIgotI2luY2x1ZGUg IkV2ZW50TGlzdGVuZXIuaCIKLSNpbmNsdWRlICJFdmVudE5hbWVzLmgiCi0jaW5jbHVkZSAiRnJh bWVWaWV3LmgiCiAjaW5jbHVkZSAiSFRNTE5hbWVzLmgiCiAjaW5jbHVkZSAiTm9kZVJlbmRlcmlu Z0NvbnRleHQuaCIKLSNpbmNsdWRlICJSZWdpc3RlcmVkRXZlbnRMaXN0ZW5lci5oIgogI2luY2x1 ZGUgIlJlbmRlck9iamVjdC5oIgotI2luY2x1ZGUgIlNoYWRvd1Jvb3QuaCIKICNpbmNsdWRlICJT VkdDdXJzb3JFbGVtZW50LmgiCiAjaW5jbHVkZSAiU1ZHRG9jdW1lbnRFeHRlbnNpb25zLmgiCiAj aW5jbHVkZSAiU1ZHRWxlbWVudEluc3RhbmNlLmgiCkBAIC00NywxMCArNDEsNyBAQAogI2luY2x1 ZGUgIlNWR1NWR0VsZW1lbnQuaCIKICNpbmNsdWRlICJTVkdTdHlsZWRMb2NhdGFibGVFbGVtZW50 LmgiCiAjaW5jbHVkZSAiU1ZHVGV4dEVsZW1lbnQuaCIKLSNpbmNsdWRlICJTVkdVUklSZWZlcmVu Y2UuaCIKLSNpbmNsdWRlICJTVkdVc2VFbGVtZW50LmgiCiAjaW5jbHVkZSAiU2NyaXB0RXZlbnRM aXN0ZW5lci5oIgotI2luY2x1ZGUgIlN0eWxlUmVzb2x2ZXIuaCIKICNpbmNsdWRlICJYTUxOYW1l cy5oIgogCiBuYW1lc3BhY2UgV2ViQ29yZSB7CkBAIC00MjMsOSArNDE0LDYgQEAgYm9vbCBTVkdF bGVtZW50OjpjaGlsZFNob3VsZENyZWF0ZVJlbmRlcmVyKGNvbnN0IE5vZGVSZW5kZXJpbmdDb250 ZXh0JiBjaGlsZENvbnQKIHZvaWQgU1ZHRWxlbWVudDo6YXR0cmlidXRlQ2hhbmdlZChBdHRyaWJ1 dGUqIGF0dHIpCiB7CiAgICAgQVNTRVJUKGF0dHIpOwotICAgIGlmICghYXR0cikKLSAgICAgICAg cmV0dXJuOwotCiAgICAgU3R5bGVkRWxlbWVudDo6YXR0cmlidXRlQ2hhbmdlZChhdHRyKTsKIAog ICAgIC8vIFdoZW4gYW4gYW5pbWF0ZWQgU1ZHIHByb3BlcnR5IGNoYW5nZXMgdGhyb3VnaCBTVkcg RE9NLCBzdmdBdHRyaWJ1dGVDaGFuZ2VkKCkgaXMgY2FsbGVkLCBub3QgYXR0cmlidXRlQ2hhbmdl ZCgpLgpAQCAtNDQzLDcgKzQzMSw3IEBAIHZvaWQgU1ZHRWxlbWVudDo6YXR0cmlidXRlQ2hhbmdl ZChBdHRyaWJ1dGUqIGF0dHIpCiAKICAgICAvLyBDaGFuZ2VzIHRvIHRoZSBzdHlsZSBhdHRyaWJ1 dGUgYXJlIHByb2Nlc3NlZCBsYXppbHkgKHNlZSBFbGVtZW50OjpnZXRBdHRyaWJ1dGUoKSBhbmQg cmVsYXRlZCBtZXRob2RzKSwKICAgICAvLyBzbyB3ZSBkb24ndCB3YW50IGNoYW5nZXMgdG8gdGhl IHN0eWxlIGF0dHJpYnV0ZSB0byByZXN1bHQgaW4gZXh0cmEgd29yayBoZXJlLgotICAgIGlmIChh dHRyLT5uYW1lKCkgIT0gSFRNTE5hbWVzOjpzdHlsZUF0dHIpCisgICAgaWYgKGF0dHItPm5hbWUo KSAhPSBzdHlsZUF0dHIpCiAgICAgICAgIHN2Z0F0dHJpYnV0ZUNoYW5nZWQoYXR0ci0+bmFtZSgp KTsKIH0KIApAQCAtNTQyLDcgKzUzMCw3IEBAIGJvb2wgU1ZHRWxlbWVudDo6aXNBbmltYXRhYmxl QXR0cmlidXRlKGNvbnN0IFF1YWxpZmllZE5hbWUmIG5hbWUpCiAgICAgREVGSU5FX1NUQVRJQ19M T0NBTChIYXNoU2V0PFF1YWxpZmllZE5hbWU+LCBhbmltYXRhYmxlQXR0cmlidXRlcywgKCkpOwog CiAgICAgaWYgKGFuaW1hdGFibGVBdHRyaWJ1dGVzLmlzRW1wdHkoKSkgewotICAgICAgICBhbmlt YXRhYmxlQXR0cmlidXRlcy5hZGQoSFRNTE5hbWVzOjpjbGFzc0F0dHIpOworICAgICAgICBhbmlt YXRhYmxlQXR0cmlidXRlcy5hZGQoY2xhc3NBdHRyKTsKICAgICAgICAgYW5pbWF0YWJsZUF0dHJp YnV0ZXMuYWRkKFhMaW5rTmFtZXM6OmhyZWZBdHRyKTsKICAgICAgICAgYW5pbWF0YWJsZUF0dHJp YnV0ZXMuYWRkKFNWR05hbWVzOjphbXBsaXR1ZGVBdHRyKTsKICAgICAgICAgYW5pbWF0YWJsZUF0 dHJpYnV0ZXMuYWRkKFNWR05hbWVzOjphemltdXRoQXR0cik7CmRpZmYgLS1naXQgYS9Tb3VyY2Uv V2ViQ29yZS9zdmcvc3ZnYXR0cnMuaW4gYi9Tb3VyY2UvV2ViQ29yZS9zdmcvc3ZnYXR0cnMuaW4K aW5kZXggYzIyMGI2N2I4YzM1NDNjNmZlMThiODZiMDVmNjUyZTQ1Yjg2ZTNhNi4uOGJjYzgxZjlj NzE0NWMyMzJkZjVlZjYyMWU4OTFlMTgxZWUzMDA2YyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNv cmUvc3ZnL3N2Z2F0dHJzLmluCisrKyBiL1NvdXJjZS9XZWJDb3JlL3N2Zy9zdmdhdHRycy5pbgpA QCAtMTk1LDcgKzE5NSw2IEBAIHN0cm9rZS1saW5lam9pbgogc3Ryb2tlLW1pdGVybGltaXQKIHN0 cm9rZS1vcGFjaXR5CiBzdHJva2Utd2lkdGgKLXN0eWxlCiBzdXJmYWNlU2NhbGUKIHN5c3RlbUxh bmd1YWdlCiB0YWJsZVZhbHVlcwo=