Bug 1361234 - Restore test_deferred_start.html to actually testing delay; r?hiro draft
authorBrian Birtles <birtles@gmail.com>
Tue, 02 May 2017 16:40:23 +0900
changeset 571192 53004104bef1359537fb1e9813353f8ce1e9b995
parent 571131 2e7c10a9b86e30691f67855f6c8f98d984508d7c
child 571193 ad9c46175dccade97d6f5972ed75b0ddc8811c4c
push id56720
push userbbirtles@mozilla.com
push dateTue, 02 May 2017 08:35:19 +0000
reviewershiro
bugs1361234
milestone55.0a1
Bug 1361234 - Restore test_deferred_start.html to actually testing delay; r?hiro Once upon a time[1] a test was added to test_deferred_start.html to test the code path where we establish the start time to pass to the compositor when resolving a pending animation. Later[2], however, we encountered intermittent failures on B2G so we made it stop waiting on animation.ready and use waitForPaints instead. There were two problems with this, however. Firstly, waiting for paints often means that extra paints are processed such that we end up updating the start time on the layer using a different code path and masking any potential bugs in the code path under test. Secondly, when we made this change we replaced: return animation.ready.then(function() { /* test code */ }) with: return waitForPaints(function() { /* test code */ }) And sadly that means that 'test code' never runs. Of course, what we meant to write was: return waitForPaints().then(function() { /* test code */ }) As a result, when we later broke the code path under test no one noticed. This patch restores the test so that it tests what it intends to (and currently fails, at least most of the time). [1] https://hg.mozilla.org/mozilla-central/rev/79cac8c71159 [2] https://hg.mozilla.org/mozilla-central/rev/986b18fdfdba [3] https://hg.mozilla.org/mozilla-central/rev/b66b75c2d042101b954e6423438cc07955c2b9bd MozReview-Commit-ID: 1iMWLQP6zae
dom/animation/test/mochitest.ini
dom/animation/test/mozilla/file_deferred_start.html
dom/animation/test/testcommon.js
--- a/dom/animation/test/mochitest.ini
+++ b/dom/animation/test/mochitest.ini
@@ -96,16 +96,17 @@ support-files =
 [css-transitions/test_event-dispatch.html]
 [css-transitions/test_keyframeeffect-getkeyframes.html]
 [css-transitions/test_pseudoElement-get-animations.html]
 [css-transitions/test_setting-effect.html]
 [document-timeline/test_document-timeline.html]
 [document-timeline/test_request_animation_frame.html]
 [mozilla/test_cubic_bezier_limits.html]
 [mozilla/test_deferred_start.html]
+skip-if = true # Bug 1361234 (mostly fails)
 [mozilla/test_disable_animations_api_core.html]
 [mozilla/test_disabled_properties.html]
 [mozilla/test_discrete-animations.html]
 [mozilla/test_document-timeline-origin-time-range.html]
 [mozilla/test_hide_and_show.html]
 [mozilla/test_set-easing.html]
 [mozilla/test_spacing_property_order.html]
 [mozilla/test_transform_limits.html]
--- a/dom/animation/test/mozilla/file_deferred_start.html
+++ b/dom/animation/test/mozilla/file_deferred_start.html
@@ -1,18 +1,14 @@
 <!doctype html>
 <meta charset=utf-8>
 <script src="../testcommon.js"></script>
 <script src="/tests/SimpleTest/paint_listener.js"></script>
 <style>
 @keyframes empty { }
-@keyframes animTransform {
-  from { transform: translate(0px); }
-  to { transform: translate(100px); }
-}
 .target {
   /* Element needs geometry to be eligible for layerization */
   width: 100px;
   height: 100px;
   background-color: white;
 }
 </style>
 <body>
@@ -83,39 +79,53 @@ promise_test(function(t) {
 // SpecialPowers.DOMWindowUtils.advanceTimeAndRefresh here since that takes
 // us through a different code path.
 promise_test(function(t) {
   // This test only applies to compositor animations
   if (!isOMTAEnabled()) {
     return;
   }
 
-  // Setup animation
-  var div = addDiv(t);
+  const div = addDiv(t);
   div.classList.add('target');
-  div.style.animation = 'animTransform 100s -50s forwards';
-  var animation = div.getAnimations()[0];
 
-  return waitForPaints(function() {
+  // As with the above test, any stray paints can cause this test to produce
+  // a false negative (that is, pass when it should fail). To avoid this we
+  // first wait for the document to load, then wait until it is idle, then wait
+  // for paints and only then do we commence the test. Even doing that, this
+  // test can sometimes pass when it should not due to a stray paint. Most of
+  // the time, however, it will correctly fail so hopefully even if we do
+  // occasionally produce a false negative on one platform, another platform
+  // will fail as expected.
+  return waitForDocLoad().then(() => waitForIdle())
+  .then(() => waitForPaints())
+  .then(() => {
+    div.animate({ transform: [ 'translate(0px)', 'translate(100px)' ] },
+                { duration: 400 * MS_PER_SEC,
+                  delay: -200 * MS_PER_SEC });
+    return waitForPaints();
+  }).then(() => {
     var transformStr =
       SpecialPowers.DOMWindowUtils.getOMTAStyle(div, 'transform');
 
     var matrixComponents =
       transformStr.startsWith('matrix(')
       ? transformStr.substring('matrix('.length, transformStr.length-1)
                     .split(',')
                     .map(component => Number(component))
       : [];
     assert_equals(matrixComponents.length, 6,
                   'Got a valid transform matrix on the compositor'
                   + ' (got: "' + transformStr + '")');
 
-    // If the delay has been applied correctly we should be at least
-    // half-way through the animation
-    assert_true(matrixComponents[4] >= 50,
-                'Animation is at least half-way through on the compositor'
-                + ' (got translation of ' + matrixComponents[4] + ')');
+    // If the delay has been applied we should be about half-way through
+    // the animation. However, if we applied it twice we will be at the
+    // end of the animation already so check that we are roughly half way
+    // through.
+    const translateX = matrixComponents[4];
+    assert_between_inclusive(translateX, 40, 75,
+        'Animation is about half-way through on the compositor');
   });
 }, 'Starting an animation with a delay starts from the correct point');
 
 done();
 </script>
 </body>
--- a/dom/animation/test/testcommon.js
+++ b/dom/animation/test/testcommon.js
@@ -230,16 +230,25 @@ function waitForAnimationFrames(frameCou
         window.requestAnimationFrame(handleFrame); // wait another frame
       }
     }
     window.requestAnimationFrame(handleFrame);
   });
 }
 
 /**
+ * Promise wrapper for requestIdleCallback.
+ */
+function waitForIdle() {
+  return new Promise(resolve => {
+    requestIdleCallback(resolve);
+  });
+}
+
+/**
  * Wrapper that takes a sequence of N animations and returns:
  *
  *   Promise.all([animations[0].ready, animations[1].ready, ... animations[N-1].ready]);
  */
 function waitForAllAnimations(animations) {
   return Promise.all(animations.map(function(animation) {
     return animation.ready;
   }));