1175357 - animation-controller starts actor while destroying toolbox

Status: RESOLVED DUPLICATE of bug 1247243

(DevTools :: Inspector, defect, P3)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

STR:

1. Open Inspector, but don't go to the Animations tab
2. Using Browser Toolbox, set a breakpoint in ActorPool.addActor from common.js
3. Close the toolbox

ER:

The breakpoint should not be hit, no reason to add actors on shutdown

AR:

animation-controller.js ends up calling "this.animationsFront.stopAnimationPlayerUpdates()" even if it never interacted with the front during the rest of the toolbox session.  This starts the actor, only to tear it down a moment later.

Comment 1

[Patrick Brosset <:pbro>]

Calling stopAnimationPlayerUpdates is only required if getAnimationPlayersForNode was called once first.
So, we could simply store a boolean on the instance of the controller to check this before calling stopAnimationPlayerUpdates. Unless there is an easy way from the front to know if the actor has been used already or not.

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Patrick Brosset [:pbrosset] [:patrick] [:pbro] from comment #1)
> Calling stopAnimationPlayerUpdates is only required if
> getAnimationPlayersForNode was called once first.
> So, we could simply store a boolean on the instance of the controller to
> check this before calling stopAnimationPlayerUpdates. Unless there is an
> easy way from the front to know if the actor has been used already or not.

Hmm, but the actor itself calls |stopAnimationPlayerUpdates| in it's own |destroy| method.  So, when the connection is destroyed, the actor will automatically |stopAnimationPlayerUpdates| without the client needing to do so manually.

So, we could potentially remove the call from shutdown, and let the actor take care of it, similar to other inspector shutdown cleanups Alex has been doing lately.

However, I guess we would keep getting updates for cases where the connection remain alive (along with actors) after toolbox close, such as WebIDE.

I can't think of anything on the front today that would help, but we could add a "did I ever send a message?" flag to fronts if this seems like it could become a common pattern.

Comment 3

[Patrick Brosset <:pbro>]

I might be missing something here, but if I change AnimationsController.destroyAnimationPlayers to contain:

    if (this.hasMutationEvents && this.animationPlayers.length) {
      yield this.animationsFront.stopAnimationPlayerUpdates();
    }

intead of:

    if (this.hasMutationEvents) {
      yield this.animationsFront.stopAnimationPlayerUpdates();
    }

The the  STR in comment 0 work as expected, the addActor method isn't called anymore, so the actor isn't being created unnecessarily.

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #3)
> I might be missing something here, but if I change
> AnimationsController.destroyAnimationPlayers to contain:
> 
>     if (this.hasMutationEvents && this.animationPlayers.length) {
>       yield this.animationsFront.stopAnimationPlayerUpdates();
>     }
> 
> intead of:
> 
>     if (this.hasMutationEvents) {
>       yield this.animationsFront.stopAnimationPlayerUpdates();
>     }
> 
> The the  STR in comment 0 work as expected, the addActor method isn't called
> anymore, so the actor isn't being created unnecessarily.

While that does fix the actor creation issue, I think it may not be quite what we want:

For this case where the animation panel actually was displayed (which in turn starts the player update observer via getAnimationPlayersForNode), but there are no animation players under the selected node, this.animationPlayers.length will be 0.

So, we may still need a separate flag.

Comment 5

[Julian Descottes [:jdescottes]]

Looks like this was fixed by bug 1247243.

Inspector bugs triage. Filter on CLIMBING SHOES.