Bug 183917 – [Web Animations] Support "transition: all" for CSS Transitions as Web Animations

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Antoine Quint]

[Web Animations] Support "transition: all" for CSS Transitions as Web Animations

[Antoine Quint]

Created attachment 336315 [details]
Patch

[EWS Watchlist]

Comment on attachment 336315 [details]
Patch

Attachment 336315 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7068084

New failing tests:
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html

[EWS Watchlist]

Created attachment 336339 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Dean Jackson]

Comment on attachment 336315 [details]
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.

[Antoine Quint]

(In reply to Dean Jackson from comment #4)
> Comment on attachment 336315 [details]
> 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()?

Sure, will fix in commit.

> > 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.

Will remove entirely, it is obvious.

> > 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.

Will try to improve the comment.

[Antoine Quint]

Committed r229888: <https://trac.webkit.org/changeset/229888>

[Radar WebKit Bug Importer]

<rdar://problem/38789890>