Devtools Animations: Display keyframes for CSS Animations in inspector

Devtools Animations: Display keyframes for CSS Animations in inspector

This change displays CSS animation keyframes alongside the associated
animation player in the Animations sidebar pane. No visible changes are
introduced except with the hidden 'Animation Inspection' experiment enabled.

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

[samli, 2014-10-20 05:07:12]

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

[samli, 2014-10-22 11:12:25]

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

[samli, 2014-10-23 06:06:42]

@vsevik: Can you please take a look?

[pfeldman, 2014-10-23 06:36:59]

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

[pfeldman, 2014-10-23 06:37:00]

lgtm given comments are addressed.

https://codereview.chromium.org/664993002/diff/1/Source/core/animation/css/CS...
File Source/core/animation/css/CSSAnimations.h (right):

https://codereview.chromium.org/664993002/diff/1/Source/core/animation/css/CS...
Source/core/animation/css/CSSAnimations.h:170: const AtomicString*
getAnimationNameForInspector(const AnimationPlayer&);
Lets return a copy, it is not expensive, while this object lifetime might be
non-trivial.

https://codereview.chromium.org/664993002/diff/1/Source/core/inspector/Inspec...
File Source/core/inspector/InspectorAnimationAgent.cpp (right):

https://codereview.chromium.org/664993002/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorAnimationAgent.cpp:56: animationPlayerObject =
buildObjectForAnimationPlayer(player, keyframeRule);
keyframeRule.release()

https://codereview.chromium.org/664993002/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorAnimationAgent.cpp:117: return
buildObjectForAnimationPlayer(animationPlayer, nullptr);
inline and simplify code above?

https://codereview.chromium.org/664993002/diff/1/Source/core/inspector/Inspec...
File Source/core/inspector/InspectorAnimationAgent.h (right):

https://codereview.chromium.org/664993002/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorAnimationAgent.h:50:
PassRefPtr<TypeBuilder::Animation::AnimationPlayer>
buildObjectForAnimationPlayer(AnimationPlayer&,
PassRefPtr<TypeBuilder::Animation::KeyframesRule>);
static builders are typically defined using non-class static functions within
.cpp.

https://codereview.chromium.org/664993002/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorAnimationAgent.h:53:
PassRefPtr<TypeBuilder::Animation::KeyframesRule>
getObjectForStyleRuleKeyframes(const Element&, const AnimationPlayer&);
There are no "get" prefixes in blink. The only ones allowed are the ones from
the protocol.json and the ones with output params.

https://codereview.chromium.org/664993002/diff/1/Source/devtools/front_end/sd...
File Source/devtools/front_end/sdk/AnimationModel.js (right):

https://codereview.chromium.org/664993002/diff/1/Source/devtools/front_end/sd...
Source/devtools/front_end/sdk/AnimationModel.js:284: this._keyframes =
this._payload.keyframes.map(function (keyframeStyle) {
Please extract as a named function w/ annotation below.

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

https://codereview.chromium.org/664993002/diff/1/Source/devtools/protocol.jso...
Source/devtools/protocol.json:4455: "hidden": "true",
No need to hide entities within hidden domain.

https://codereview.chromium.org/664993002/diff/1/Source/devtools/protocol.jso...
Source/devtools/protocol.json:4457: { "name": "name", "type": "string",
"optional": "true", "description": "CSS keyframed animation's name." },
true is a boolean, not string.

https://codereview.chromium.org/664993002/diff/1/Source/devtools/protocol.jso...
Source/devtools/protocol.json:4465: "hidden": "true",
ditto - no need and a boolean.

[vsevik, 2014-10-23 14:52:11]

Keyframe rules have grey background which makes them look like they are not a
part of the animation above. I think they should be white as well as animation
section.

[samli, 2014-10-24 05:06:20]

Addressed review comments.

Keyframe rules now have a white background.

https://codereview.chromium.org/664993002/diff/1/Source/core/animation/css/CS...
File Source/core/animation/css/CSSAnimations.h (right):

https://codereview.chromium.org/664993002/diff/1/Source/core/animation/css/CS...
Source/core/animation/css/CSSAnimations.h:170: const AtomicString*
getAnimationNameForInspector(const AnimationPlayer&);
On 2014/10/23 06:37:00, pfeldman wrote:
> Lets return a copy, it is not expensive, while this object lifetime might be
> non-trivial.

Done.

https://codereview.chromium.org/664993002/diff/1/Source/core/inspector/Inspec...
File Source/core/inspector/InspectorAnimationAgent.cpp (right):

https://codereview.chromium.org/664993002/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorAnimationAgent.cpp:56: animationPlayerObject =
buildObjectForAnimationPlayer(player, keyframeRule);
On 2014/10/23 06:37:00, pfeldman wrote:
> keyframeRule.release()

Done.

https://codereview.chromium.org/664993002/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorAnimationAgent.cpp:117: return
buildObjectForAnimationPlayer(animationPlayer, nullptr);
On 2014/10/23 06:37:00, pfeldman wrote:
> inline and simplify code above?

Done.

https://codereview.chromium.org/664993002/diff/1/Source/core/inspector/Inspec...
File Source/core/inspector/InspectorAnimationAgent.h (right):

https://codereview.chromium.org/664993002/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorAnimationAgent.h:50:
PassRefPtr<TypeBuilder::Animation::AnimationPlayer>
buildObjectForAnimationPlayer(AnimationPlayer&,
PassRefPtr<TypeBuilder::Animation::KeyframesRule>);
On 2014/10/23 06:37:00, pfeldman wrote:
> static builders are typically defined using non-class static functions within
> .cpp.

Done.

https://codereview.chromium.org/664993002/diff/1/Source/core/inspector/Inspec...
Source/core/inspector/InspectorAnimationAgent.h:53:
PassRefPtr<TypeBuilder::Animation::KeyframesRule>
getObjectForStyleRuleKeyframes(const Element&, const AnimationPlayer&);
On 2014/10/23 06:37:00, pfeldman wrote:
> There are no "get" prefixes in blink. The only ones allowed are the ones from
> the protocol.json and the ones with output params.

Done.

https://codereview.chromium.org/664993002/diff/1/Source/devtools/front_end/sd...
File Source/devtools/front_end/sdk/AnimationModel.js (right):

https://codereview.chromium.org/664993002/diff/1/Source/devtools/front_end/sd...
Source/devtools/front_end/sdk/AnimationModel.js:284: this._keyframes =
this._payload.keyframes.map(function (keyframeStyle) {
On 2014/10/23 06:37:00, pfeldman wrote:
> Please extract as a named function w/ annotation below.

Done.

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

https://codereview.chromium.org/664993002/diff/1/Source/devtools/protocol.jso...
Source/devtools/protocol.json:4455: "hidden": "true",
On 2014/10/23 06:37:00, pfeldman wrote:
> No need to hide entities within hidden domain.

Done.

https://codereview.chromium.org/664993002/diff/1/Source/devtools/protocol.jso...
Source/devtools/protocol.json:4457: { "name": "name", "type": "string",
"optional": "true", "description": "CSS keyframed animation's name." },
On 2014/10/23 06:37:00, pfeldman wrote:
> true is a boolean, not string.

Done.

https://codereview.chromium.org/664993002/diff/1/Source/devtools/protocol.jso...
Source/devtools/protocol.json:4465: "hidden": "true",
On 2014/10/23 06:37:00, pfeldman wrote:
> ditto - no need and a boolean.

Done.

[samli, 2014-10-24 05:06:24]

The CQ bit was checked by samli@chromium.org

[commit-bot: I haz the power, 2014-10-24 05:06:58]

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

[commit-bot: I haz the power, 2014-10-24 05:51:19]

Committed patchset #2 (id:20001) as 184328