1213651 - Reuse AnimationTargetNode instances across timeline refresh

Status: RESOLVED FIXED

(DevTools :: Inspector: Animations, defect)

Description

[Patrick Brosset <:pbro>]

The AnimationTargetNode component represents a DOM Node in the animation-inspector.
Every time the panel is refreshed, new instances are created and rendered.

We should:
- store instances in a WeakMap indexed by AnimationPlayerFront so that we can reuse instead of re-instantiate
- on render, see if the NodeFront was already retrieved (via walker.getNodeFromActor) and if so, don't retrieve it again.

The goal is to reduce the number of objects being created and to avoid the flickering each time the panel is refreshed (which happens also when you press play, because we need to refresh the players's states).

Comment 1

[Patrick Brosset <:pbro>]

Attachment: Bug_1213651_-_1_-_Store_NodeFronts_across_Animatio.diff

This simple change reuses NodeFronts when the UI refreshes on play/pause/rewind.
The advantage is that this prevents the UI from flickering, so it's already a good step forward.

Comment 2

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: Bug 1213651 - Avoid server round-trips when displaying animated dom nodes

The main change here is that nodeFronts that have already been displayed
in the timeline are stored in a WeakMap so they can be retrieved from it
next time they're displayed and avoid a server-side round trip which, in
turn, causes the UI to flicker.

The other change is that now, it is possible to tell the animations actor
what is the current walker actor, which allows animation player actors to
directly send the NodeActor ID as part of their forms. Which, in most cases,
completely eliminates the server round-trip, because the corresponding
NodeFronts are already known on the client, so we get them from there.

The last change done here is that AnimationTargetNode now becomes a thin
wrapper on top of the new DomNodePreview component that was extracted so
it can be reused in other places.

Review commit: https://reviewboard.mozilla.org/r/31417/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31417/

Comment 3

[Patrick Brosset <:pbro>]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a22fb9df062&group_state=expanded

Comment 4

[Tom Tromey :tromey]

Comment on attachment 8709392 [details]
MozReview Request: Bug 1213651 - Avoid server round-trips when displaying animated dom nodes

https://reviewboard.mozilla.org/r/31417/#review28391

Looks good to me.  I have a few nits but nothing very serious.

::: devtools/client/animationinspector/animation-controller.js:98
(Diff revision 1)
> +      method: "setWalkerActor" }

bgrins convinced me that lists like this should have a trailing "," so that future additions don't touch the history of the previous line.

It's fine to ignore this, I know opinions vary on it, and I don't think it's super important.

::: devtools/client/animationinspector/components/animation-target-node.js:61
(Diff revision 1)
> -    }
> +      } finally {

Putting this logic into a "finally" makes the code more obscure, especially because the "catch" always ends in a "return".

I think it would be clearer to remove the finally and just put the condition after the try/catch.

::: devtools/client/inspector/shared/dom-node-preview.js:7
(Diff revision 1)
> +Cu.import("resource://devtools/client/shared/widgets/ViewHelpers.jsm");

I didn't check but I wonder how eslint-clean this code is.  Cu.import often yields some undefined warnings from eslint, see bug 1240068.  One workaround for that is to add an "import-globals-from" comment: http://gecko.readthedocs.org/en/latest/testing/eslint-plugin-mozilla/eslint-plugin-mozilla/import-globals-from.html

Naturally if this is eslint-clean, just ignore.

Comment 5

[Patrick Brosset <:pbro>]

(In reply to Tom Tromey :tromey from comment #4)
> ::: devtools/client/animationinspector/animation-controller.js:98
> (Diff revision 1)
> > +      method: "setWalkerActor" }
> 
> bgrins convinced me that lists like this should have a trailing "," so that
> future additions don't touch the history of the previous line.
> 
> It's fine to ignore this, I know opinions vary on it, and I don't think it's
> super important.
Makes sense. I'll change this.
> ::: devtools/client/animationinspector/components/animation-target-node.js:61
> (Diff revision 1)
> > -    }
> > +      } finally {
> 
> Putting this logic into a "finally" makes the code more obscure, especially
> because the "catch" always ends in a "return".
> 
> I think it would be clearer to remove the finally and just put the condition
> after the try/catch.
Oh yeah, good catch. Thanks, fixed.
> ::: devtools/client/inspector/shared/dom-node-preview.js:7
> (Diff revision 1)
> > +Cu.import("resource://devtools/client/shared/widgets/ViewHelpers.jsm");
> 
> I didn't check but I wonder how eslint-clean this code is.  Cu.import often
> yields some undefined warnings from eslint, see bug 1240068.  One workaround
> for that is to add an "import-globals-from" comment:
> http://gecko.readthedocs.org/en/latest/testing/eslint-plugin-mozilla/eslint-
> plugin-mozilla/import-globals-from.html
> 
> Naturally if this is eslint-clean, just ignore.
Turns out that this Cu.import does work with our eslint rule, so I didn't have to use the import-from-globals rule.

Comment 6

[Patrick Brosset <:pbro>]

In fact, because I'm creating a new file, there's no reason to exclude it in the .eslintignore, so I'm going to exclude everything else but this file (and maybe the files in the same directory, since they're almost lint-free already and it will make it easier to write the ignore rules in .eslintignore).

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/a7b909eabadf

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a7b909eabadf

Comment 9

[Mayur Patil]

[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
`var exampleObj = {
    type: "example",
    url: "https://maps.googleapis.com/maps/api/streetview?size=600x400&location=San+Francisco+CA",
    example: true
}

console.log(exampleObj);`

STR: Not clear.
Developer specific testing

Component: 
Name 			Firefox
Version 		46.0b4
Build ID 		20160322075646
Update Channel 	        beta
User Agent 		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS                      Windows 7 SP1 x86_64

Expected Results: 
Developer specific testing

Actual Results: 
As expected