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 286021 396a68628a6bfc7bd0b96a9f64c94b4975f4cf93
parent 286020 c2977d29a60bfb9eaf02227840f91db3032ce032
child 286022 37ca88b45a72c955fda3069a4f9eb2b2e3d04b0a
push id17889
push userryanvm@gmail.com
push dateMon, 29 Feb 2016 17:49:02 +0000
treeherderfx-team@04634ec900b2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspbrosset
bugs1247243
milestone47.0a1
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>