Web Animations: Correctly handle incomplete keyframes in CSS animations

Web Animations: Correctly handle incomplete keyframes in CSS animations

BUG=240653
R=dstockwell

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=158612

[Steve Block, 2013-09-30 04:25:23]

Note that this depends on https://codereview.chromium.org/25079006

[dstockwell, 2013-09-30 07:30:57]

https://codereview.chromium.org/23875044/diff/17001/Source/core/css/resolver/...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/23875044/diff/17001/Source/core/css/resolver/...
Source/core/css/resolver/StyleResolver.cpp:961: // properties, as values for
these properties will be filled in by
Can't we fill in the end keyframes before counting?

https://codereview.chromium.org/23875044/diff/17001/Source/core/css/resolver/...
Source/core/css/resolver/StyleResolver.cpp:1025:
keyframesAndTimingFunctions.append(std::make_pair(keyframes,
generateTimingFunction(keyframes, perKeyframeTimingFunctions)));
We probably don't want to do this if numPropertiesSpecifiedInAllKeyframes is 0
and keyframesAndTimingFunctions is not empty.

[Steve Block, 2013-09-30 12:17:54]

https://codereview.chromium.org/23875044/diff/17001/Source/core/css/resolver/...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/23875044/diff/17001/Source/core/css/resolver/...
Source/core/css/resolver/StyleResolver.cpp:961: // properties, as values for
these properties will be filled in by
We could, but in the case that we have to split out any properties, we'd have to
find those newly added property values in the end keyframes and move them to the
new keyframe groups. That approach would eliminate a little code duplication,
but is less efficient and seems an odd way to do it.

Note that in the common case where all non-end keyframes specify all properties,
there's no significant difference between these two approaches.

https://codereview.chromium.org/23875044/diff/17001/Source/core/css/resolver/...
Source/core/css/resolver/StyleResolver.cpp:1025:
keyframesAndTimingFunctions.append(std::make_pair(keyframes,
generateTimingFunction(keyframes, perKeyframeTimingFunctions)));
Good point. Done.

[dstockwell, 2013-09-30 21:56:43]

lgtm

https://codereview.chromium.org/23875044/diff/17001/Source/core/css/resolver/...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/23875044/diff/17001/Source/core/css/resolver/...
Source/core/css/resolver/StyleResolver.cpp:961: // properties, as values for
these properties will be filled in by
On 2013/09/30 12:17:54, Steve Block wrote:
> We could, but in the case that we have to split out any properties, we'd have
to
> find those newly added property values in the end keyframes and move them to
the
> new keyframe groups. That approach would eliminate a little code duplication,
> but is less efficient and seems an odd way to do it.

It would eliminate lines 973 -> 987, right? I found that section quite
confusing.

I'm not sure being trivially more efficient is worth it for the non-common case,
but it's your call.

[Steve Block, 2013-10-01 04:39:53]

https://codereview.chromium.org/23875044/diff/17001/Source/core/css/resolver/...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/23875044/diff/17001/Source/core/css/resolver/...
Source/core/css/resolver/StyleResolver.cpp:961: // properties, as values for
these properties will be filled in by
I've updated the patch to handle end keyframes first. We can revert to the
previous approach if we find this approach to be too slow or confusing.

[dstockwell, 2013-10-01 05:55:24]

lgtm

[commit-bot: I haz the power, 2013-10-01 06:12:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/steveblock@chromium.org/23875044/31001

[commit-bot: I haz the power, 2013-10-01 08:42:40]

Change committed as 158612