[webkit-reviews] review denied: [Bug 137583] Introduce a better AnimationController::isRunningAnimationOnRenderer() API for performance : [Attachment 239695] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Chris Dumez
<cdumez at apple.com>'s request for review:
Bug 137583: Introduce a better
AnimationController::isRunningAnimationOnRenderer() API for performance
https://bugs.webkit.org/show_bug.cgi?id=137583

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

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=239695&action=review


> Source/WebCore/page/animation/AnimationBase.h:42
> +enum class AnimationQueryFilter { Any, AcceleratedOnly };

We often use the term "FooOrNot" for thee kinds of enums. I find that a bit
more readable than "queryfilter".

> Source/WebCore/page/animation/AnimationBase.h:44
> +enum AnimatedCSSProperties {

These are really properties that can have accelerated animations. So I'd call
them AcceleratedAnimationPropertyFlags or something.

> Source/WebCore/page/animation/AnimationController.cpp:254
> +    AnimatedCSSPropertyMask animatedProperties = 0;
> +    if (animation.isAnimatingProperty(CSSPropertyOpacity, filter,
runningState))
> +	   animatedProperties |= AnimatingOpacityProperty;
> +    if (animation.isAnimatingProperty(CSSPropertyWebkitFilter, filter,
runningState))
> +	   animatedProperties |= AnimatingWebKitFilterProperty;
> +    if (animation.isAnimatingProperty(CSSPropertyWebkitTransform, filter,
runningState))
> +	   animatedProperties |= AnimatingWebKitTransformProperty;
> +    return animatedProperties;

This is a bit deceptive. animatedProperties makes it sound like a set of all
properties that are animating, but it's really animating properties that happen
to be acceleratable.

> Source/WebCore/rendering/RenderElement.h:164
> +    bool hasCompositeAnimation() const { return m_hasCompositeAnimation; }
> +    void setHasCompositeAnimation(bool b) { m_hasCompositeAnimation = b; }

I think this should be "isCSSAnimating" or something. The fact that it's a
composite animation (crappy name) is just an imp. detail of
AnimationController.