[webkit-reviews] review granted: [Bug 183917] [Web Animations] Support "transition: all" for CSS Transitions as Web Animations : [Attachment 336315] 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 183917: [Web Animations] Support "transition: all" for CSS Transitions as
Web Animations
https://bugs.webkit.org/show_bug.cgi?id=183917

Attachment 336315: Patch

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




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

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

> Source/WebCore/animation/AnimationTimeline.cpp:245
> +	       for (int propertyIndex = 0; propertyIndex <
CSSPropertyAnimation::getNumProperties(); ++propertyIndex) {

Maybe use a variable to hold getNumProperties()?

> Source/WebCore/animation/AnimationTimeline.cpp:247
> +		       // Get the next property which is not a shorthand.

I think this comment is slightly misleading. It should be // Ignore this
property if it is a shorthand.
but that's actually obvious from the code.

> Source/WebCore/animation/AnimationTimeline.cpp:255
> +		   } else if (propertyIndex) {
> +		       // We only go once through this loop if we are
transitioning a single property.
> +		       break;
> +		   }

This logic is a bit hard to understand. I wonder if it would be easier to split
the all and individual cases, to avoid having the strange step of breaking the
loop after one iteration. It seems like you've made an effort to avoid
duplicating code, but at the expense of being able to work out what's
happening.

Anyway, I hope you can think of a more clear way to express what is happening
here.