[webkit-reviews] review denied: [Bug 53442] SVGAnimateElement needs information about the animated attribute type : [Attachment 80757] Patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied 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 80757: Patch
https://bugs.webkit.org/attachment.cgi?id=80757&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=80757&action=review

Almost there, still r- though.

> Source/WebCore/svg/SVGAnimateElement.cpp:91
> +    AnimatedAttributeType type =
targetElement()->animatedPropertyTypeForAttribute(QualifiedName(nullAtom,
attribute, nullAtom));
>      // FIXME: We need a full property table for figuring this out reliably.
> -    if (hasTagName(SVGNames::animateColorTag))
> +    if (hasTagName(SVGNames::animateColorTag) || type == AnimatedColor)
>	   return ColorProperty;

You don't need to ask the targetElement(), if
hasTagNames(SVGNames::animateColorTag) is true. Make it a early exit shortcut.

> Source/WebCore/svg/SVGCircleElement.cpp:139
> +    AttributeToPropertyTypeMap& animatablePropertyMap =
this->animatablePropertyMap();

s/animatablePropertyMap/attributeToPropertyTypeMap/ ?

> Source/WebCore/svg/SVGElement.h:140
> +    AttributeToPropertyTypeMap m_animatedAttributeMap;

Obsolete, please remove.

> Source/WebCore/svg/SVGElementRareData.h:60
> +    HashMap<QualifiedName, AnimatedAttributeType>& animatedAttributeMap() {
return m_animatedAttributeMap; }

Use the typdef.

> Source/WebCore/svg/SVGElementRareData.h:73
> +    HashMap<QualifiedName, AnimatedAttributeType> m_animatedAttributeMap;

Ditto.

> Source/WebCore/svg/SVGGlyphElement.cpp:83
> +void SVGGlyphElement::svgAttributeChanged(const QualifiedName& attrName)
> +{
> +    if (attrName == SVGNames::dAttr)
> +	   invalidateGlyphCache();
> +}

This seems unrelated.

> Source/WebCore/svg/SVGGlyphElement.h:120
> +    virtual void svgAttributeChanged(const QualifiedName&);

Ditto.

> Source/WebCore/svg/SVGStyledElement.cpp:204
> +AttributeToPropertyTypeMap& SVGStyledElement::cssPropertyToTypeMap()

Make it a static inline.

> Source/WebCore/svg/SVGStyledElement.h:86
> +    AttributeToPropertyTypeMap& cssPropertyToTypeMap();

No need for this, if you convert it to be a static inline.