Devtools Animations: Show subtree animations for a given node

Devtools Animations: Show subtree animations for a given node

Animations are shown for the selected element making it difficult to
find and navigate between animations. This change introduces a persisted
checkbox setting to show all animations for the selected node's DOM tree.
When hovering on an animation section title the animating element on the
page is now highlighted. Clicking on the title to expand/collapse an
animation section is also enabled. The subtree setting is turned on by
default. This change is behind the Animation Inspection experiment.

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

[samli, 2014-10-29 05:48:14]

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

[samli, 2014-10-29 05:48:14]

A video demo of this: http://youtu.be/cp_wJvLAnVI
Other demo animation pages:
http://web-animations.github.io/web-animations-demos/#starfield

[samli, 2014-11-03 18:00:51]

PTAL

[vsevik, 2014-11-05 09:48:37]

https://codereview.chromium.org/682423002/diff/20001/Source/core/inspector/In...
File Source/core/inspector/InspectorDOMAgent.h (right):

https://codereview.chromium.org/682423002/diff/20001/Source/core/inspector/In...
Source/core/inspector/InspectorDOMAgent.h:190: int
pushNodePathToFrontend(Node*);
You shouldn't do this.
Pushing node to front-end is expensive and we should only do this when
necessary. 
We have a special type of NodeId called BackendNodeId that is used for nodes
that were not pushed to front-end yet and you could use them here.

InspectorNodeIds::idForNode(node) allow you to get BackendNodeId.
DOM.pushNodesByBackendIdsToFrontend protocol method allows you to request
pushing of given nodes to front-end.

You could see LayerTreeModel.js / InspectorLayerTreeAgent for reference.

https://codereview.chromium.org/682423002/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/elements/AnimationsSidebarPane.js (right):

https://codereview.chromium.org/682423002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/elements/AnimationsSidebarPane.js:65:
this._animationSections[i] = new WebInspector.AnimationSection(this,
this._stylesPane, player);
Is there any specific order of the animations here?
We should probably group them by Element at least.

[samli, 2014-11-10 23:59:31]

PTAL

https://codereview.chromium.org/682423002/diff/20001/Source/core/inspector/In...
File Source/core/inspector/InspectorDOMAgent.h (right):

https://codereview.chromium.org/682423002/diff/20001/Source/core/inspector/In...
Source/core/inspector/InspectorDOMAgent.h:190: int
pushNodePathToFrontend(Node*);
On 2014/11/05 09:48:37, vsevik wrote:
> You shouldn't do this.
> Pushing node to front-end is expensive and we should only do this when
> necessary. 
> We have a special type of NodeId called BackendNodeId that is used for nodes
> that were not pushed to front-end yet and you could use them here.
> 
> InspectorNodeIds::idForNode(node) allow you to get BackendNodeId.
> DOM.pushNodesByBackendIdsToFrontend protocol method allows you to request
> pushing of given nodes to front-end.
> 
> You could see LayerTreeModel.js / InspectorLayerTreeAgent for reference.

I've updated as suggested to use a similar behavior of resolving the node from
the front-end.

https://codereview.chromium.org/682423002/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/elements/AnimationsSidebarPane.js (right):

https://codereview.chromium.org/682423002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/elements/AnimationsSidebarPane.js:65:
this._animationSections[i] = new WebInspector.AnimationSection(this,
this._stylesPane, player);
On 2014/11/05 09:48:37, vsevik wrote:
> Is there any specific order of the animations here?
> We should probably group them by Element at least.

Currently they're sorted by sequence number, which is the order in which they
started. This should be sufficient for now. It seems unlikely that there are
many animations targeting the same element. In the case that is true, they were
likely started at the same time and will appear grouped anyway.

[vsevik, 2014-11-12 09:14:36]

https://codereview.chromium.org/682423002/diff/40001/Source/devtools/front_en...
File Source/devtools/front_end/common/Settings.js (right):

https://codereview.chromium.org/682423002/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/common/Settings.js:94: this.showSubtreeAnimations =
this.createSetting("showSubtreeAnimations", true);
Let's create this setting inside animation sidebar pane. It will hardly be used
outside of it anyway (e.g. I don't think we would want to show it in settings).

https://codereview.chromium.org/682423002/diff/40001/Source/devtools/front_en...
File Source/devtools/front_end/elements/AnimationsSidebarPane.js (right):

https://codereview.chromium.org/682423002/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/elements/AnimationsSidebarPane.js:127: expanded:
function()
Can we make it private?

https://codereview.chromium.org/682423002/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/elements/AnimationsSidebarPane.js:132: toggle:
function()
Can we make it private?

https://codereview.chromium.org/682423002/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/elements/AnimationsSidebarPane.js:142: if (expand) {
this.bodyElement.classList.toggle("expanded", expanded);
if (expanded)
    this.updateCurrentTime();

if (expanded) check is actually redundant since you do it inside this method
anyway.

https://codereview.chromium.org/682423002/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/elements/AnimationsSidebarPane.js:152: if
(this.expanded)
this.expanded is a function, it is always true :)

https://codereview.chromium.org/682423002/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/elements/AnimationsSidebarPane.js:222:
headerElement.addEventListener("mouseleave",
node.domModel().hideDOMNodeHighlight.bind(node.domModel()), false);
I think we should also add a node description with a link to reveal it.
You need to run something like
WebInspector.DOMPresentationUtils.linkifyNodeReference(node) to achieve that.

https://codereview.chromium.org/682423002/diff/40001/Source/devtools/front_en...
File Source/devtools/front_end/elements/StylesSidebarPane.js (right):

https://codereview.chromium.org/682423002/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/elements/StylesSidebarPane.js:582:
this._target.animationModel.animationPlayers(node.id, false,
animationPlayersCallback.bind(this));
getAnimationPlayers, see below.

https://codereview.chromium.org/682423002/diff/40001/Source/devtools/front_en...
File Source/devtools/front_end/sdk/AnimationModel.js (left):

https://codereview.chromium.org/682423002/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/sdk/AnimationModel.js:199: console.error(error);
Why did you remove this?
You should also remove brackets.

https://codereview.chromium.org/682423002/diff/40001/Source/devtools/front_en...
File Source/devtools/front_end/sdk/AnimationModel.js (right):

https://codereview.chromium.org/682423002/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/sdk/AnimationModel.js:49:
this._nodeIdToCallbackData[nodeId].push(userCallback);
Now that we could have different types of such requests for the same node this
seems wrong.

https://codereview.chromium.org/682423002/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/sdk/AnimationModel.js:298: node: function(callback)
We usually use get prefix for asynchronous getters.
Let's name it getNode or resolveNode.

[samli, 2014-11-13 01:12:44]

PTAL

https://codereview.chromium.org/682423002/diff/40001/Source/devtools/front_en...
File Source/devtools/front_end/common/Settings.js (right):

https://codereview.chromium.org/682423002/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/common/Settings.js:94: this.showSubtreeAnimations =
this.createSetting("showSubtreeAnimations", true);
On 2014/11/12 09:14:35, vsevik wrote:
> Let's create this setting inside animation sidebar pane. It will hardly be
used
> outside of it anyway (e.g. I don't think we would want to show it in
settings).

Done.

https://codereview.chromium.org/682423002/diff/40001/Source/devtools/front_en...
File Source/devtools/front_end/elements/AnimationsSidebarPane.js (right):

https://codereview.chromium.org/682423002/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/elements/AnimationsSidebarPane.js:127: expanded:
function()
On 2014/11/12 09:14:35, vsevik wrote:
> Can we make it private?

Done.

https://codereview.chromium.org/682423002/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/elements/AnimationsSidebarPane.js:132: toggle:
function()
On 2014/11/12 09:14:35, vsevik wrote:
> Can we make it private?

Done.

https://codereview.chromium.org/682423002/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/elements/AnimationsSidebarPane.js:142: if (expand) {
On 2014/11/12 09:14:35, vsevik wrote:
> this.bodyElement.classList.toggle("expanded", expanded);
> if (expanded)
>     this.updateCurrentTime();
> 
> if (expanded) check is actually redundant since you do it inside this method
> anyway.

Done.

https://codereview.chromium.org/682423002/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/elements/AnimationsSidebarPane.js:152: if
(this.expanded)
On 2014/11/12 09:14:35, vsevik wrote:
> this.expanded is a function, it is always true :)

Done.

https://codereview.chromium.org/682423002/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/elements/AnimationsSidebarPane.js:222:
headerElement.addEventListener("mouseleave",
node.domModel().hideDOMNodeHighlight.bind(node.domModel()), false);
On 2014/11/12 09:14:35, vsevik wrote:
> I think we should also add a node description with a link to reveal it.
> You need to run something like
> WebInspector.DOMPresentationUtils.linkifyNodeReference(node) to achieve that.

I agree, but I think we need to agree on a UI for this and put it in a separate
patch. crbug.com/432743

https://codereview.chromium.org/682423002/diff/40001/Source/devtools/front_en...
File Source/devtools/front_end/elements/StylesSidebarPane.js (right):

https://codereview.chromium.org/682423002/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/elements/StylesSidebarPane.js:582:
this._target.animationModel.animationPlayers(node.id, false,
animationPlayersCallback.bind(this));
On 2014/11/12 09:14:35, vsevik wrote:
> getAnimationPlayers, see below.

Done.

https://codereview.chromium.org/682423002/diff/40001/Source/devtools/front_en...
File Source/devtools/front_end/sdk/AnimationModel.js (left):

https://codereview.chromium.org/682423002/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/sdk/AnimationModel.js:199: console.error(error);
On 2014/11/12 09:14:35, vsevik wrote:
> Why did you remove this?
> You should also remove brackets.

When the selected node changes, the list of animation players in the
InspectorAnimationAgent is discarded. However, the current time updater in the
animation player is still polling for changes. A protocol error then results
where the animation player can't be found.

https://codereview.chromium.org/682423002/diff/40001/Source/devtools/front_en...
File Source/devtools/front_end/sdk/AnimationModel.js (right):

https://codereview.chromium.org/682423002/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/sdk/AnimationModel.js:49:
this._nodeIdToCallbackData[nodeId].push(userCallback);
On 2014/11/12 09:14:35, vsevik wrote:
> Now that we could have different types of such requests for the same node this
> seems wrong.

I've removed this behavior.

https://codereview.chromium.org/682423002/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/sdk/AnimationModel.js:298: node: function(callback)
On 2014/11/12 09:14:35, vsevik wrote:
> We usually use get prefix for asynchronous getters.
> Let's name it getNode or resolveNode.

Done.

[vsevik, 2014-11-13 20:51:40]

lgtm

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

https://codereview.chromium.org/682423002/diff/60001/Source/devtools/front_en...
Source/devtools/front_end/elements/AnimationsSidebarPane.js:15:
this.showSubtreeSetting =
WebInspector.settings.createSetting("showSubtreeAnimations", true);
private?

[samli, 2014-11-13 23:11:54]

The CQ bit was checked by samli@chromium.org

[commit-bot: I haz the power, 2014-11-13 23:12:16]

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

[commit-bot: I haz the power, 2014-11-14 00:18:37]

Committed patchset #5 (id:80001) as 185333