[webkit-reviews] review denied: [Bug 54410] SVG animation doesn't support attribute value 'inherit' : [Attachment 82367] 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 54410: SVG animation doesn't support attribute value 'inherit'
https://bugs.webkit.org/show_bug.cgi?id=54410

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

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

Almost r=me, still some comments to be resolved, setting r- for the meanwhile.

> Source/WebCore/ChangeLog:19
> +	   (WebCore::SVGAnimateElement::calculateAnimatedValue): Get the
computed style of attribute for 'inherit'
> +	   or 'currentColor' during the animation, since the values could be
animated themselves.

When a property value is 'inherit' or 'currentColor' during the animation, get
the computed style of the property since the values could be animated
themselves.

> Source/WebCore/ChangeLog:28
> +	   the attribute is an animatable CSS property with the use of a CSS
property map in SVGStyledElement.

s/with the use of .../by using the CSS property map in SVGStyledElement/.

> Source/WebCore/ChangeLog:30
> +	   (WebCore::SVGAnimationElement::setTargetAttributeAnimatedValue):
Take targetElement instead of target
> +	   for consistency reasons.

s/Take..../ -> s/target/targetElement/ for consistency.

> Source/WebCore/ChangeLog:33
> +	   (WebCore::SVGStyledElement::isAnimatableCSSProperty): Check if
attribute is a animatable CSS property.

s/Check if/.../ -> Checks if the CSS property is animable.

> Source/WebCore/svg/SVGAnimateElement.cpp:101
> +static inline String adujustForInheritance(SVGElement* targetElement, const
String& attributeName)

For consistency: avoid the return value, and use a "String& attributeValue" as
parameter?
s/adujust/adjust/

> Source/WebCore/svg/SVGAnimateElement.cpp:103
> +    // FIXME: Needs to handle animation types directly.

What does that mean?

> Source/WebCore/svg/SVGAnimateElement.cpp:163
> +	   // Get current values on property value 'currentColor' as well as
'inherit'

Replace 'currentColor' / 'inherit' by their computed property values.

> Source/WebCore/svg/SVGAnimateElement.cpp:192
> +	   // Get current values on property value 'currentColor' and 'inherit'


Same comment as I suggested above. And use periods at the end of sentences.

> Source/WebCore/svg/SVGAnimateElement.h:54
> +    // If we have 'currentColor' or 'inherit' as animation value, we need to
grep the value during the animation

s/grep/grab/

> Source/WebCore/svg/SVGAnimateElement.h:56
> +    enum BasicValueChange {

Why BasicValueChange? Doesn't make a lot of sense to me.
enum AnimatedPropertyValueType {
RegularPropertyValue,
CurrentColorValue,
InheritValue
}; ?

> Source/WebCore/svg/SVGAnimateElement.h:59
> +	   constantBasicValue,
> +	   currentColorBasicValue,
> +	   inheritBasicValue

Capitalize those enum values.