Bug 86004 – Cleanup SVGElement.cpp

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

[Rob Buis]

SVGElement.cpp has potential for cleanup since a lot of code moved in and out over time.

[Rob Buis]

Created attachment 140970 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 140970 [details]
Patch

OK.

[Eric Seidel (no email)]

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?

[Eric Seidel (no email)]

It appears there is!

DEFINE_GLOBAL(QualifiedName, styleAttr, nullAtom, "style", xhtmlNamespaceURI)

I'm not sure if that's intentional or not.

[Eric Seidel (no email)]

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?

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

You're not changing the code, I understand,b ut I believe you may be stumbling on two bugs here.

[Rob Buis]

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...

[Rob Buis]

The style attribute change was not committed, this needs more investigation.

Fixed in r 116569.