SVGElement.cpp has potential for cleanup since a lot of code moved in and out over time.
Created attachment 140970 [details] Patch
Comment on attachment 140970 [details] Patch OK.
Comment on attachment 140970 [details] 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?
It appears there is! DEFINE_GLOBAL(QualifiedName, styleAttr, nullAtom, "style", xhtmlNamespaceURI) I'm not sure if that's intentional or not.
Comment on attachment 140970 [details] 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?
Comment on attachment 140970 [details] 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.
You're not changing the code, I understand,b ut I believe you may be stumbling on two bugs here.
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...
The style attribute change was not committed, this needs more investigation. Fixed in r 116569.