Devtools Animations: Update transition timing on timeline interaction

Devtools Animations: Update transition timing on timeline interaction

Updates to transition timing are ignored when the transition is still
running. This change ensures that when we modify delay and/or duration
of a transition in the animation timeline, that the transitions timing
is updated accordingly.

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

[samli, 2015-03-17 06:22:15]

samli@chromium.org changed reviewers:
+ dstockwell@chromium.org

[samli, 2015-03-17 06:22:15]

This patch doesn't work in properly in a few ways:
 - when replayed while transition occurs, the animation is not updated properly.
I suspect timing (duration) is updated properly but not the keyframes
 - after the transition finishes and we hit replay, a new transition fires. I
suspect its because transitionData in CSSAnimations::calculateTransitionUpdate
doesn't exist since the transition moves to animation priority when it finishes.
 - hits assert at the end of
CSSAnimations::calculateTransitionUpdateForProperty()

[samli, 2015-03-25 03:08:58]

samli@chromium.org changed reviewers:
+ pfeldman@chromium.org

[samli, 2015-03-25 03:19:11]

PTAL, this is ready for review. It was blocked on
https://codereview.chromium.org/1030103002/

[samli, 2015-03-25 23:01:02]

samli@chromium.org changed reviewers:
+ dgozman@chromium.org

[samli, 2015-03-25 23:01:03]

+dgozman since pfeldman is away.

[dgozman, 2015-03-26 10:30:06]

https://codereview.chromium.org/993413004/diff/40001/Source/core/inspector/In...
File Source/core/inspector/InspectorAnimationAgent.cpp (right):

https://codereview.chromium.org/993413004/diff/40001/Source/core/inspector/In...
Source/core/inspector/InspectorAnimationAgent.cpp:264: for (int i = 0; i < 3;
i++)
Why exactly 3 elements?

https://codereview.chromium.org/993413004/diff/40001/Source/core/inspector/In...
Source/core/inspector/InspectorAnimationAgent.cpp:267:
newFrames[1]->setOffset(delay / (delay + duration));
Why only first frame?

https://codereview.chromium.org/993413004/diff/40001/Source/core/inspector/In...
Source/core/inspector/InspectorAnimationAgent.cpp:269: // Update duration
These comments seem unnecessary.

[dstockwell, 2015-03-26 22:56:16]

lgtm

https://codereview.chromium.org/993413004/diff/40001/Source/core/animation/Ke...
File Source/core/animation/KeyframeEffectModel.cpp (right):

https://codereview.chromium.org/993413004/diff/40001/Source/core/animation/Ke...
Source/core/animation/KeyframeEffectModel.cpp:60: // FIXME: Should also
notify/invalidate the player
TODO(samli):

https://codereview.chromium.org/993413004/diff/40001/Source/core/inspector/In...
File Source/core/inspector/InspectorAnimationAgent.cpp (right):

https://codereview.chromium.org/993413004/diff/40001/Source/core/inspector/In...
Source/core/inspector/InspectorAnimationAgent.cpp:264: for (int i = 0; i < 3;
i++)
ASSERT that there are exactly 3 keyframes

https://codereview.chromium.org/993413004/diff/40001/Source/core/inspector/In...
Source/core/inspector/InspectorAnimationAgent.cpp:266: // Update delay
// Update delay, represented by the distance between the first two keyframes.

[samli, 2015-03-30 00:22:55]

@dgozman: PTAL inspector changes

https://codereview.chromium.org/993413004/diff/40001/Source/core/animation/Ke...
File Source/core/animation/KeyframeEffectModel.cpp (right):

https://codereview.chromium.org/993413004/diff/40001/Source/core/animation/Ke...
Source/core/animation/KeyframeEffectModel.cpp:60: // FIXME: Should also
notify/invalidate the player
On 2015/03/26 22:56:16, dstockwell wrote:
> TODO(samli):

Done.

https://codereview.chromium.org/993413004/diff/40001/Source/core/inspector/In...
File Source/core/inspector/InspectorAnimationAgent.cpp (right):

https://codereview.chromium.org/993413004/diff/40001/Source/core/inspector/In...
Source/core/inspector/InspectorAnimationAgent.cpp:264: for (int i = 0; i < 3;
i++)
On 2015/03/26 22:56:16, dstockwell wrote:
> ASSERT that there are exactly 3 keyframes

dstockwell: Done.
dgozman: Our internal representation of transitions now uses 3 keyframes all the
time.

https://codereview.chromium.org/993413004/diff/40001/Source/core/inspector/In...
Source/core/inspector/InspectorAnimationAgent.cpp:266: // Update delay
On 2015/03/26 22:56:16, dstockwell wrote:
> // Update delay, represented by the distance between the first two keyframes.

Done.

https://codereview.chromium.org/993413004/diff/40001/Source/core/inspector/In...
Source/core/inspector/InspectorAnimationAgent.cpp:267:
newFrames[1]->setOffset(delay / (delay + duration));
On 2015/03/26 10:30:05, dgozman wrote:
> Why only first frame?

As per updated comment, transition delays are represented by the offset of the
second frame.

https://codereview.chromium.org/993413004/diff/40001/Source/core/inspector/In...
Source/core/inspector/InspectorAnimationAgent.cpp:269: // Update duration
On 2015/03/26 10:30:05, dgozman wrote:
> These comments seem unnecessary.

Done.

[dgozman, 2015-03-30 10:38:59]

https://codereview.chromium.org/993413004/diff/60001/Source/core/inspector/In...
File Source/core/inspector/InspectorAnimationAgent.cpp (right):

https://codereview.chromium.org/993413004/diff/60001/Source/core/inspector/In...
Source/core/inspector/InspectorAnimationAgent.cpp:257: KeyframeEffectModelBase*
effect = toKeyframeEffectModelBase(animation->effect());
Can we assert before calling |toKeyframeEffectModelBase| that effect is indeed
keyframe effect?

https://codereview.chromium.org/993413004/diff/60001/Source/core/inspector/In...
Source/core/inspector/InspectorAnimationAgent.cpp:261: ASSERT(frames.size() ==
3);
Is there any documentation/explanation about the design of transition (being 3
keyframes)? Let's point there in the comment, otherwise this code is too
cryptic.

https://codereview.chromium.org/993413004/diff/60001/Source/core/inspector/In...
Source/core/inspector/InspectorAnimationAgent.cpp:265: // Update delay,
represented by the distance between the first two keyframes
nit: full stop please.

https://codereview.chromium.org/993413004/diff/60001/Source/devtools/front_en...
File Source/devtools/front_end/elements/AnimationTimeline.js (right):

https://codereview.chromium.org/993413004/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/elements/AnimationTimeline.js:789: if
(this._animation.type() !== "CSSAnimation") {
What about CSSAnimation? Should you leave the FIXME?

[samli, 2015-03-30 23:14:17]

PTAL.

https://codereview.chromium.org/993413004/diff/60001/Source/core/inspector/In...
File Source/core/inspector/InspectorAnimationAgent.cpp (right):

https://codereview.chromium.org/993413004/diff/60001/Source/core/inspector/In...
Source/core/inspector/InspectorAnimationAgent.cpp:257: KeyframeEffectModelBase*
effect = toKeyframeEffectModelBase(animation->effect());
On 2015/03/30 10:38:58, dgozman wrote:
> Can we assert before calling |toKeyframeEffectModelBase| that effect is indeed
> keyframe effect?

Done.

https://codereview.chromium.org/993413004/diff/60001/Source/core/inspector/In...
Source/core/inspector/InspectorAnimationAgent.cpp:261: ASSERT(frames.size() ==
3);
On 2015/03/30 10:38:58, dgozman wrote:
> Is there any documentation/explanation about the design of transition (being 3
> keyframes)? Let's point there in the comment, otherwise this code is too
> cryptic.

Done. There isn't really much to it, except for the fact that delay is
represented by a keyframe instead of "delay" timing parameter in the web
animations API. We have an internal doc discussing why we needed to do this in
the first place, but that isn't necessary for the comprehension of this code.

https://codereview.chromium.org/993413004/diff/60001/Source/core/inspector/In...
Source/core/inspector/InspectorAnimationAgent.cpp:265: // Update delay,
represented by the distance between the first two keyframes
On 2015/03/30 10:38:58, dgozman wrote:
> nit: full stop please.

Done.

https://codereview.chromium.org/993413004/diff/60001/Source/devtools/front_en...
File Source/devtools/front_end/elements/AnimationTimeline.js (right):

https://codereview.chromium.org/993413004/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/elements/AnimationTimeline.js:789: if
(this._animation.type() !== "CSSAnimation") {
On 2015/03/30 10:38:58, dgozman wrote:
> What about CSSAnimation? Should you leave the FIXME?

No, CSS animations are already supported in http://crrev.com/895783004. The
inline style is updated and changes are automatically detected by the animations
engine. Transitions are not spec-ed in the same manner which is why we need to
update it in this manner.

[dgozman, 2015-03-31 05:11:29]

lgtm modulo isTransition check

https://codereview.chromium.org/993413004/diff/80001/Source/core/inspector/In...
File Source/core/inspector/InspectorAnimationAgent.cpp (right):

https://codereview.chromium.org/993413004/diff/80001/Source/core/inspector/In...
Source/core/inspector/InspectorAnimationAgent.cpp:249: void
InspectorAnimationAgent::setTiming(ErrorString* errorString, const String&
playerId, double duration, double delay, bool isTransition)
Can you check |isTransition| flag here on backend instead of passing it from
frontend? If you can, let's remove it.

[samli, 2015-03-31 06:17:00]

https://codereview.chromium.org/993413004/diff/80001/Source/core/inspector/In...
File Source/core/inspector/InspectorAnimationAgent.cpp (right):

https://codereview.chromium.org/993413004/diff/80001/Source/core/inspector/In...
Source/core/inspector/InspectorAnimationAgent.cpp:249: void
InspectorAnimationAgent::setTiming(ErrorString* errorString, const String&
playerId, double duration, double delay, bool isTransition)
On 2015/03/31 05:11:29, dgozman wrote:
> Can you check |isTransition| flag here on backend instead of passing it from
> frontend? If you can, let's remove it.

No. The way I classify if a created animation is a transition is quite
complicated. (Look up the targeted elements style resolver and check if it is
there). When this call is made, it is likely that it is no longer in this list
because it may have finished or its priority has changed.

The only way I could do this in the backend is to create a new map of the type
in the AnimationAgent and store it when I create it, but this seems unnecessary.

Another option would be to create separate setTiming() methods:
setTimingForTransition(), setTimingForWebAnimation() since they don't really
reuse any code.

WDYT?

[samli, 2015-03-31 06:19:04]

I removed the assertions since all to*() methods are macros which have
ASSERT_WITH_SECURITY_IMPLICATIONS() within them (see Assertions.h)

[dgozman, 2015-03-31 06:34:43]

On 2015/03/31 06:17:00, samli wrote:
>
https://codereview.chromium.org/993413004/diff/80001/Source/core/inspector/In...
> File Source/core/inspector/InspectorAnimationAgent.cpp (right):
> 
>
https://codereview.chromium.org/993413004/diff/80001/Source/core/inspector/In...
> Source/core/inspector/InspectorAnimationAgent.cpp:249: void
> InspectorAnimationAgent::setTiming(ErrorString* errorString, const String&
> playerId, double duration, double delay, bool isTransition)
> On 2015/03/31 05:11:29, dgozman wrote:
> > Can you check |isTransition| flag here on backend instead of passing it from
> > frontend? If you can, let's remove it.
> 
> No. The way I classify if a created animation is a transition is quite
> complicated. (Look up the targeted elements style resolver and check if it is
> there). When this call is made, it is likely that it is no longer in this list
> because it may have finished or its priority has changed.
> 
> The only way I could do this in the backend is to create a new map of the type
> in the AnimationAgent and store it when I create it, but this seems
unnecessary.
> 
> Another option would be to create separate setTiming() methods:
> setTimingForTransition(), setTimingForWebAnimation() since they don't really
> reuse any code.
> 
> WDYT?

Well, I think the current solution is better than both of these.
But you have to turn asserts into ifs and report protocol error if something is
wrong.

[dgozman, 2015-03-31 06:52:21]

As discussed offline, please don't land these before map animationId->type is
added to InspectorAnimationAgent.

[samli, 2015-04-01 00:17:31]

The CQ bit was checked by samli@chromium.org

[samli, 2015-04-01 00:17:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
dstockwell@chromium.org, dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/993413004/#ps120001
(title: " ")

[commit-bot: I haz the power, 2015-04-01 00:17:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/993413004/120001

[commit-bot: I haz the power, 2015-04-01 01:36:11]

Committed patchset #7 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192893