In bug 964646 we discovered that the code for testing CSS Transitions on the compositor (OMTA) introduced in bug 788549 isn't actually run on any of the build machines. This is because getOMTAOrComputedStyle would fall back to using the computed style on the only platform that had OMTA enabled (B2G). We need to make the CSS Transitions tests actually confirm they are running on the compositor when they are expected to be. We should then remove getOMTAOrComputedStyle. We also need to increase test coverage of CSS transitions running on the compositor in general. For example, there are no tests that check that a transition with a delay is run correctly (and I know from ad-hoc testing that at least at one point they did not).

I've mostly converted the transitions tests but I was having trouble with one transition from "skew(45deg, 45deg)"--which is not invertible--to "none". After some debugging it turns out: * The transition gets created * The updateTransformLayer hint gets set * RestyleManager processes it and schedules an invalidating paint * When we get to nsDisplayTransform::BuildLayer we call FrameLayerBuilder::BuildContainerLayerFor which calls ChooseScaleAndSetTransform * The call to transform.IsSingular() returns false, * FrameLayerBuilder::BuildContainerLayerFor returns nullptr, * And nsDisplayTransform::BuildLayer bails out early since 'container' is null I haven't been able to create a simplified test case that reproduces this problem in practice. Presumably we create the layer on the next sample where the transform is no longer non-invertible. But how do we update the transform in that case? Shouldn't any updates be throttled by that point? Perhaps in CommonElementAnimationData::CanThrottleAnimation we detect that layer isn't there so we skip throttling then?

This patch moves some test utility methods from test_animations_omta.html to animation_utils.js. It also renames addAsyncTest to addAsyncAnimTest and likewise for a few other methods.

This patch removes the line from new_div that forced a style flush. This was very confusing because: * It behaved differently to new_div in test_animations.html so copying tests over was more complex (particularly when registering for events is involved). * It meant after setting up initial style using new_div you could just call waitForPaints but if you updated style using elem.style you'd need to call waitForPaintsFlushed.

This patch simply re-arranges the order of arguments to omta_is_approx so that the delta sits along side the values being compared. This, I think, makes more sense and also is more consistent when converting tests from test_animations.html to test_animations_omta.html since the "RunningOn" parameter is consistently inserted in the second-to-last position just before the description for both omta_is and omta_is_approx.

This patch simply moves code from test_animations_omta.html to animation_utils.js so that we can use it for testing transitions as well.

This patch moves some utility methods from test_animations_omta.html to animation_utils.js so that they can be used for testing transitions as well.

This patch fixes a bug in the handling of 3d matrices represented as an array of numbers.

This patch takes the existing tests for transitions running on the compositor and makes them re-use the same test utility methods as used for testing CSS Animations that run on the compositor. This means these tests now also check that the transition is in fact running on the compositor when it is expected to. It seems the big_omta_round_error is no longer needed so I've removed that. The test that begins with "skew(45deg, 45deg)" currently fails so it is skipped here. This is addressed in the next patch in the series.

This patch adjusts the tests for transform transitions that run on the compositor to handle transitions that begin with a non-invertible transform. In this case the first sample at the start of the animation won't create a layer because in nsDisplayTransform::BuildLayer / FrameLayerBuilder::BuildContainerLayerFor we'll skip creating the layer once we notice the equivalent matrix is singular. In this patch we detect that case and force an extra sample betwee 0 and 200s at 100s. This means the layer will be created at t=100s and be available for querying at the next sample. Similar issues could occur, for example, if the transforms at both t=0s and t=100s are not invertible but currently that doesn't occur. We can add handling for that if and when it becomes necessary. This patch adds an internal getDeterminant method to animation_utils.js which is pretty massive. The 3D part is actually not used but it here for completeness. Ultimately I hope we can remove much of this code and use DOMMatrix once it lands. That's why I haven't factored this out into some generic matrix handling utility class.

Now that getOMTAOrComputedStyle is no longer used by any tests it should be removed. It is a footgun because it can lead the test author to think they are testing compositor animations (OMTA) when in fact they are just testing main thread animations. This has bitten us before because while we were getting results from the compositor on some platforms when run locally, on Tinderbox the only build configurations that had OMTA turned on were falling back to returning the computed style.

Restore accidentally deleted tests

Rebase

Fix shortcuts for omta_is and co. since they were completely wrong.

Comment on attachment 8434707 [details] [diff] [review] part 1 - Factor out common async animation test methods Review of attachment 8434707 [details] [diff] [review]: ----------------------------------------------------------------- With one exception, I think these review comments all are about pre-existing [moved-in-this-patch] code. Feel free to address them in a separate patch, if you'd like to keep this patch focused on moving-without-much-rewriting. (The one exception is for my second comment, about 'tests.length = 0' -- I don't think that exists in the original code.) ::: layout/style/test/animation_utils.js @@ +197,5 @@ > + > + // Returns a promise when all tests have run > + window.runAllAsyncAnimTests = function(aOnAbort) { > + // The async test runner returns a Promise that is resolved when the > + // test is finished so we can chain them together "The async test runner" is a bit vague/abstract here. This comment is talking about "runAsyncAnimTest", right? If so, maybe just use that function's name directly in the comment here, or mention it in a parenthetical. @@ +203,5 @@ > + return sequence.then(function() { > + return runAsyncAnimTest(test, aOnAbort); > + }); > + }, Promise.resolve() /* the start of the sequence */) > + .then(function() { tests.length = 0; }); I'm not clear on why we clear 'tests' at the end. Won't we already be done with this array, by the time we execute this line? Does something depend on it being emptied? If not, it seems like it'd be simpler to leave it alone. (But if we do need to empty it, add a comment to explain why.) @@ +208,5 @@ > + }; > + > + // Takes a generator function that represents a test case. Each point in the > + // test case that waits asynchronously for some result yields a Promise that > + // is resolved when the asychronous action has completed. By chaining these "asychronous" there is missing an 'n'. (s/sych/synch/) @@ +216,5 @@ > + // function has completed. > + // > + // This arrangement is based on add_task() which is currently only available > + // in mochitest-chrome (bug 872229). Once add_task is available in > + // mochitest-plain we can remove this function and use add_task instead. (This implies that there are plans to make add_task available in mochitest-plain -- is that actually the case? If so, is there a bug for this, which we can link to here? I couldn't find one in a quick bugzilla search.) @@ +254,5 @@ > + } > + > + window.addAsyncAnimTest = function(generator) { > + tests.push(generator); > + }; Maybe move these last 3 lines (for addAsyncAnimTest) to right after where you declare "var tests"? In its current spot, at the end of the file, this code gets kind of gets visually mixed in with runAsyncAnimTest()'s impl. (And since it's a tiny one-off that just appends to tests, it seems like it makes more sense to have it live right next to tests.)

Comment on attachment 8434718 [details] [diff] [review] part 2 - Make new_div no longer secretly flush styles r=me

Comment on attachment 8434693 [details] [diff] [review] part 3 - Fix the order of arguments to omta_is_approx r=me

Comment on attachment 8436680 [details] [diff] [review] part 4 - Move omta_is and friends to animation_utils.js Review of attachment 8436680 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/test/test_animations_omta.html @@ +170,5 @@ > + var args = Array.slice(arguments); > + if (!(args[0] instanceof Element)) { > + args.unshift(gDiv); > + } > + return origFn.apply(undefined, args); Nit: In this "Shortcut omta_is and friends" code, I think we should be passing "window" as the first ("this") arg to the apply() call, instead of "undefined", right? (These methods probably don't currently care about their 'this' pointer, so it doesn't much matter. But if someone ever tweaks them such that they do start caring, then I think the "undefined" in this apply() call would break things in a sneaky-and-mysterious way.)

Comment on attachment 8434695 [details] [diff] [review] part 5 - Move paint listener promise wrappers to animation_utils.js r=me

Comment on attachment 8434696 [details] [diff] [review] part 6 - Fix handling of 3d matrices in omta_is and co. Yikes! Do you know why this 3D matrix slice() issue hasn't caused problems? (Were we just not using it for actual 4x4 matrices yet?) r=me, anyway

Comment on attachment 8434698 [details] [diff] [review] part 7 - Convert test_transitions_per_property.html to use OMTA test methods r=me

Comment on attachment 8434700 [details] [diff] [review] part 8 - Add special handling for non-invertible transforms (In reply to Brian Birtles (:birtles) from comment #9) > This patch adds an internal getDeterminant method to animation_utils.js > which is > pretty massive. The 3D part is actually not used but it here for > completeness. Hmmm... Give that the 3D part is not used, I worry a bit that we have no way of knowing whether it's correct or not (or of testing that it remains correct, assuming that it is correct now). I wonder if it might be better to leave that unused code commented-out, with a helpful ok(false, "...") message adjacent to it, telling people to uncomment it if they ever need it? (And if anyone does need it, they'll fail that "ok(false" and see your helpful message.) Or instead of commenting it out, you could even copypaste it into a bug comment here, and reference the bug comment's URL from the ok(false) message. > Ultimately I hope we can remove much of this code and use DOMMatrix (This is part of the reason I'm wondering about this.) If you'd like to keep it, though, I'm also OK with that.

Comment on attachment 8434702 [details] [diff] [review] part 9 - Remove nsIDOMWindowUtils::GetOMTAOrComputedStyle r=me

(In reply to Daniel Holbert [:dholbert] from comment #14) > Comment on attachment 8434707 [details] [diff] [review] > part 1 - Factor out common async animation test methods > > Review of attachment 8434707 [details] [diff] [review]: > ... > @@ +203,5 @@ > > + return sequence.then(function() { > > + return runAsyncAnimTest(test, aOnAbort); > > + }); > > + }, Promise.resolve() /* the start of the sequence */) > > + .then(function() { tests.length = 0; }); > > I'm not clear on why we clear 'tests' at the end. > > Won't we already be done with this array, by the time we execute this line? > Does something depend on it being emptied? If not, it seems like it'd be > simpler to leave it alone. (But if we do need to empty it, add a comment to > explain why.) The idea is that you use it like so: addAsyncAnimTest(function *() { ... }); // A addAsyncAnimTest(function *() { ... }); // B addAsyncAnimTest(function *() { ... }); // C runAllAsyncAnimTests() // Runs A, B, C .then(function() { addAsyncAnimTest(function *() { ... }); // D addAsyncAnimTest(function *() { ... }); // E runAllAsyncAnimTests(); // Runs D, E }); So I thought it makes sense to clear the internal list of tests after running them. What do you think? I guess I could just drop it and then add a "clear-list" type function later if it becomes necessary. Or alternatively name it "queueAsyncAnimTest". > @@ +216,5 @@ > > + // function has completed. > > + // > > + // This arrangement is based on add_task() which is currently only available > > + // in mochitest-chrome (bug 872229). Once add_task is available in > > + // mochitest-plain we can remove this function and use add_task instead. > > (This implies that there are plans to make add_task available in > mochitest-plain -- is that actually the case? If so, is there a bug for > this, which we can link to here? I couldn't find one in a quick bugzilla > search.) There are no concrete plans that I'm aware of but it seems likely to happen eventually. I've changed the comment to say "If add_task becomes available...".

(In reply to Daniel Holbert [:dholbert] from comment #19) > Comment on attachment 8434696 [details] [diff] [review] > part 6 - Fix handling of 3d matrices in omta_is and co. > > Yikes! Do you know why this 3D matrix slice() issue hasn't caused problems? > (Were we just not using it for actual 4x4 matrices yet?) That's right. We'd never encountered 3D matrices in this code path before.

(In reply to Daniel Holbert [:dholbert] from comment #21) > Or instead of commenting it out, you could even copypaste it into a bug > comment here, and reference the bug comment's URL from the ok(false) message. Possible code for getting the determinant of a 3D matrix: return matrix[0][3] * matrix[1][2] * matrix[2][1] * matrix[3][0] - matrix[0][2] * matrix[1][3] * matrix[2][1] * matrix[3][0] - matrix[0][3] * matrix[1][1] * matrix[2][2] * matrix[3][0] + matrix[0][1] * matrix[1][3] * matrix[2][2] * matrix[3][0] + matrix[0][2] * matrix[1][1] * matrix[2][3] * matrix[3][0] - matrix[0][1] * matrix[1][2] * matrix[2][3] * matrix[3][0] - matrix[0][3] * matrix[1][2] * matrix[2][0] * matrix[3][1] + matrix[0][2] * matrix[1][3] * matrix[2][0] * matrix[3][1] + matrix[0][3] * matrix[1][0] * matrix[2][2] * matrix[3][1] - matrix[0][0] * matrix[1][3] * matrix[2][2] * matrix[3][1] - matrix[0][2] * matrix[1][0] * matrix[2][3] * matrix[3][1] + matrix[0][0] * matrix[1][2] * matrix[2][3] * matrix[3][1] + matrix[0][3] * matrix[1][1] * matrix[2][0] * matrix[3][2] - matrix[0][1] * matrix[1][3] * matrix[2][0] * matrix[3][2] - matrix[0][3] * matrix[1][0] * matrix[2][1] * matrix[3][2] + matrix[0][0] * matrix[1][3] * matrix[2][1] * matrix[3][2] + matrix[0][1] * matrix[1][0] * matrix[2][3] * matrix[3][2] - matrix[0][0] * matrix[1][1] * matrix[2][3] * matrix[3][2] - matrix[0][2] * matrix[1][1] * matrix[2][0] * matrix[3][3] + matrix[0][1] * matrix[1][2] * matrix[2][0] * matrix[3][3] + matrix[0][2] * matrix[1][0] * matrix[2][1] * matrix[3][3] - matrix[0][0] * matrix[1][2] * matrix[2][1] * matrix[3][3] - matrix[0][1] * matrix[1][0] * matrix[2][2] * matrix[3][3] + matrix[0][0] * matrix[1][1] * matrix[2][2] * matrix[3][3];

(In reply to Brian Birtles (:birtles) from comment #23) > The idea is that you use it like so: > > addAsyncAnimTest(function *() { ... }); // A > addAsyncAnimTest(function *() { ... }); // B > addAsyncAnimTest(function *() { ... }); // C > runAllAsyncAnimTests() // Runs A, B, C > .then(function() { > addAsyncAnimTest(function *() { ... }); // D > addAsyncAnimTest(function *() { ... }); // E > runAllAsyncAnimTests(); // Runs D, E > }); > > So I thought it makes sense to clear the internal list of tests after > running them. What do you think? Hmm.... I worry a bit that this sort of hypothetical control flow -- with multiple chained runAllAsyncAnimTests calls -- could get a bit hard to follow. What's the benefit of that kind of structure, as compared to just making all of our addAsyncAnimTest() calls up-front? (I think that's what we do with your patches right now, right?) (I can imagine that one use-case would be if there were some state that we need to set up before starting tests D and E, in your example... but it's probably better for any such state to be built-up/torn-down by those test functions themselves, individually). Anyway, if you think the array-clearing is useful, I don't care enough to push back too hard on it. :) Maybe just add a comment explaining it (e.g. "Clear tests in case more addAsyncAnimTest / runAllAsyncAnimTests calls are coming.)

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f3384e94822 https://hg.mozilla.org/integration/mozilla-inbound/rev/4900384bce9f https://hg.mozilla.org/integration/mozilla-inbound/rev/1c0a44e761ec https://hg.mozilla.org/integration/mozilla-inbound/rev/49775bc6825e https://hg.mozilla.org/integration/mozilla-inbound/rev/0f6fd048a317 https://hg.mozilla.org/integration/mozilla-inbound/rev/99f24b59c782 https://hg.mozilla.org/integration/mozilla-inbound/rev/1d7898e89dab https://hg.mozilla.org/integration/mozilla-inbound/rev/cea6a1920ce7 https://hg.mozilla.org/integration/mozilla-inbound/rev/441a1c280cd8 (In reply to Daniel Holbert [:dholbert] from comment #26) > (In reply to Brian Birtles (:birtles) from comment #23) > > The idea is that you use it like so: > > > > addAsyncAnimTest(function *() { ... }); // A > > addAsyncAnimTest(function *() { ... }); // B > > addAsyncAnimTest(function *() { ... }); // C > > runAllAsyncAnimTests() // Runs A, B, C > > .then(function() { > > addAsyncAnimTest(function *() { ... }); // D > > addAsyncAnimTest(function *() { ... }); // E > > runAllAsyncAnimTests(); // Runs D, E > > }); > > > > So I thought it makes sense to clear the internal list of tests after > > running them. What do you think? > > Hmm.... I worry a bit that this sort of hypothetical control flow -- with > multiple chained runAllAsyncAnimTests calls -- could get a bit hard to > follow. I've removed this list-clearing behaviour for now since it's not needed. (In reply to Brian Birtles (:birtles) from comment #25) > (In reply to Daniel Holbert [:dholbert] from comment #21) > > Or instead of commenting it out, you could even copypaste it into a bug > > comment here, and reference the bug comment's URL from the ok(false) message. > > Possible code for getting the determinant of a 3D matrix: > > return matrix[0][3] * matrix[1][2] * matrix[2][1] * matrix[3][0] > ... It turns out this was needed because a couple of the test cases do produce 3D matrices so I've left it in.

(In reply to Brian Birtles (:birtles) from comment #0) > We also need to increase test coverage of CSS transitions running on the > compositor in general. For example, there are no tests that check that a > transition with a delay is run correctly (and I know from ad-hoc testing > that at least at one point they did not). I decided to look into this in a separate bug and make this bug just about making the OMTA transitions tests confirm the animations are running on the compositor.

https://hg.mozilla.org/mozilla-central/rev/1f3384e94822 https://hg.mozilla.org/mozilla-central/rev/4900384bce9f https://hg.mozilla.org/mozilla-central/rev/1c0a44e761ec https://hg.mozilla.org/mozilla-central/rev/49775bc6825e https://hg.mozilla.org/mozilla-central/rev/0f6fd048a317 https://hg.mozilla.org/mozilla-central/rev/99f24b59c782 https://hg.mozilla.org/mozilla-central/rev/1d7898e89dab https://hg.mozilla.org/mozilla-central/rev/cea6a1920ce7 https://hg.mozilla.org/mozilla-central/rev/441a1c280cd8