[webkit-reviews] review granted: [Bug 12437] SVG Animations update baseVal instead of animVal : [Attachment 138960] Final patch v4 (Updated v3 against ToT)

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has granted Nikolas Zimmermann
<zimmermann at kde.org>'s request for review:
Bug 12437: SVG Animations update baseVal instead of animVal
https://bugs.webkit.org/show_bug.cgi?id=12437

Attachment 138960: Final patch v4 (Updated v3 against ToT)
https://bugs.webkit.org/attachment.cgi?id=138960&action=review

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


The patch and the tests look good to me. I would suggest that you use a
different term for computedOverrideStyle. This term seems to be based on SMIL,
but is questionable nowadays where we have more animations than just SMIL on
properties. I give you r=me but with the renaming or a response why you stay on
this naming before landing.

> Source/WebCore/svg/SVGElementRareData.h:110
> +    RenderStyle* overrideComputedStyle(SVGElement* element)
> +    {
> +	   ASSERT(element);
> +	   if (!m_useOverrideComputedStyle)
> +	       return 0;
> +	   if (!m_needsOverrideComputedStyleUpdate && m_overrideComputedStyle)
> +	       return m_overrideComputedStyle.get();
> +
> +	   m_needsOverrideComputedStyleUpdate = false;
> +	   if (!element->document()) {
> +	       m_overrideComputedStyle.clear();
> +	       return 0;
> +	   }
> +
> +	   m_overrideComputedStyle =
element->document()->styleResolver()->styleForElement(element,
element->parentNode() ? element->parentNode()->computedStyle() : 0,
AllowStyleSharing, MatchAllRulesExcludingSMIL);
> +	   return m_overrideComputedStyle.get();
> +    }

with your clarification it makes more sense what you do here. At least I
believe that. override style and computed style terms are still confusing and
shouldn't be used here, since they include animations. We discussed it on IRC
and by mail and you don't want to include these. So why not use the new term
"intrinsic style"? This seems to be introduced in CSS Animations:
http://www.w3.org/TR/css3-animations/#animations. There is a bug report on W3C
bugzilla to nail the definition down, but during writing this review the server
was down. I think you should consider using "intrinsicStyle".