Re: [WIP] Devtools Animations: Update transition timing on timeline interaction (issue 993413004 by samli@chromium.org)

0 views
Skip to first unread message

sa...@chromium.org

unread,
Mar 29, 2015, 8:22:55 PM3/29/15
to dstoc...@chromium.org, dgo...@chromium.org, pfel...@chromium.org, dstoc...@chromium.org, aandre...@chromium.org, alexis...@intel.com, apavlo...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, caseq...@chromium.org, devtools...@chromium.org, ericwi...@chromium.org, eustas...@chromium.org, kozyatins...@chromium.org, loislo...@chromium.org, lushnik...@chromium.org, malch...@chromium.org, mikel...@chromium.org, pfeldma...@chromium.org, rjwr...@chromium.org, sergey...@chromium.org, sh...@chromium.org, steve...@chromium.org, tim...@chromium.org, yurys...@chromium.org
@dgozman: PTAL inspector changes


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

https://codereview.chromium.org/993413004/diff/40001/Source/core/animation/KeyframeEffectModel.cpp#newcode60
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/InspectorAnimationAgent.cpp
File Source/core/inspector/InspectorAnimationAgent.cpp (right):

https://codereview.chromium.org/993413004/diff/40001/Source/core/inspector/InspectorAnimationAgent.cpp#newcode264
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/InspectorAnimationAgent.cpp#newcode266
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/InspectorAnimationAgent.cpp#newcode267
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/InspectorAnimationAgent.cpp#newcode269
Source/core/inspector/InspectorAnimationAgent.cpp:269: // Update
duration
On 2015/03/26 10:30:05, dgozman wrote:
> These comments seem unnecessary.

Done.

https://codereview.chromium.org/993413004/

dgo...@chromium.org

unread,
Mar 30, 2015, 6:38:58 AM3/30/15
to sa...@chromium.org, dstoc...@chromium.org, pfel...@chromium.org, dstoc...@chromium.org, aandre...@chromium.org, alexis...@intel.com, apavlo...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, caseq...@chromium.org, devtools...@chromium.org, ericwi...@chromium.org, eustas...@chromium.org, kozyatins...@chromium.org, loislo...@chromium.org, lushnik...@chromium.org, malch...@chromium.org, mikel...@chromium.org, pfeldma...@chromium.org, rjwr...@chromium.org, sergey...@chromium.org, sh...@chromium.org, steve...@chromium.org, tim...@chromium.org, yurys...@chromium.org

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

https://codereview.chromium.org/993413004/diff/60001/Source/core/inspector/InspectorAnimationAgent.cpp#newcode257
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/InspectorAnimationAgent.cpp#newcode261
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/InspectorAnimationAgent.cpp#newcode265
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_end/elements/AnimationTimeline.js
File Source/devtools/front_end/elements/AnimationTimeline.js (right):

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

https://codereview.chromium.org/993413004/

sa...@chromium.org

unread,
Mar 30, 2015, 7:14:17 PM3/30/15
to dstoc...@chromium.org, dgo...@chromium.org, pfel...@chromium.org, dstoc...@chromium.org, aandre...@chromium.org, alexis...@intel.com, apavlo...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, caseq...@chromium.org, devtools...@chromium.org, ericwi...@chromium.org, eustas...@chromium.org, kozyatins...@chromium.org, loislo...@chromium.org, lushnik...@chromium.org, malch...@chromium.org, mikel...@chromium.org, pfeldma...@chromium.org, rjwr...@chromium.org, sergey...@chromium.org, sh...@chromium.org, steve...@chromium.org, tim...@chromium.org, yurys...@chromium.org
PTAL.


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

https://codereview.chromium.org/993413004/diff/60001/Source/core/inspector/InspectorAnimationAgent.cpp#newcode257
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/InspectorAnimationAgent.cpp#newcode261
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/InspectorAnimationAgent.cpp#newcode265
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_end/elements/AnimationTimeline.js
File Source/devtools/front_end/elements/AnimationTimeline.js (right):

https://codereview.chromium.org/993413004/diff/60001/Source/devtools/front_end/elements/AnimationTimeline.js#newcode789
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.

https://codereview.chromium.org/993413004/

dgo...@chromium.org

unread,
Mar 31, 2015, 1:11:29 AM3/31/15
to sa...@chromium.org, dstoc...@chromium.org, pfel...@chromium.org, dstoc...@chromium.org, aandre...@chromium.org, alexis...@intel.com, apavlo...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, caseq...@chromium.org, devtools...@chromium.org, ericwi...@chromium.org, eustas...@chromium.org, kozyatins...@chromium.org, loislo...@chromium.org, lushnik...@chromium.org, malch...@chromium.org, mikel...@chromium.org, pfeldma...@chromium.org, rjwr...@chromium.org, sergey...@chromium.org, sh...@chromium.org, steve...@chromium.org, tim...@chromium.org, yurys...@chromium.org
lgtm modulo isTransition check


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

https://codereview.chromium.org/993413004/diff/80001/Source/core/inspector/InspectorAnimationAgent.cpp#newcode249
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.

https://codereview.chromium.org/993413004/

sa...@chromium.org

unread,
Mar 31, 2015, 2:17:01 AM3/31/15
to dstoc...@chromium.org, dgo...@chromium.org, pfel...@chromium.org, dstoc...@chromium.org, aandre...@chromium.org, alexis...@intel.com, apavlo...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, caseq...@chromium.org, devtools...@chromium.org, ericwi...@chromium.org, eustas...@chromium.org, kozyatins...@chromium.org, loislo...@chromium.org, lushnik...@chromium.org, malch...@chromium.org, mikel...@chromium.org, pfeldma...@chromium.org, rjwr...@chromium.org, sergey...@chromium.org, sh...@chromium.org, steve...@chromium.org, tim...@chromium.org, yurys...@chromium.org

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

https://codereview.chromium.org/993413004/diff/80001/Source/core/inspector/InspectorAnimationAgent.cpp#newcode249
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?

https://codereview.chromium.org/993413004/

sa...@chromium.org

unread,
Mar 31, 2015, 2:19:04 AM3/31/15
to dstoc...@chromium.org, dgo...@chromium.org, pfel...@chromium.org, dstoc...@chromium.org, aandre...@chromium.org, alexis...@intel.com, apavlo...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, caseq...@chromium.org, devtools...@chromium.org, ericwi...@chromium.org, eustas...@chromium.org, kozyatins...@chromium.org, loislo...@chromium.org, lushnik...@chromium.org, malch...@chromium.org, mikel...@chromium.org, pfeldma...@chromium.org, rjwr...@chromium.org, sergey...@chromium.org, sh...@chromium.org, steve...@chromium.org, tim...@chromium.org, yurys...@chromium.org
I removed the assertions since all to*() methods are macros which have
ASSERT_WITH_SECURITY_IMPLICATIONS() within them (see Assertions.h)

https://codereview.chromium.org/993413004/

dgo...@chromium.org

unread,
Mar 31, 2015, 2:34:45 AM3/31/15
to sa...@chromium.org, dstoc...@chromium.org, pfel...@chromium.org, dstoc...@chromium.org, aandre...@chromium.org, alexis...@intel.com, apavlo...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, caseq...@chromium.org, devtools...@chromium.org, ericwi...@chromium.org, eustas...@chromium.org, kozyatins...@chromium.org, loislo...@chromium.org, lushnik...@chromium.org, malch...@chromium.org, mikel...@chromium.org, pfeldma...@chromium.org, rjwr...@chromium.org, sergey...@chromium.org, sh...@chromium.org, steve...@chromium.org, tim...@chromium.org, yurys...@chromium.org
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.

https://codereview.chromium.org/993413004/

dgo...@chromium.org

unread,
Mar 31, 2015, 2:52:22 AM3/31/15
to sa...@chromium.org, dstoc...@chromium.org, pfel...@chromium.org, dstoc...@chromium.org, aandre...@chromium.org, alexis...@intel.com, apavlo...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, caseq...@chromium.org, devtools...@chromium.org, ericwi...@chromium.org, eustas...@chromium.org, kozyatins...@chromium.org, loislo...@chromium.org, lushnik...@chromium.org, malch...@chromium.org, mikel...@chromium.org, pfeldma...@chromium.org, rjwr...@chromium.org, sergey...@chromium.org, sh...@chromium.org, steve...@chromium.org, tim...@chromium.org, yurys...@chromium.org
As discussed offline, please don't land these before map animationId->type
is
added to InspectorAnimationAgent.

https://codereview.chromium.org/993413004/

commi...@chromium.org

unread,
Mar 31, 2015, 8:17:55 PM3/31/15
to sa...@chromium.org, dstoc...@chromium.org, dgo...@chromium.org, pfel...@chromium.org, dstoc...@chromium.org, aandre...@chromium.org, alexis...@intel.com, apavlo...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, caseq...@chromium.org, devtools...@chromium.org, ericwi...@chromium.org, eustas...@chromium.org, kozyatins...@chromium.org, loislo...@chromium.org, lushnik...@chromium.org, malch...@chromium.org, mikel...@chromium.org, pfeldma...@chromium.org, rjwr...@chromium.org, sergey...@chromium.org, sh...@chromium.org, steve...@chromium.org, tim...@chromium.org, yurys...@chromium.org
CQ is trying da patch. Follow status at
https://chromium-cq-status.appspot.com/patch-status/993413004/120001


https://codereview.chromium.org/993413004/

commi...@chromium.org

unread,
Mar 31, 2015, 9:36:12 PM3/31/15
to sa...@chromium.org, dstoc...@chromium.org, dgo...@chromium.org, pfel...@chromium.org, dstoc...@chromium.org, aandre...@chromium.org, alexis...@intel.com, apavlo...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, caseq...@chromium.org, devtools...@chromium.org, ericwi...@chromium.org, eustas...@chromium.org, kozyatins...@chromium.org, loislo...@chromium.org, lushnik...@chromium.org, malch...@chromium.org, mikel...@chromium.org, pfeldma...@chromium.org, rjwr...@chromium.org, sergey...@chromium.org, sh...@chromium.org, steve...@chromium.org, tim...@chromium.org, yurys...@chromium.org
Committed patchset #7 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192893

https://codereview.chromium.org/993413004/
Reply all
Reply to author
Forward
0 new messages