[webkit-reviews] review granted: [Bug 179973] [Web Animations] Perform hardware-composited animations when possible : [Attachment 327498] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Antoine Quint <graouts at apple.com>'s
request for review:
Bug 179973: [Web Animations] Perform hardware-composited animations when
possible
https://bugs.webkit.org/show_bug.cgi?id=179973

Attachment 327498: Patch

https://bugs.webkit.org/attachment.cgi?id=327498&action=review




--- Comment #3 from Dean Jackson <dino at apple.com> ---
Comment on attachment 327498
  --> https://bugs.webkit.org/attachment.cgi?id=327498
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=327498&action=review

I know you're unsure about the term "hardware compositing" vs "accelerated". I
think more people are familiar with the term accelerated, or maybe just
"composited".

> Source/WebCore/animation/DocumentTimeline.cpp:170
> +	   animation->toggleHardwareCompositing();

I don't like the name "toggle", because it's not clear what is actually
happening. Is this turning it on or off? Or does it depend on the animation?

> Source/WebCore/animation/DocumentTimeline.cpp:182
> +bool
DocumentTimeline::elementRunningAnimationsAreAllHardwareComposited(Element&
element)

This is also a weird name. It's asking if the Element has only hardware
composited animations. Maybe elementUsingHardwareCompositingForAnimations?

> Source/WebCore/animation/KeyframeEffect.cpp:150
> +    if (m_startedHardwareCompositedAnimation && localTime >=
timing()->duration()) {

I wonder if relativeTime is a better name than localTime. I had to think a bit
to work out what localTime >= duration meant (because it makes no sense to
compare a general time to a duration).

> Source/WebCore/animation/KeyframeEffect.cpp:184
> +    if (needsToStartWithHardwareCompositing)
> +	   animation()->hardwareCompositingActiveStateDidChange();

I assume you have to call this after you flip the Z index? Otherwise it makes
more sense to go up where you set needsToStartWithHardwareCompositing.