[webkit-reviews] review denied: [Bug 79790] Introduce SMIL overrideStyle, to make SVG stop mutating CSS styles directly : [Attachment 129434] Patch v4

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied  review:
Bug 79790: Introduce SMIL overrideStyle, to make SVG stop mutating CSS styles
directly
https://bugs.webkit.org/show_bug.cgi?id=79790

Attachment 129434: Patch v4
https://bugs.webkit.org/attachment.cgi?id=129434&action=review

------- Additional Comments from Dirk Schulze <krit at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=129434&action=review


Sorry, you changed the behavior without testing it. r-

A great patch in general. But I am still skeptical of shouldApplyAnimation.
This needs more testing and verifications. I would also like to discuss the
role of attributeType on SVG WG first.

> Source/WebCore/svg/SVGAnimationElement.cpp:409
> +	   // For attributeType="auto", try CSS first. If that fails, try XML.

Difficult, this might not be future confident. We will move more attributes to
presentation attributes like 'transform', for theses properties, it might be
better to use XML as default (in theory). So that we can use the XML parser for
parsing. Alternatively we could extend SVG only rules to the CSS parser. Not
sure if the last suggestion is the best approach.

> Source/WebCore/svg/SVGAnimationElement.cpp:414
> +	   // If attributeName is unknown and ttributeType is not 'auto', don't
apply the animation.

s/ttributeType/attributeType/

> Source/WebCore/svg/SVGAnimationElement.cpp:415
> +	   if (attributeType == AttributeTypeCSS)

<animate attributeName="width" attributeType="CSS"> won't animate? Did you
check this on other browsers? This is a change of the current behavior! Did you
add tests for that?