Bug 86004 - Cleanup SVGElement.cpp
Summary: Cleanup SVGElement.cpp
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rob Buis
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-09 10:19 PDT by Rob Buis
Modified: 2012-05-09 18:09 PDT (History)
2 users (show)

See Also:


Attachments
Patch (3.58 KB, patch)
2012-05-09 10:25 PDT, Rob Buis
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rob Buis 2012-05-09 10:19:48 PDT
SVGElement.cpp has potential for cleanup since a lot of code moved in and out over time.
Comment 1 Rob Buis 2012-05-09 10:25:43 PDT
Created attachment 140970 [details]
Patch
Comment 2 Eric Seidel (no email) 2012-05-09 14:28:14 PDT
Comment on attachment 140970 [details]
Patch

OK.
Comment 3 Eric Seidel (no email) 2012-05-09 14:29:15 PDT
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?
Comment 4 Eric Seidel (no email) 2012-05-09 14:30:18 PDT
It appears there is!

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

I'm not sure if that's intentional or not.
Comment 5 Eric Seidel (no email) 2012-05-09 14:36:42 PDT
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 6 Eric Seidel (no email) 2012-05-09 14:39:59 PDT
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.
Comment 7 Eric Seidel (no email) 2012-05-09 14:40:21 PDT
You're not changing the code, I understand,b ut I believe you may be stumbling on two bugs here.
Comment 8 Rob Buis 2012-05-09 14:45:33 PDT
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...
Comment 9 Rob Buis 2012-05-09 18:09:17 PDT
The style attribute change was not committed, this needs more investigation.

Fixed in r 116569.