Devtools Animations: Support multiple frames in the animation timeline (issue 1042143005 by samli@chromium.org)

0 views
Skip to first unread message

sa...@chromium.org

unread,
Mar 31, 2015, 2:56:08 AM3/31/15
to dgo...@chromium.org, aandre...@chromium.org, alexis...@intel.com, apavlo...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, caseq...@chromium.org, devtools...@chromium.org, dstoc...@chromium.org, ericwi...@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
Reviewers: dgozman,

Message:
WIP.

Incomplete tasks:
- Check properly attached to all new incoming frames (and set playback rate
on
them)
- on didCommitLoadForLocalFrame(), clear maps appropriately - this is
causing an
issue at the moment
- Update start times when IAA::setCurrentTime() is called (just need to the
callback)

Description:
Devtools Animations: Support multiple frames in the animation timeline

Currently, the animation timeline only works for animations attached to
the main document frame. This change ensures the inspector agent is properly
attached to all frames within the document. All start times sent to the
front-end are normalised to the monotonic time. As the playback rate or
current time of an animation timeline are manipulated, these monotonic
time values are invalidated. Since these methods are controlled from the
front-end, the normalised start times are updated. This has no visible UI
change.

BUG=466827

Please review this at https://codereview.chromium.org/1042143005/

Base URL: svn://svn.chromium.org/blink/trunk

Affected files (+79, -9 lines):
M Source/core/inspector/InspectorAnimationAgent.h
M Source/core/inspector/InspectorAnimationAgent.cpp
M Source/devtools/front_end/elements/AnimationTimeline.js
M Source/devtools/front_end/sdk/AnimationModel.js
M Source/devtools/protocol.json


Index: Source/core/inspector/InspectorAnimationAgent.cpp
diff --git a/Source/core/inspector/InspectorAnimationAgent.cpp
b/Source/core/inspector/InspectorAnimationAgent.cpp
index
7d3068ba83fd43dba20412ccbd001dd69b5cb3bb..67e54adeb094c0d601808e462528ae5c375f4604
100644
--- a/Source/core/inspector/InspectorAnimationAgent.cpp
+++ b/Source/core/inspector/InspectorAnimationAgent.cpp
@@ -162,6 +162,12 @@ static
PassRefPtr<TypeBuilder::Animation::KeyframesRule> buildObjectForAnimation
return keyframesObject.release();
}

+static double normalizedStartTime(AnimationPlayer& player)
+{
+ ASSERT(player.timeline());
+ return player.startTime() / player.timeline()->playbackRate() +
player.timeline()->zeroTime();
+}
+
PassRefPtr<TypeBuilder::Animation::AnimationPlayer>
InspectorAnimationAgent::buildObjectForAnimationPlayer(AnimationPlayer&
player)
{
// Find type of animation
@@ -195,7 +201,7 @@ PassRefPtr<TypeBuilder::Animation::AnimationPlayer>
InspectorAnimationAgent::bui
.setPausedState(player.paused())
.setPlayState(player.playState())
.setPlaybackRate(player.playbackRate())
- .setStartTime(player.startTime())
+ .setStartTime(normalizedStartTime(player))
.setCurrentTime(player.currentTime())
.setSource(animationObject.release())
.setType(animationType);
@@ -230,20 +236,41 @@ void
InspectorAnimationAgent::getAnimationPlayersForNode(ErrorString* errorStrin

void InspectorAnimationAgent::getPlaybackRate(ErrorString*, double*
playbackRate)
{
+ // All timelines should have the same playback rate
*playbackRate =
m_pageAgent->inspectedFrame()->document()->timeline().playbackRate();
}

-void InspectorAnimationAgent::setPlaybackRate(ErrorString*, double
playbackRate)
+void InspectorAnimationAgent::setPlaybackRate(ErrorString*, double
playbackRate,
RefPtr<TypeBuilder::Array<TypeBuilder::Animation::AnimationStartTime>>&
animationStartTimes)
{
for (Frame* frame = m_pageAgent->inspectedFrame(); frame; frame =
frame->tree().traverseNext(m_pageAgent->inspectedFrame())) {
if (frame->isLocalFrame())

toLocalFrame(frame)->document()->timeline().setPlaybackRate(playbackRate);
}
+ animationStartTimes = buildArrayForStartTimes();
+}
+
+void InspectorAnimationAgent::setCurrentTime(ErrorString*, double
normalisedCurrentTime,
RefPtr<TypeBuilder::Array<TypeBuilder::Animation::AnimationStartTime>>&
animationStartTimes)
+{
+ for (Frame* frame = m_pageAgent->inspectedFrame(); frame; frame =
frame->tree().traverseNext(m_pageAgent->inspectedFrame())) {
+ if (frame->isLocalFrame()) {
+ AnimationTimeline& timeline =
toLocalFrame(frame)->document()->timeline();
+ timeline.setCurrentTime((normalisedCurrentTime -
timeline.zeroTime()) * timeline.playbackRate());
+ }
+ }
+ animationStartTimes = buildArrayForStartTimes();
}

-void InspectorAnimationAgent::setCurrentTime(ErrorString*, double
currentTime)
+PassRefPtr<TypeBuilder::Array<TypeBuilder::Animation::AnimationStartTime>>
InspectorAnimationAgent::buildArrayForStartTimes()
{
-
m_pageAgent->inspectedFrame()->document()->timeline().setCurrentTime(currentTime);
+ RefPtr<TypeBuilder::Array<TypeBuilder::Animation::AnimationStartTime>>
animationStartTimes =
TypeBuilder::Array<TypeBuilder::Animation::AnimationStartTime>::create();
+ for (const auto& pair : m_idToAnimationPlayer) {
+ const RefPtr<AnimationPlayer> player = pair.value;
+ RefPtr<TypeBuilder::Animation::AnimationStartTime> startTimeObject
= TypeBuilder::Animation::AnimationStartTime::create()
+ .setId(pair.key)
+ .setStartTime(normalizedStartTime(*player.get()));
+ animationStartTimes->addItem(startTimeObject);
+ }
+ return animationStartTimes.release();
}

void InspectorAnimationAgent::setTiming(ErrorString* errorString, const
String& playerId, double duration, double delay)
Index: Source/core/inspector/InspectorAnimationAgent.h
diff --git a/Source/core/inspector/InspectorAnimationAgent.h
b/Source/core/inspector/InspectorAnimationAgent.h
index
9fc782d2301a7e0a638ac7be91a73ecb88a38663..430bed110b41b353801ab3835210f05e7024a347
100644
--- a/Source/core/inspector/InspectorAnimationAgent.h
+++ b/Source/core/inspector/InspectorAnimationAgent.h
@@ -35,8 +35,8 @@ public:
// Protocol method implementations
virtual void getAnimationPlayersForNode(ErrorString*, int nodeId, bool
includeSubtreeAnimations,
RefPtr<TypeBuilder::Array<TypeBuilder::Animation::AnimationPlayer> >&
animationPlayersArray) override;
virtual void getPlaybackRate(ErrorString*, double* playbackRate)
override;
- virtual void setPlaybackRate(ErrorString*, double playbackRate)
override;
- virtual void setCurrentTime(ErrorString*, double currentTime) override;
+ virtual void setPlaybackRate(ErrorString*, double playbackRate,
RefPtr<TypeBuilder::Array<TypeBuilder::Animation::AnimationStartTime>>&
animationStartTimes) override;
+ virtual void setCurrentTime(ErrorString*, double
normalisedCurrentTime,
RefPtr<TypeBuilder::Array<TypeBuilder::Animation::AnimationStartTime>>&
animationStartTimes) override;
virtual void setTiming(ErrorString*, const String& playerId, double
duration, double delay) override;

// API for InspectorInstrumentation
@@ -59,6 +59,7 @@ private:
PassRefPtr<TypeBuilder::Animation::AnimationPlayer>
buildObjectForAnimationPlayer(AnimationPlayer&);
PassRefPtr<TypeBuilder::Animation::AnimationPlayer>
buildObjectForAnimationPlayer(AnimationPlayer&, AnimationType,
PassRefPtr<TypeBuilder::Animation::KeyframesRule> keyframeRule = nullptr);
PassRefPtr<TypeBuilder::Array<TypeBuilder::Animation::AnimationPlayer>
> buildArrayForAnimationPlayers(Element&, const
WillBeHeapVector<RefPtrWillBeMember<AnimationPlayer> >);
+
PassRefPtr<TypeBuilder::Array<TypeBuilder::Animation::AnimationStartTime>>
buildArrayForStartTimes();

RawPtrWillBeMember<InspectorPageAgent> m_pageAgent;
RawPtrWillBeMember<InspectorDOMAgent> m_domAgent;
Index: Source/devtools/front_end/elements/AnimationTimeline.js
diff --git a/Source/devtools/front_end/elements/AnimationTimeline.js
b/Source/devtools/front_end/elements/AnimationTimeline.js
index
b8c496a74ba361ade64c588bb80aea30271c2c08..0ffba06a68aaf81119f04056c74b913c6db40016
100644
--- a/Source/devtools/front_end/elements/AnimationTimeline.js
+++ b/Source/devtools/front_end/elements/AnimationTimeline.js
@@ -46,7 +46,7 @@ WebInspector.AnimationTimeline.prototype = {
this._animationsPlaybackRate =
WebInspector.AnimationsSidebarPane.GlobalPlaybackRates[event.target.value];
var target = WebInspector.targetManager.mainTarget();
if (target)
-
target.animationAgent().setPlaybackRate(this._animationsPlaybackRate);
+
target.animationAgent().setPlaybackRate(this._animationsPlaybackRate,
this._updateStartTimes.bind(this));
this._playbackLabel.textContent = this._animationsPlaybackRate
+ "x";

WebInspector.userMetrics.AnimationsPlaybackRateChanged.record();
if (this._scrubberPlayer)
@@ -83,6 +83,25 @@ WebInspector.AnimationTimeline.prototype = {
return container;
},

+ /**
+ * @param {?Protocol.Error} error
+ * @param {!Array.<!AnimationAgent.AnimationStartTime>} startTimes
+ */
+ _updateStartTimes: function(error, startTimes)
+ {
+ if (error)
+ return;
+
+ delete this._startTime;
+ for (var player in startTimes) {
+ var animation = this._animationsMap.get(player.id);
+ console.assert(animation);
+ animation.updateStartTime(player.startTime);
+ if (!this._startTime || player.startTime < this._startTime)
+ this._startTime = player.startTime;
+ }
+ },
+
_updateAnimationsPlaybackRate: function()
{
/**
Index: Source/devtools/front_end/sdk/AnimationModel.js
diff --git a/Source/devtools/front_end/sdk/AnimationModel.js
b/Source/devtools/front_end/sdk/AnimationModel.js
index
573293b402253abe935d0204af9832f2782d23e9..89e066f349b2eddfee87a10a3f947473d72ea66c
100644
--- a/Source/devtools/front_end/sdk/AnimationModel.js
+++ b/Source/devtools/front_end/sdk/AnimationModel.js
@@ -159,7 +159,15 @@ WebInspector.AnimationModel.AnimationPlayer.prototype
= {
*/
startTime: function()
{
- return this._payload.startTime;
+ return this._startTime === undefined ? this._payload.startTime :
this._startTime;
+ },
+
+ /**
+ * @param {number} startTime
+ */
+ updateStartTime: function(startTime)
+ {
+ this._startTime = startTime;
},

/**
Index: Source/devtools/protocol.json
diff --git a/Source/devtools/protocol.json b/Source/devtools/protocol.json
index
92bb2d92a9e9099cb327e4d55c9e51894ad32ced..f91508c7632dee990fa36d761b4d8506e0ebbba8
100644
--- a/Source/devtools/protocol.json
+++ b/Source/devtools/protocol.json
@@ -4871,6 +4871,15 @@

{ "name": "easing", "type": "string", "description": "<code>AnimationNode</code>'s
timing function." }
],
"description": "Keyframe Style"
+ },
+ {
+ "id": "AnimationStartTime",
+ "type": "object",
+ "properties": [
+
{ "name": "id", "type": "string", "description": "<code>AnimationPlayer</code>'s
id." },
+
{ "name": "startTime", "type": "number", "description": "Updated normalized
<code>AnimationPlayer</code>'s start time." }
+ ],
+ "description": "Map from animation id to animation start
time"
}
],
"commands": [
@@ -4902,12 +4911,18 @@
"parameters": [

{ "name": "playbackRate", "type": "number", "description": "Playback rate
for animations on page" }
],
+ "returns": [
+
{ "name": "animationStartTimes", "type": "array", "items":
{ "$ref": "AnimationStartTime" }, "description": "List of updated animation
start times." }
+ ],
"description": "Sets the playback rate of the document
timeline."
},
{
"name": "setCurrentTime",
"parameters": [
-
{ "name": "currentTime", "type": "number", "description": "Current time for
the page animation timeline" }
+
{ "name": "normalisedCurrentTime", "type": "number", "description": "Current
time for the page animation timeline" }
+ ],
+ "returns": [
+
{ "name": "animationStartTimes", "type": "array", "items":
{ "$ref": "AnimationStartTime" }, "description": "List of updated animation
start times." }
],
"description": "Sets the current time of the document
timeline."
},


sa...@chromium.org

unread,
Mar 31, 2015, 7:02:24 AM3/31/15
to dgo...@chromium.org, pfel...@chromium.org, aandre...@chromium.org, alexis...@intel.com, apavlo...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, caseq...@chromium.org, devtools...@chromium.org, dstoc...@chromium.org, ericwi...@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
On 2015/03/31 at 06:56:08, samli wrote:
> WIP.

> Incomplete tasks:
> - Check properly attached to all new incoming frames (and set playback
> rate on
them)
> - on didCommitLoadForLocalFrame(), clear maps appropriately - this is
> causing
an issue at the moment
> - Update start times when IAA::setCurrentTime() is called (just need to
> the
callback)

Complete now, PTAL. Lacking a test, haven't thought much about how to test
this
since it deals with lots of cross-frame stuff and monotonic time. Haven't
put
much thought into the testing yet.

https://codereview.chromium.org/1042143005/

sa...@chromium.org

unread,
Mar 31, 2015, 9:14:47 PM3/31/15
to dgo...@chromium.org, pfel...@chromium.org, aandre...@chromium.org, alexis...@intel.com, apavlo...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, caseq...@chromium.org, devtools...@chromium.org, dstoc...@chromium.org, ericwi...@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

pfel...@chromium.org

unread,
Apr 1, 2015, 9:06:46 AM4/1/15
to sa...@chromium.org, dgo...@chromium.org, aandre...@chromium.org, alexis...@intel.com, apavlo...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, caseq...@chromium.org, devtools...@chromium.org, dstoc...@chromium.org, ericwi...@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/1042143005/diff/40001/Source/core/inspector/InspectorAnimationAgent.cpp
File Source/core/inspector/InspectorAnimationAgent.cpp (right):

https://codereview.chromium.org/1042143005/diff/40001/Source/core/inspector/InspectorAnimationAgent.cpp#newcode177
Source/core/inspector/InspectorAnimationAgent.cpp:177: return
player.startTime() / player.timeline()->playbackRate() +
player.timeline()->zeroTime() * 1000;
What time is this?

https://codereview.chromium.org/1042143005/diff/40001/Source/core/inspector/InspectorAnimationAgent.cpp#newcode267
Source/core/inspector/InspectorAnimationAgent.cpp:267: for (Frame* frame
= m_pageAgent->inspectedFrame(); frame; frame =
frame->tree().traverseNext(m_pageAgent->inspectedFrame())) {
It is strange that this synchronization takes place in the inspector
code. It seems unrelated to inspector, some timelines belonging to
different documents could just stay in sync on their own. You also need
to sync the newly added frames and I don't see this code here.

https://codereview.chromium.org/1042143005/diff/40001/Source/devtools/front_end/elements/AnimationTimeline.js
File Source/devtools/front_end/elements/AnimationTimeline.js (right):

https://codereview.chromium.org/1042143005/diff/40001/Source/devtools/front_end/elements/AnimationTimeline.js#newcode50
Source/devtools/front_end/elements/AnimationTimeline.js:50:
target.animationAgent().setPlaybackRate(this._animationsPlaybackRate,
this._updateStartTimes.bind(this));
You should not be talking to the agents from outside the model. Model
should talk to agent and update its instances internally, you should not
need to use public methods to update model internals (updateStartTime).

https://codereview.chromium.org/1042143005/diff/40001/Source/devtools/protocol.json
File Source/devtools/protocol.json (right):

https://codereview.chromium.org/1042143005/diff/40001/Source/devtools/protocol.json#newcode4879
Source/devtools/protocol.json:4879: { "name": "id", "type": "string",
"description": "<code>AnimationPlayer</code>'s id." },
For the ids, we typically introduce a separate type "AnimationPlayerId"
in this case that inherits from string. Then we call parameters using
full qualified names such as animationPlayerId: AnimationPlayerId.

https://codereview.chromium.org/1042143005/diff/40001/Source/devtools/protocol.json#newcode4880
Source/devtools/protocol.json:4880: { "name": "startTime", "type":
"number", "description": "Updated normalized
<code>AnimationPlayer</code>'s start time." }
Is this monotonicallyIncreasingTime or wall?

https://codereview.chromium.org/1042143005/diff/40001/Source/devtools/protocol.json#newcode4922
Source/devtools/protocol.json:4922: { "name": "normalisedCurrentTime",
"type": "number", "description": "Current time for the page animation
timeline" }
normalizedCurrentTime

https://codereview.chromium.org/1042143005/

sa...@chromium.org

unread,
Apr 2, 2015, 5:58:00 AM4/2/15
to pfel...@chromium.org, aandre...@chromium.org, alexis...@intel.com, apavlo...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, caseq...@chromium.org, devtools...@chromium.org, dstoc...@chromium.org, ericwi...@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/1042143005/diff/40001/Source/core/inspector/InspectorAnimationAgent.cpp
File Source/core/inspector/InspectorAnimationAgent.cpp (right):

https://codereview.chromium.org/1042143005/diff/40001/Source/core/inspector/InspectorAnimationAgent.cpp#newcode177
Source/core/inspector/InspectorAnimationAgent.cpp:177: return
player.startTime() / player.timeline()->playbackRate() +
player.timeline()->zeroTime() * 1000;
On 2015/04/01 at 13:06:45, pfeldman wrote:
> What time is this?

We take relative timeline times and convert them to monotonic times.

https://codereview.chromium.org/1042143005/diff/40001/Source/core/inspector/InspectorAnimationAgent.cpp#newcode267
Source/core/inspector/InspectorAnimationAgent.cpp:267: for (Frame* frame
= m_pageAgent->inspectedFrame(); frame; frame =
frame->tree().traverseNext(m_pageAgent->inspectedFrame())) {
On 2015/04/01 at 13:06:45, pfeldman wrote:
> It is strange that this synchronization takes place in the inspector
code. It seems unrelated to inspector, some timelines belonging to
different documents could just stay in sync on their own. You also need
to sync the newly added frames and I don't see this code here.

This code isn't to sync timelines. This code is used to seek timelines.
For example, when 'replay' is hit or the scrubber is dragged, we want to
seek back in time. Instead of maintaining all timelines separately, the
front-end only understands monotonic time values. It then says: "take me
back to time x", which setCurrentTime() then seeks each animation
timeline individually (converting monotonic time back into timeline
time).

New frames are automatically captured in this. New frames have their
timeline playback rate set in the frontend.

https://codereview.chromium.org/1042143005/diff/40001/Source/devtools/front_end/elements/AnimationTimeline.js
File Source/devtools/front_end/elements/AnimationTimeline.js (right):

https://codereview.chromium.org/1042143005/diff/40001/Source/devtools/front_end/elements/AnimationTimeline.js#newcode50
Source/devtools/front_end/elements/AnimationTimeline.js:50:
target.animationAgent().setPlaybackRate(this._animationsPlaybackRate,
this._updateStartTimes.bind(this));
On 2015/04/01 at 13:06:45, pfeldman wrote:
> You should not be talking to the agents from outside the model. Model
should talk to agent and update its instances internally, you should not
need to use public methods to update model internals (updateStartTime).

Done.

https://codereview.chromium.org/1042143005/diff/40001/Source/devtools/protocol.json
File Source/devtools/protocol.json (right):

https://codereview.chromium.org/1042143005/diff/40001/Source/devtools/protocol.json#newcode4879
Source/devtools/protocol.json:4879: { "name": "id", "type": "string",
"description": "<code>AnimationPlayer</code>'s id." },
On 2015/04/01 at 13:06:45, pfeldman wrote:
> For the ids, we typically introduce a separate type
"AnimationPlayerId" in this case that inherits from string. Then we call
parameters using full qualified names such as animationPlayerId:
AnimationPlayerId.

Done.

https://codereview.chromium.org/1042143005/diff/40001/Source/devtools/protocol.json#newcode4880
Source/devtools/protocol.json:4880: { "name": "startTime", "type":
"number", "description": "Updated normalized
<code>AnimationPlayer</code>'s start time." }
On 2015/04/01 at 13:06:45, pfeldman wrote:
> Is this monotonicallyIncreasingTime or wall?

Monotonically increasing time.

https://codereview.chromium.org/1042143005/diff/40001/Source/devtools/protocol.json#newcode4922
Source/devtools/protocol.json:4922: { "name": "normalisedCurrentTime",
"type": "number", "description": "Current time for the page animation
timeline" }
On 2015/04/01 at 13:06:45, pfeldman wrote:
> normalizedCurrentTime

Done.

https://codereview.chromium.org/1042143005/

pfel...@chromium.org

unread,
Apr 2, 2015, 10:42:38 AM4/2/15
to sa...@chromium.org, aandre...@chromium.org, alexis...@intel.com, apavlo...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, caseq...@chromium.org, devtools...@chromium.org, dstoc...@chromium.org, ericwi...@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/1042143005/diff/40001/Source/core/inspector/InspectorAnimationAgent.cpp
File Source/core/inspector/InspectorAnimationAgent.cpp (right):

https://codereview.chromium.org/1042143005/diff/40001/Source/core/inspector/InspectorAnimationAgent.cpp#newcode177
Source/core/inspector/InspectorAnimationAgent.cpp:177: return
player.startTime() / player.timeline()->playbackRate() +
player.timeline()->zeroTime() * 1000;
On 2015/04/02 09:58:00, samli wrote:
> On 2015/04/01 at 13:06:45, pfeldman wrote:
> > What time is this?

> We take relative timeline times and convert them to monotonic times.

But zeroTime is already monotonicTime, why do multiply it by 1000?

https://codereview.chromium.org/1042143005/diff/40001/Source/core/inspector/InspectorAnimationAgent.cpp#newcode267
Source/core/inspector/InspectorAnimationAgent.cpp:267: for (Frame* frame
= m_pageAgent->inspectedFrame(); frame; frame =
frame->tree().traverseNext(m_pageAgent->inspectedFrame())) {
On 2015/04/02 09:58:00, samli wrote:
> On 2015/04/01 at 13:06:45, pfeldman wrote:
> > It is strange that this synchronization takes place in the inspector
code. It
> seems unrelated to inspector, some timelines belonging to different
documents
> could just stay in sync on their own. You also need to sync the newly
added
> frames and I don't see this code here.

> This code isn't to sync timelines. This code is used to seek
timelines. For
> example, when 'replay' is hit or the scrubber is dragged, we want to
seek back
> in time. Instead of maintaining all timelines separately, the
front-end only
> understands monotonic time values. It then says: "take me back to time
x", which
> setCurrentTime() then seeks each animation timeline individually
(converting
> monotonic time back into timeline time).

> New frames are automatically captured in this. New frames have their
timeline
> playback rate set in the frontend.

Ok, I think I understand.

https://codereview.chromium.org/1042143005/diff/40001/Source/core/inspector/InspectorAnimationAgent.cpp#newcode270
Source/core/inspector/InspectorAnimationAgent.cpp:270:
timeline.setCurrentTime((normalisedCurrentTime - timeline.zeroTime() *
1000) * timeline.playbackRate());
There is something wrong with the timeline units: you can't do
timeline.setCurrentTime(foo - timeline.zeroTime() * 1000). You should
stick to one scale within one timeline.

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

https://codereview.chromium.org/1042143005/diff/80001/Source/core/inspector/InspectorAnimationAgent.cpp#newcode217
Source/core/inspector/InspectorAnimationAgent.cpp:217:
.setStartTime(normalizedStartTime(player))
Different time scales is confusing, if you are using different scales,
properties should have corresponding suffixes.

https://codereview.chromium.org/1042143005/diff/80001/Source/devtools/front_end/elements/AnimationTimeline.js
File Source/devtools/front_end/elements/AnimationTimeline.js (right):

https://codereview.chromium.org/1042143005/diff/80001/Source/devtools/front_end/elements/AnimationTimeline.js#newcode92
Source/devtools/front_end/elements/AnimationTimeline.js:92: if
(!startTimes || !startTimes.length)
Why is it nullable?

https://codereview.chromium.org/1042143005/diff/80001/Source/devtools/front_end/elements/AnimationTimeline.js#newcode99
Source/devtools/front_end/elements/AnimationTimeline.js:99:
this._startTime = player.startTime;
I don't understand how multiple players within multiple frames
synchronize. Why is this different from simply several players?

https://codereview.chromium.org/1042143005/

sa...@chromium.org

unread,
Apr 2, 2015, 4:13:19 PM4/2/15
to 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, dstoc...@chromium.org, ericwi...@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
+dstockwell for animation related changes


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

https://codereview.chromium.org/1042143005/diff/40001/Source/core/inspector/InspectorAnimationAgent.cpp#newcode177
Source/core/inspector/InspectorAnimationAgent.cpp:177: return
player.startTime() / player.timeline()->playbackRate() +
player.timeline()->zeroTime() * 1000;
On 2015/04/02 14:42:38, pfeldman wrote:
> On 2015/04/02 09:58:00, samli wrote:
> > On 2015/04/01 at 13:06:45, pfeldman wrote:
> > > What time is this?
> >
> > We take relative timeline times and convert them to monotonic times.

> But zeroTime is already monotonicTime, why do multiply it by 1000?

Chrome's internal representations are in seconds. Public Web Animations
API uses milliseconds. As such, front end only understands milliseconds.

To clarify: zeroTime() is in seconds, startTime() is milliseconds since
it's a public API.

https://codereview.chromium.org/1042143005/diff/40001/Source/core/inspector/InspectorAnimationAgent.cpp#newcode270
Source/core/inspector/InspectorAnimationAgent.cpp:270:
timeline.setCurrentTime((normalisedCurrentTime - timeline.zeroTime() *
1000) * timeline.playbackRate());
On 2015/04/02 14:42:38, pfeldman wrote:
> There is something wrong with the timeline units: you can't do
> timeline.setCurrentTime(foo - timeline.zeroTime() * 1000). You should
stick to
> one scale within one timeline.

I think this is the milliseconds confusion?

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

https://codereview.chromium.org/1042143005/diff/80001/Source/core/inspector/InspectorAnimationAgent.cpp#newcode217
Source/core/inspector/InspectorAnimationAgent.cpp:217:
.setStartTime(normalizedStartTime(player))
On 2015/04/02 14:42:38, pfeldman wrote:
> Different time scales is confusing, if you are using different scales,
> properties should have corresponding suffixes.

Will fix

https://codereview.chromium.org/1042143005/diff/80001/Source/devtools/front_end/elements/AnimationTimeline.js
File Source/devtools/front_end/elements/AnimationTimeline.js (right):

https://codereview.chromium.org/1042143005/diff/80001/Source/devtools/front_end/elements/AnimationTimeline.js#newcode92
Source/devtools/front_end/elements/AnimationTimeline.js:92: if
(!startTimes || !startTimes.length)
On 2015/04/02 14:42:38, pfeldman wrote:
> Why is it nullable?

In the case of protocol error it's called with null

https://codereview.chromium.org/1042143005/diff/80001/Source/devtools/front_end/elements/AnimationTimeline.js#newcode99
Source/devtools/front_end/elements/AnimationTimeline.js:99:
this._startTime = player.startTime;
On 2015/04/02 14:42:38, pfeldman wrote:
> I don't understand how multiple players within multiple frames
synchronize. Why
> is this different from simply several players?

All animation players have times calculated based on their attached
animation timeline which in turn are based on monotonic time. Ie: a
player can return a start time of "2000" which means it started 2000ms
after the timeline started. We can compare players in one Crame and have
been doing so for a while. However, we can't compare players in separate
frames (timelines) without factoring in the timeline start times, which
is what we're doing in this patch.

https://codereview.chromium.org/1042143005/

pfel...@chromium.org

unread,
Apr 2, 2015, 4:25:19 PM4/2/15
to sa...@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, dstoc...@chromium.org, ericwi...@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
Can we make all the time that goes through the protocol monotonic?

https://codereview.chromium.org/1042143005/

sa...@chromium.org

unread,
Apr 2, 2015, 5:50:37 PM4/2/15
to dstoc...@chromium.org, pfel...@chromium.org, aandre...@chromium.org, alexis...@intel.com, apavlo...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, caseq...@chromium.org, devtools...@chromium.org, dstoc...@chromium.org, ericwi...@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
That's what this patch is :)

https://codereview.chromium.org/1042143005/

pfel...@chromium.org

unread,
Apr 3, 2015, 5:19:41 AM4/3/15
to sa...@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, dstoc...@chromium.org, ericwi...@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/1042143005/diff/80001/Source/devtools/protocol.json
File Source/devtools/protocol.json (right):

https://codereview.chromium.org/1042143005/diff/80001/Source/devtools/protocol.json#newcode4834
Source/devtools/protocol.json:4834: { "name": "startTime", "type":
"number", "description": "<code>AnimationPlayer</code>'s start time." },
Is this monotonicallyIncreasing time?

https://codereview.chromium.org/1042143005/diff/80001/Source/devtools/protocol.json#newcode4835
Source/devtools/protocol.json:4835: { "name": "currentTime", "type":
"number", "description": "<code>AnimationPlayer</code>'s current time."
},
Is this monotonicallyIncreasing time?

https://codereview.chromium.org/1042143005/diff/80001/Source/devtools/protocol.json#newcode4885
Source/devtools/protocol.json:4885: { "name": "startTime", "type":
"number", "description": "Updated normalized
<code>AnimationPlayer</code>'s start time." }
Is this monotonicallyIncreasing time?

https://codereview.chromium.org/1042143005/diff/80001/Source/devtools/protocol.json#newcode4927
Source/devtools/protocol.json:4927: { "name": "normalizedCurrentTime",
"type": "number", "description": "Current time for the page animation
timeline" }
Is this monotonicallyIncreasing time?

https://codereview.chromium.org/1042143005/

sa...@chromium.org

unread,
Apr 3, 2015, 5:56:01 AM4/3/15
to dstoc...@chromium.org, pfel...@chromium.org, aandre...@chromium.org, alexis...@intel.com, apavlo...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, caseq...@chromium.org, devtools...@chromium.org, dstoc...@chromium.org, ericwi...@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/1042143005/diff/80001/Source/devtools/protocol.json
File Source/devtools/protocol.json (right):

https://codereview.chromium.org/1042143005/diff/80001/Source/devtools/protocol.json#newcode4834
Source/devtools/protocol.json:4834: { "name": "startTime", "type":
"number", "description": "<code>AnimationPlayer</code>'s start time." },
On 2015/04/03 09:19:41, pfeldman wrote:
> Is this monotonicallyIncreasing time?

Yes.

https://codereview.chromium.org/1042143005/diff/80001/Source/devtools/protocol.json#newcode4835
Source/devtools/protocol.json:4835: { "name": "currentTime", "type":
"number", "description": "<code>AnimationPlayer</code>'s current time."
},
On 2015/04/03 09:19:41, pfeldman wrote:
> Is this monotonicallyIncreasing time?

This is always relative to the player. It's start time + current time

https://codereview.chromium.org/1042143005/diff/80001/Source/devtools/protocol.json#newcode4885
Source/devtools/protocol.json:4885: { "name": "startTime", "type":
"number", "description": "Updated normalized
<code>AnimationPlayer</code>'s start time." }
On 2015/04/03 09:19:41, pfeldman wrote:
> Is this monotonicallyIncreasing time?

Yes.

https://codereview.chromium.org/1042143005/diff/80001/Source/devtools/protocol.json#newcode4927
Source/devtools/protocol.json:4927: { "name": "normalizedCurrentTime",
"type": "number", "description": "Current time for the page animation
timeline" }
On 2015/04/03 09:19:41, pfeldman wrote:
> Is this monotonicallyIncreasing time?

Yes

https://codereview.chromium.org/1042143005/

pfel...@chromium.org

unread,
Apr 3, 2015, 6:36:50 AM4/3/15
to sa...@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, dstoc...@chromium.org, ericwi...@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/1042143005/diff/80001/Source/devtools/protocol.json#newcode4835
Source/devtools/protocol.json:4835: { "name": "currentTime", "type":
"number", "description": "<code>AnimationPlayer</code>'s current time."
},
Lets rename it to currentOffset?

https://codereview.chromium.org/1042143005/diff/80001/Source/devtools/protocol.json#newcode4885
Source/devtools/protocol.json:4885: { "name": "startTime", "type":
"number", "description": "Updated normalized
<code>AnimationPlayer</code>'s start time." }
On 2015/04/03 09:56:00, samli wrote:
> On 2015/04/03 09:19:41, pfeldman wrote:
> > Is this monotonicallyIncreasing time?

> Yes.

Why is it called normalized then?

https://codereview.chromium.org/1042143005/diff/80001/Source/devtools/protocol.json#newcode4920
Source/devtools/protocol.json:4920: { "name": "animationStartTimes",
"type": "array", "items": { "$ref": "AnimationStartTime" },
"description": "List of updated animation start times." }
So why do you want to collect all start times upon setPlaybackRate and
setCurrentTimes? When does the start time of the animation player
change?

https://codereview.chromium.org/1042143005/

sa...@chromium.org

unread,
Apr 3, 2015, 7:31:32 AM4/3/15
to dstoc...@chromium.org, pfel...@chromium.org, aandre...@chromium.org, alexis...@intel.com, apavlo...@chromium.org, blink-...@chromium.org, blink-revie...@chromium.org, caseq...@chromium.org, devtools...@chromium.org, dstoc...@chromium.org, ericwi...@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/1042143005/diff/80001/Source/devtools/protocol.json
File Source/devtools/protocol.json (right):

https://codereview.chromium.org/1042143005/diff/80001/Source/devtools/protocol.json#newcode4835
Source/devtools/protocol.json:4835: { "name": "currentTime", "type":
"number", "description": "<code>AnimationPlayer</code>'s current time."
},
On 2015/04/03 10:36:49, pfeldman wrote:
> Lets rename it to currentOffset?

No, this matches the Web Animations spec.
https://w3c.github.io/web-animations/

https://codereview.chromium.org/1042143005/diff/80001/Source/devtools/protocol.json#newcode4885
Source/devtools/protocol.json:4885: { "name": "startTime", "type":
"number", "description": "Updated normalized
<code>AnimationPlayer</code>'s start time." }
On 2015/04/03 10:36:49, pfeldman wrote:
> On 2015/04/03 09:56:00, samli wrote:
> > On 2015/04/03 09:19:41, pfeldman wrote:
> > > Is this monotonicallyIncreasing time?
> >
> > Yes.

> Why is it called normalized then?

Normalise means to "bring to a standard". We convert relative timeline
times to a standard interface of time. I think it's a reasonable name.

https://codereview.chromium.org/1042143005/diff/80001/Source/devtools/protocol.json#newcode4920
Source/devtools/protocol.json:4920: { "name": "animationStartTimes",
"type": "array", "items": { "$ref": "AnimationStartTime" },
"description": "List of updated animation start times." }
On 2015/04/03 10:36:49, pfeldman wrote:
> So why do you want to collect all start times upon setPlaybackRate and
> setCurrentTimes? When does the start time of the animation player
change?

Conceptual start time is always relative to timeline time. When we
manipulate playback rate of a timeline the current time of the timeline
does not change. Old monotonic values are invalidated since they cannot
be converted back to timeline times since the timeline 'scaled' relative
to monotonic time.

When seeking a timeline, we are effectively shifting the timeline
relative to monotonic time. This invalidates the start times we sent
earlier.

In both cases, the original monotonic start times no longer reference
the right time in the timelines so we need to update the start times.

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