[webkit-reviews] review granted: [Bug 96697] Refactor SMILTimeContainer to maintain animation information instead of recalculating it every frame : [Attachment 165406] Address reviewer comments

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted Philip Rogers <pdr at google.com>'s
request for review:
Bug 96697: Refactor SMILTimeContainer to maintain animation information instead
of recalculating it every frame
https://bugs.webkit.org/show_bug.cgi?id=96697

Attachment 165406: Address reviewer comments
https://bugs.webkit.org/attachment.cgi?id=165406&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=165406&action=review


This seems reasonable.	I think a few more comments could make this clearer for
future hackers in this area.

> Source/WebCore/svg/animation/SMILTimeContainer.cpp:106
> +    startTimer(0);

We should comment here as to why this uses a timer. :)

> Source/WebCore/svg/animation/SMILTimeContainer.cpp:187
> +    m_preventScheduledAnimationsChanges = true;

This will eventually become a RAII I figured. :)

> Source/WebCore/svg/animation/SMILTimeContainer.cpp:264
> +    // This boolean protects against these cases.

It doesn't protect JS from executing, so we should be more clear here. :)  You
could set teh eventDispatchForbidden bool if you want that. :)	 (that doesn't
prevent event dispatch, but will ASSERT if ti happens).

// This boolean will catch any attempts to modify scheduledAnimations during
this critical section.... or some such.

> Source/WebCore/svg/animation/SMILTimeContainer.h:96
> +    bool m_preventScheduledAnimationsChanges;

Should this be a static?  s_ instead?

> Source/WebCore/svg/animation/SVGSMILElement.cpp:205
> +	   setAttributeName(constructQualifiedName(this,
fastGetAttribute(SVGNames::attributeNameAttr)));

Why not just do this at the top like before?

> Source/WebCore/svg/animation/SVGSMILElement.cpp:616
> +    // Force resolution of target element

"Why" is more useful than "what" here.