1568874 - Make the Animation Inspector able to display animations running on any selected node, even when those nodes are in oop iframes

Status: RESOLVED FIXED

(DevTools :: Inspector: Animations, enhancement, P1)

Description

[Patrick Brosset <:pbro>]

With fission, and once bug 1560200 is fixed, we'll be able to select nodes in the inspector that may live in out of process iframes.

When this happens, the Animation Inspector must continue being able to display all of the active animations that run on the selected node and node's subtree.

Comment 1

[Patrick Brosset <:pbro>]

One call site in particular will need to be updated:

devtools/client/inspector/animation/animation.js line 312
return this.inspector.walker.getNodeFromActor(actorID, ["node"]);

this will need to use the walker contextual to the animation actor instead.

Comment 2

[Chris Peterson [:cpeterson]]

Tentatively moving all bugs whose summaries mention "Fission" (or other Fission-related keywords) but are not assigned to a Fission Milestone to the "?" triage milestone.

This will generate a lot of bugmail, so you can filter your bugmail for the following UUID and delete them en masse:

0ee3c76a-bc79-4eb2-8d12-05dc0b68e732

Comment 3

[Patrick Brosset <:pbro>]

In the main panel file: animation.js, this function

  _getAnimationsFront() {
    if (this.animationsFrontPromise) {
      return this.animationsFrontPromise;
    }
    this.animationsFrontPromise = (async () => {
      const target = this.inspector.currentTarget;
      const front = await target.getFront("animations");
      front.setWalkerActor(this.inspector.walker);
      return front;
    })();
    return this.animationsFrontPromise;
  }

will need to be changed. This is called just once when the panel starts.
This is wrong for 2 reasons:

• with fission, there will be process switches on navigation, so we need to use the TargetList API to react to new top-level targets and get the new animations front when that happens
• but also, we want to get the animations that are running inside the frame of the currently selected element, so we might as well get the animations front only when a new node is selected, using selection.nodeFront.targetFront.getFront("animations") instead.

Comment 4

[Chris Peterson [:cpeterson]]

Tracking for Fission Nightly (M6)

Comment 5

[Patrick Brosset <:pbro>]

Attachment: Bug 1568874 - Using TargetList API in animation panel to react to new targets

The animations panel retrieves the instance of the AnimationsFront when starting
that corresponds to the top-level target the inspector is connected to.
It uses that to list animations and set their states and current times.
This is fine, this is the current logic of this panel, which we want to retain
(i.e. we do want to change it to also list animations inside nested, perhaps
remote, iframes).

However with fission, navigating to a different site in the same tab will cause
a process switch and we therefore need to make sure an updated instance of the
AnimationsFront is retrieved then.

We can do this using the TargetList API.

Comment 6

[Patrick Brosset <:pbro>]

(In reply to Patrick Brosset <:pbro> from comment #3)

• with fission, there will be process switches on navigation, so we need to use the TargetList API to react to new top-level targets and get the new animations front when that happens

This is what the patch above does.

• but also, we want to get the animations that are running inside the frame of the currently selected element, so we might as well get the animations front only when a new node is selected, using selection.nodeFront.targetFront.getFront("animations") instead.

I ended up deciding against this as this would introduce a new feature to the animation panel that doesn't exist today. Whenever selecting a node inside an iframe, this would have the effect of showing the animations for this iframe.

Comment 7

[Daisuke Akatsuka (:daisuke)]

I discussed with Patrick on Slack, I will take this bug.

Comment 8

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1568874: Install target-switching mechanism. r?jdescottes!

Comment 9

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1568874: Rename and refactor some functions. r?jdescottes!

Depends on D60477

Comment 10

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1568874: Add a test for target-switching. r?jdescottes!

Depends on D60478

Comment 11

[Daisuke Akatsuka (:daisuke)]

I wrote a test for target-switching for this, but it is failing when navigating from the main process page to the content process page.
As long as I investigated, it seems that this failure occurs for a few reasons.
As at least it seems that we are hitting the bug 1604126, I'll fix bug 1604126 before this one.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/84bea78312b2
Install target-switching mechanism. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/03a2a8f68e02
Rename and refactor some functions. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/2d442aa5fd93
Add a test for target-switching. r=jdescottes

Comment 13

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/84bea78312b2
https://hg.mozilla.org/mozilla-central/rev/03a2a8f68e02
https://hg.mozilla.org/mozilla-central/rev/2d442aa5fd93