[webkit-reviews] review granted: [Bug 53442] SVGAnimateElement needs information about the animated attribute type : [Attachment 81974] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Feb 10 09:05:21 PST 2011
Nikolas Zimmermann <zimmermann at kde.org> has granted Dirk Schulze
<krit at webkit.org>'s request for review:
Bug 53442: SVGAnimateElement needs information about the animated attribute
type
https://bugs.webkit.org/show_bug.cgi?id=53442
Attachment 81974: Patch
https://bugs.webkit.org/attachment.cgi?id=81974&action=review
------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=81974&action=review
r=me with some fixups:
> Source/WebCore/ChangeLog:9
> + attribute name and a type is not possible, since one attribute name
can be bounded to different property types:
s/bounded/bound/
> Source/WebCore/ChangeLog:11
> + the animated attribute and of which type it is. Just for CSS
properties we can share an explicite mappings between
s/explicite mappings/explicit mapping/
> Source/WebCore/ChangeLog:13
> + HashMaps for all SVG elements with animated properties. These maps
get filled once with the function fillAttributeToPropertyTypeMap
with the function <abc> -> with the <abc> function.
> Source/WebCore/svg/SVGElement.h:92
> + void fillAttributeToPropertyTypeMap(AttributeToPropertyTypeMap&) { }
This one is not needed, eh? Please remove it.
> Source/WebCore/svg/SVGStyledElement.cpp:273
> + if (!cssPropertyTypeMap.contains(attrName))
> + return AnimatedUnknown;
> + return cssPropertyTypeMap.get(attrName);
I'd rather write:
if (cssPropertyTypeMap.contains(attrName))
return css...get(attrName)p;
return AnimatedUnknown;
> Source/WebCore/svg/SVGStyledElement.cpp:278
> + SVGElement::fillAttributeToPropertyTypeMap(attributeToPropertyTypeMap);
This is not needed, please remove it (even for consistency, it would be lame,
to call an empty function in sVGElement here)
More information about the webkit-reviews
mailing list