[webkit-reviews] review denied: [Bug 53442] SVGAnimateElement needs information about the animated attribute type : [Attachment 80765] 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 80765: Patch
https://bugs.webkit.org/attachment.cgi?id=80765&action=review

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

Ok, I'm stopping the review here, as I think the general concept is wrong,
after thinking about it again.
We just need a map between eg. SVGRectElements property types for any attribute
it supports.
When storing this information per element in eg. SVGElement or
SVGElementRareData (aka. storing the HashMap), we're basically storing the same
information for _each_ rect in the DOM.
Consider 100 <rect> elements, each holds such a HashMap...

We need to discuss this a bit further.

> Source/WebCore/ChangeLog:11
> +	   the animated attribute and of which type it is. Just for CSS
properties we can use an explicite static mappings between

s/Just for/For/. s/explicite/explicit/

> Source/WebCore/ChangeLog:13
> +	   HashMap in SVGElement. This map gets filled once with the function
fillAttributeToPropertyTypeMap and needs to be

s/SVGElement/SVGElementRareData/

> Source/WebCore/ChangeLog:14
> +	   included in every SVG element.

s/included/implemented/

> Source/WebCore/ChangeLog:16
> +	   No change of functionality, so no new test cases.

Isn't it possible now to animate types, that didn't work before?

> Source/WebCore/svg/SVGFontElement.cpp:65
> +void SVGFontElement::fillAttributeToPropertyTypeMap()
> +{
> +    SVGStyledElement::fillAttributeToPropertyTypeMap();
> +}
> +

No need for this function, if you're not doing anything? Is the implementation
missing so far?