Bug 1247243 - Animations are shown only every 2 reloads. r=pbrosset
authorNicolas Chevobbe <chevobbe.nicolas@gmail.com>
Tue, 23 Feb 2016 00:15:04 +0100
changeset 324261 396a68628a6bfc7bd0b96a9f64c94b4975f4cf93
parent 324260 c2977d29a60bfb9eaf02227840f91db3032ce032
child 324262 37ca88b45a72c955fda3069a4f9eb2b2e3d04b0a
push id1128
push userjlund@mozilla.com
push dateWed, 01 Jun 2016 01:31:59 +0000
treeherdermozilla-release@fe0d30de989d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspbrosset
bugs1247243
milestone47.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1247243 - Animations are shown only every 2 reloads. r=pbrosset MozReview-Commit-ID: 71XsHc9WXHw
devtools/client/animationinspector/animation-controller.js
devtools/client/animationinspector/components/animation-timeline.js
devtools/client/animationinspector/test/browser_animation_playerFronts_are_refreshed.js
devtools/server/actors/animation.js
devtools/server/tests/browser/browser_animation_playPauseSeveral.js
devtools/server/tests/browser/browser_animation_setCurrentTime.js
devtools/server/tests/mochitest/animation-data.html
devtools/server/tests/mochitest/chrome.ini
devtools/server/tests/mochitest/test_animation_actor-lifetime.html
--- a/devtools/client/animationinspector/animation-controller.js
+++ b/devtools/client/animationinspector/animation-controller.js
@@ -143,23 +143,16 @@ var AnimationsController = {
     this.onAnimationMutations = this.onAnimationMutations.bind(this);
 
     let target = gToolbox.target;
     this.animationsFront = new AnimationsFront(target.client, target.form);
 
     // Expose actor capabilities.
     this.traits = yield getServerTraits(target);
 
-    // We want to handle animation mutation events synchronously to avoid race
-    // conditions when there are many rapid mutations. So when a mutation occurs
-    // and animations are removed, we don't release the corresponding actors
-    // in a blocking way, we just release asynchronously and don't wait for
-    // completion, but instead store the promise in this array.
-    this.nonBlockingPlayerReleases = [];
-
     if (this.destroyed) {
       console.warn("Could not fully initialize the AnimationsController");
       return;
     }
 
     // Let the AnimationsActor know what WalkerActor we're using. This will
     // come in handy later to return references to DOM Nodes.
     if (this.traits.hasSetWalkerActor) {
@@ -179,22 +172,19 @@ var AnimationsController = {
 
     if (this.destroyed) {
       yield this.destroyed.promise;
       return;
     }
     this.destroyed = promise.defer();
 
     this.stopListeners();
-    yield this.destroyAnimationPlayers();
+    this.destroyAnimationPlayers();
     this.nodeFront = null;
 
-    // Finish releasing players that haven't been released yet.
-    yield promise.all(this.nonBlockingPlayerReleases);
-
     if (this.animationsFront) {
       this.animationsFront.destroy();
       this.animationsFront = null;
     }
 
     this.destroyed.resolve();
   }),
 
@@ -233,17 +223,17 @@ var AnimationsController = {
         this.nodeFront === gInspector.selection.nodeFront) {
       return;
     }
 
     let done = gInspector.updating("animationscontroller");
 
     if (!gInspector.selection.isConnected() ||
         !gInspector.selection.isElementNode()) {
-      yield this.destroyAnimationPlayers();
+      this.destroyAnimationPlayers();
       this.emit(this.PLAYERS_UPDATED_EVENT);
       done();
       return;
     }
 
     this.nodeFront = gInspector.selection.nodeFront;
     yield this.refreshAnimationPlayers(this.nodeFront);
     this.emit(this.PLAYERS_UPDATED_EVENT, this.animationPlayers);
@@ -329,17 +319,17 @@ var AnimationsController = {
 
   // AnimationPlayerFront objects are managed by this controller. They are
   // retrieved when refreshAnimationPlayers is called, stored in the
   // animationPlayers array, and destroyed when refreshAnimationPlayers is
   // called again.
   animationPlayers: [],
 
   refreshAnimationPlayers: Task.async(function*(nodeFront) {
-    yield this.destroyAnimationPlayers();
+    this.destroyAnimationPlayers();
 
     this.animationPlayers = yield this.animationsFront
                                       .getAnimationPlayersForNode(nodeFront);
 
     // Start listening for animation mutations only after the first method call
     // otherwise events won't be sent.
     if (!this.isListeningToMutations && this.traits.hasMutationEvents) {
       this.animationsFront.on("mutations", this.onAnimationMutations);
@@ -351,18 +341,16 @@ var AnimationsController = {
     // Insert new players into this.animationPlayers when new animations are
     // added.
     for (let {type, player} of changes) {
       if (type === "added") {
         this.animationPlayers.push(player);
       }
 
       if (type === "removed") {
-        // Don't wait for the release request to complete, we can do that later.
-        this.nonBlockingPlayerReleases.push(player.release());
         let index = this.animationPlayers.indexOf(player);
         this.animationPlayers.splice(index, 1);
       }
     }
 
     // Let the UI know the list has been updated.
     this.emit(this.PLAYERS_UPDATED_EVENT, this.animationPlayers);
   },
@@ -381,24 +369,14 @@ var AnimationsController = {
       if (!state.documentCurrentTime) {
         return false;
       }
       time = Math.max(time, state.documentCurrentTime);
     }
     return time;
   },
 
-  destroyAnimationPlayers: Task.async(function*() {
-    // Let the server know that we're not interested in receiving updates about
-    // players for the current node. We're either being destroyed or a new node
-    // has been selected.
-    if (this.traits.hasMutationEvents) {
-      yield this.animationsFront.stopAnimationPlayerUpdates();
-    }
-
-    for (let front of this.animationPlayers) {
-      yield front.release();
-    }
+  destroyAnimationPlayers: function() {
     this.animationPlayers = [];
-  })
+  }
 };
 
 EventEmitter.decorate(AnimationsController);
--- a/devtools/client/animationinspector/components/animation-timeline.js
+++ b/devtools/client/animationinspector/components/animation-timeline.js
@@ -149,16 +149,17 @@ AnimationsTimeline.prototype = {
     }
     this[name] = [];
   },
 
   unrender: function() {
     for (let animation of this.animations) {
       animation.off("changed", this.onAnimationStateChanged);
     }
+    this.stopAnimatingScrubber();
     TimeScale.reset();
     this.destroySubComponents("targetNodes");
     this.destroySubComponents("timeBlocks");
     this.destroySubComponents("details", [{
       event: "frame-selected",
       fn: this.onFrameSelected
     }]);
     this.animationsEl.innerHTML = "";
--- a/devtools/client/animationinspector/test/browser_animation_playerFronts_are_refreshed.js
+++ b/devtools/client/animationinspector/test/browser_animation_playerFronts_are_refreshed.js
@@ -23,29 +23,14 @@ add_task(function*() {
     "One AnimationPlayerFront has been created");
 
   info("Selecting a node with mutliple animations");
   yield selectNode(".multi", inspector);
 
   is(controller.animationPlayers.length, 2,
     "2 AnimationPlayerFronts have been created");
 
-  // Hold on to one of the AnimationPlayerFront objects and mock its release
-  // method to test that it is released correctly and that its auto-refresh is
-  // stopped.
-  let retainedFront = controller.animationPlayers[0];
-  let oldRelease = retainedFront.release;
-  let releaseCalled = false;
-  retainedFront.release = () => {
-    releaseCalled = true;
-  };
-
   info("Selecting a node with no animations");
   yield selectNode(".still", inspector);
 
   is(controller.animationPlayers.length, 0,
     "There are no more AnimationPlayerFront objects");
-
-  info("Checking the destroyed AnimationPlayerFront object");
-  ok(releaseCalled, "The AnimationPlayerFront has been released");
-
-  yield oldRelease.call(retainedFront);
 });
--- a/devtools/server/actors/animation.js
+++ b/devtools/server/actors/animation.js
@@ -556,28 +556,35 @@ var AnimationsActor = exports.Animations
       walker: Arg(0, "domwalker")
     },
     response: {}
   }),
 
   /**
    * Retrieve the list of AnimationPlayerActor actors for currently running
    * animations on a node and its descendants.
+   * Note that calling this method a second time will destroy all previously
+   * retrieved AnimationPlayerActors. Indeed, the lifecycle of these actors
+   * is managed here on the server and tied to getAnimationPlayersForNode
+   * being called.
    * @param {NodeActor} nodeActor The NodeActor as defined in
    * /devtools/server/actors/inspector
    */
   getAnimationPlayersForNode: method(function(nodeActor) {
     let animations = [
       ...nodeActor.rawNode.getAnimations(),
       ...this.getAllAnimations(nodeActor.rawNode)
     ];
 
-    // No care is taken here to destroy the previously stored actors because it
-    // is assumed that the client is responsible for lifetimes of actors.
+    // Destroy previously stored actors
+    if (this.actors) {
+      this.actors.forEach(actor => actor.destroy());
+    }
     this.actors = [];
+
     for (let i = 0; i < animations.length; i++) {
       let actor = AnimationPlayerActor(this, animations[i]);
       this.actors.push(actor);
     }
 
     // When a front requests the list of players for a node, start listening
     // for animation mutations on this node to send updates to the front, until
     // either getAnimationPlayersForNode is called again or
--- a/devtools/server/tests/browser/browser_animation_playPauseSeveral.js
+++ b/devtools/server/tests/browser/browser_animation_playPauseSeveral.js
@@ -9,18 +9,16 @@
 
 // List of selectors that match "all" animated nodes in the test page.
 // This list misses a bunch of animated nodes on purpose. Only the ones that
 // have infinite animations are listed. This is done to avoid intermittents
 // caused when finite animations are already done playing by the time the test
 // runs.
 const ALL_ANIMATED_NODES = [".simple-animation", ".multiple-animations",
                             ".delayed-animation"];
-// List of selectors that match some animated nodes in the test page only.
-const SOME_ANIMATED_NODES = [".simple-animation", ".delayed-animation"];
 
 add_task(function*() {
   let {client, walker, animations} =
     yield initAnimationsFrontForUrl(MAIN_DOMAIN + "animation.html");
 
   info("Pause all animations in the test document");
   yield animations.pauseAll();
   yield checkStates(walker, animations, ALL_ANIMATED_NODES, "paused");
@@ -32,29 +30,44 @@ add_task(function*() {
   info("Pause all animations in the test document using toggleAll");
   yield animations.toggleAll();
   yield checkStates(walker, animations, ALL_ANIMATED_NODES, "paused");
 
   info("Play all animations in the test document using toggleAll");
   yield animations.toggleAll();
   yield checkStates(walker, animations, ALL_ANIMATED_NODES, "running");
 
-  info("Pause a given list of animations only");
-  let players = [];
-  for (let selector of SOME_ANIMATED_NODES) {
-    let [player] = yield getPlayersFor(walker, animations, selector);
-    players.push(player);
-  }
-  yield animations.toggleSeveral(players, true);
-  yield checkStates(walker, animations, SOME_ANIMATED_NODES, "paused");
-  yield checkStates(walker, animations, [".multiple-animations"], "running");
+  info("Play all animations from multiple animated node using toggleSeveral");
+  let players = yield getPlayersFor(walker, animations,
+                                   [".multiple-animations"]);
+  is(players.length, 2, "Node has 2 animation players");
+  yield animations.toggleSeveral(players, false);
+  let state1 = yield players[0].getCurrentState();
+  is(state1.playState, "running",
+    "The playState of the first player is running");
+  let state2 = yield players[1].getCurrentState();
+  is(state2.playState, "running",
+    "The playState of the second player is running");
 
-  info("Play the same list of animations");
-  yield animations.toggleSeveral(players, false);
-  yield checkStates(walker, animations, ALL_ANIMATED_NODES, "running");
+  info("Pause one animation from a multiple animated node using toggleSeveral");
+  yield animations.toggleSeveral([players[0]], true);
+  state1 = yield players[0].getCurrentState();
+  is(state1.playState, "paused", "The playState of the first player is paused");
+  state2 = yield players[1].getCurrentState();
+  is(state2.playState, "running",
+    "The playState of the second player is running");
+
+  info("Play the same animation");
+  yield animations.toggleSeveral([players[0]], false);
+  state1 = yield players[0].getCurrentState();
+  is(state1.playState, "running",
+    "The playState of the first player is running");
+  state2 = yield players[1].getCurrentState();
+  is(state2.playState, "running",
+    "The playState of the second player is running");
 
   yield closeDebuggerClient(client);
   gBrowser.removeCurrentTab();
 });
 
 function* checkStates(walker, animations, selectors, playState) {
   info("Checking the playState of all the nodes that have infinite running " +
        "animations");
--- a/devtools/server/tests/browser/browser_animation_setCurrentTime.js
+++ b/devtools/server/tests/browser/browser_animation_setCurrentTime.js
@@ -49,25 +49,26 @@ function* testSetCurrentTime(walker, ani
   let updatedState2 = yield player.getCurrentState();
   is(Math.round(updatedState2.currentTime - updatedState1.currentTime), -2000,
     "The currentTime was updated to -2s");
 }
 
 function* testSetCurrentTimes(walker, animations) {
   ok(animations.setCurrentTimes, "The AnimationsActor has the right method");
 
-  info("Retrieve multiple animated nodes and their animation players");
-  let node1 = yield walker.querySelector(walker.rootNode, ".simple-animation");
-  let player1 = (yield animations.getAnimationPlayersForNode(node1))[0];
-  let node2 = yield walker.querySelector(walker.rootNode, ".delayed-animation");
-  let player2 = (yield animations.getAnimationPlayersForNode(node2))[0];
+  info("Retrieve multiple animated node and its animation players");
+
+  let nodeMulti = yield walker.querySelector(walker.rootNode,
+    ".multiple-animations");
+  let players = (yield animations.getAnimationPlayersForNode(nodeMulti));
+
+  ok(players.length > 1, "Node has more than 1 animation player");
 
   info("Try to set multiple current times at once");
-  yield animations.setCurrentTimes([player1, player2], 500, true);
+  yield animations.setCurrentTimes(players, 500, true);
 
-  info("Get the states of both players and verify their correctness");
-  let state1 = yield player1.getCurrentState();
-  let state2 = yield player2.getCurrentState();
-  is(state1.playState, "paused", "Player 1 is paused");
-  is(state2.playState, "paused", "Player 2 is paused");
-  is(state1.currentTime, 500, "Player 1 has the right currentTime");
-  is(state2.currentTime, 500, "Player 2 has the right currentTime");
+  info("Get the states of players and verify their correctness");
+  for (let i = 0; i < players.length; i++) {
+    let state = yield players[i].getCurrentState();
+    is(state.playState, "paused", `Player ${i + 1} is paused`);
+    is(state.currentTime, 500, `Player ${i + 1} has the right currentTime`);
+  }
 }
new file mode 100644
--- /dev/null
+++ b/devtools/server/tests/mochitest/animation-data.html
@@ -0,0 +1,120 @@
+<html>
+<head>
+  <meta charset="UTF-8">
+  <title>Animation Test Data</title>
+  <style>
+    .ball {
+      width: 80px;
+      height: 80px;
+      border-radius: 50%;
+      background: #f06;
+
+      position: absolute;
+    }
+
+    .still {
+      top: 0;
+      left: 10px;
+    }
+
+    .animated {
+      top: 100px;
+      left: 10px;
+
+      animation: simple-animation 2s infinite alternate;
+    }
+
+    .multi {
+      top: 200px;
+      left: 10px;
+
+      animation: simple-animation 2s infinite alternate,
+                 other-animation 5s infinite alternate;
+    }
+
+    .delayed {
+      top: 300px;
+      left: 10px;
+      background: rebeccapurple;
+
+      animation: simple-animation 3s 60s 10;
+    }
+
+    .multi-finite {
+      top: 400px;
+      left: 10px;
+      background: yellow;
+
+      animation: simple-animation 3s,
+                 other-animation 4s;
+    }
+
+    .short {
+      top: 500px;
+      left: 10px;
+      background: red;
+
+      animation: simple-animation 2s;
+    }
+
+    .long {
+      top: 600px;
+      left: 10px;
+      background: blue;
+
+      animation: simple-animation 120s;
+    }
+
+    .negative-delay {
+      top: 700px;
+      left: 10px;
+      background: gray;
+
+      animation: simple-animation 15s -10s;
+      animation-fill-mode: forwards;
+    }
+
+    .no-compositor {
+      top: 0;
+      right: 10px;
+      background: gold;
+
+      animation: no-compositor 10s cubic-bezier(.57,-0.02,1,.31) forwards;
+    }
+
+    @keyframes simple-animation {
+      100% {
+        transform: translateX(300px);
+      }
+    }
+
+    @keyframes other-animation {
+      100% {
+        background: blue;
+      }
+    }
+
+    @keyframes no-compositor {
+      100% {
+        margin-right: 600px;
+      }
+    }
+  </style>
+  <script type="text/javascript">
+    window.onload = function() {
+      window.opener.postMessage('ready', '*');
+    };
+  </script>
+</head>
+</body>
+  <div class="ball still"></div>
+  <div class="ball animated"></div>
+  <div class="ball multi"></div>
+  <div class="ball delayed"></div>
+  <div class="ball multi-finite"></div>
+  <div class="ball short"></div>
+  <div class="ball long"></div>
+  <div class="ball negative-delay"></div>
+  <div class="ball no-compositor"></div>
+</body>
+</html>
--- a/devtools/server/tests/mochitest/chrome.ini
+++ b/devtools/server/tests/mochitest/chrome.ini
@@ -1,12 +1,13 @@
 [DEFAULT]
 tags = devtools
 skip-if = buildapp == 'b2g' || os == 'android'
 support-files =
+  animation-data.html
   Debugger.Source.prototype.element.js
   Debugger.Source.prototype.element-2.js
   Debugger.Source.prototype.element.html
   director-helpers.js
   hello-actor.js
   inspector_getImageData.html
   inspector-delay-image-response.sjs
   inspector-helpers.js
@@ -17,16 +18,17 @@ support-files =
   large-image.jpg
   memory-helpers.js
   memprof-helpers.js
   nonchrome_unsafeDereference.html
   small-image.gif
   setup-in-child.js
   setup-in-parent.js
 
+[test_animation_actor-lifetime.html]
 [test_connection-manager.html]
 skip-if = buildapp == 'mulet'
 [test_connectToChild.html]
 skip-if = buildapp == 'mulet'
 [test_css-logic.html]
 [test_css-logic-inheritance.html]
 [test_css-logic-media-queries.html]
 [test_css-logic-specificity.html]
new file mode 100644
--- /dev/null
+++ b/devtools/server/tests/mochitest/test_animation_actor-lifetime.html
@@ -0,0 +1,92 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=1247243
+-->
+<head>
+  <meta charset="utf-8">
+  <title>Test for Bug 1247243</title>
+
+  <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="chrome://mochikit/content/tests/SimpleTest/test.css">
+  <script type="application/javascript;version=1.8" src="inspector-helpers.js"></script>
+  <script type="application/javascript;version=1.8">
+window.onload = function() {
+  const Ci = Components.interfaces;
+  const {AnimationsFront} = require("devtools/server/actors/animation");
+  const {InspectorFront} = require("devtools/server/actors/inspector");
+
+  SimpleTest.waitForExplicitFinish();
+
+  let gWalker = null;
+  let gClient = null;
+  let animationsFront = null;
+
+  addTest(function setup() {
+    info ("Setting up inspector and animation actors.");
+
+    let url = document.getElementById("animationContent").href;
+    attachURL(url, function(err, client, tab, doc) {
+      let inspector = InspectorFront(client, tab);
+
+      animationsFront = new AnimationsFront(client, tab);
+
+      promiseDone(inspector.getWalker().then(walker => {
+        ok(walker, "getWalker() should return an actor.");
+        gClient = client;
+        gWalker = walker;
+      }).then(runNextTest));
+
+    });
+  });
+
+  addAsyncTest(function* testActorLifetime() {
+
+    info ("Testing animated node actor");
+    let animatedNodeActor = yield gWalker.querySelector(gWalker.rootNode,
+      ".animated");
+    yield animationsFront.getAnimationPlayersForNode(animatedNodeActor);
+
+    let serverConnection = animationsFront.conn._transport._serverConnection;
+    let animationsActor = serverConnection.getActor(animationsFront.actorID);
+
+    is(animationsActor.actors.length, 1,
+      "AnimationActor have 1 AnimationPlayerActors");
+
+    info ("Testing AnimationPlayerActors release");
+    let stillNodeActor = yield gWalker.querySelector(gWalker.rootNode,
+      ".still");
+    yield animationsFront.getAnimationPlayersForNode(stillNodeActor);
+    is(animationsActor.actors.length, 0,
+      "AnimationActor does not have any AnimationPlayerActors anymore");
+
+    info ("Testing multi animated node actor");
+    let multiNodeActor = yield gWalker.querySelector(gWalker.rootNode,
+      ".multi");
+    yield animationsFront.getAnimationPlayersForNode(multiNodeActor);
+    is(animationsActor.actors.length, 2,
+      "AnimationActor has now 2 AnimationPlayerActors");
+
+    info ("Testing single animated node actor");
+    yield animationsFront.getAnimationPlayersForNode(animatedNodeActor);
+    is(animationsActor.actors.length, 1,
+      "AnimationActor has only one AnimationPlayerActors");
+
+    info ("Testing AnimationPlayerActors release again");
+    yield animationsFront.getAnimationPlayersForNode(stillNodeActor);
+    is(animationsActor.actors.length, 0,
+      "AnimationActor does not have any AnimationPlayerActors anymore");
+
+    runNextTest();
+  });
+
+
+  runNextTest();
+};
+  </script>
+</head>
+<body>
+  <a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1247243">Mozilla Bug 1247243</a>
+  <a id="animationContent" target="_blank" href="animation-data.html">Test Document</a>
+</body>
+</html>