Closed
Bug 964646
Opened 11 years ago
Closed 11 years ago
Add tests for async CSS Animations (OMTA)
Categories
(Core :: Graphics: Layers, defect)
Core
Graphics: Layers
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: birtles, Assigned: birtles)
References
Details
Attachments
(21 files, 22 obsolete files)
(deleted),
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
Splitting this off from bug 788549 which I'll mark as specifically covering CSS Transitions only.
Assignee | ||
Updated•11 years ago
|
Blocks: b2g-testing, 788522
Assignee | ||
Comment 1•11 years ago
|
||
nsDOMWindowUtils.getOMTAOrComputedStyle falls back to using getComputedStyle
when an OMTA style is not available. However, in order to be sure we are testing
OMTA this patch adds getOMTAStyle that returns an empty string if no OMTA style
is available.
This patch also includes some minor stylistic tweaks. The method signature for
getOMTAOrComputedStyle now takes an nsIDOMElement parameter rather than
nsIDOMNode in order to simplify error-checking. (When we support OMTA of
pseudo-elements we will have to adjust the method signature but for now we only
support elements.) Also, some lines have been wrapped, ErrorResult is
declared closer to where it is used, and the return value aResult is only
truncated when returning NS_OK.
Assignee | ||
Comment 2•11 years ago
|
||
LayerTransactionParent::RecvGetTransform takes care to reverse all the
transformations applied by AsyncCompositionManager::SampleValue to the CSS
values calculated so that it can return CSS values for testing. However, it
fails to revert the conversion from CSS pixels to device pixels applied to the
translation components of the transform by
nsStyleTransformMatrix::ProcessTranslatePart as called by
nsDisplayTransform::GetResultingTransformMatrix.
This patch converts the resulting transform's translation components from device
pixels back to CSS pixels. It also adds documentation for the other operations
in LayerTransactionParent::RecvGetTransform.
Assignee | ||
Comment 3•11 years ago
|
||
PLayerTransaction.GetTransform doesn't actually return the same kind of value
when the transform on the layer is not set by animation. This is because it uses
information stored with the animation to undo various transforms. We shouldn't
pretend to return something useful/similar when we don't have that information
available.
This patch renames GetTransform to GetAnimationTransform and makes it take an
extra parameter to indicate if the layer's transform is set by animation or not.
Assignee | ||
Comment 4•11 years ago
|
||
This patch adds an additional mochitest for specifically targetting CSS
Animations that run on the compositor thread. The content of the test mimicks
test_animations.html but using properties whose animations are expected to run
on the compositor thread.
For platforms where OMTC is not enabled by default the test is skipped since
OMTC cannot be enabled at run-time. For platforms where OMTC is enabled by
default, OMTA is switched on as necessary.
Assignee | ||
Comment 5•11 years ago
|
||
These tests are very much a work in progress. For my own notes, they can be run on Windows using:
export EXTRA_TEST_ARGS='--test-path=layout/style/test/test_animations_omta.html --setpref=layers.offmainthreadcomposition.enabled=true --setpref=layers.offmainthreadcomposition.async-animations=true --setpref=layers.offmainthreadcomposition.log-animations=true'
Then, from the obj dir:
mozmake mochitest-plain
Currently the tests don't pass for me on Windows because delayed animations seem to get started very late on Windows. Running a manual test I see the same behavior so I don't think the tests themselves are to blame.
Also, currently we don't test values when the animation isn't actually running (e.g. when its filling). We probably should:
1. Add a LayerTransaction.GetTransform that just calls GetLocalTransform on the shadow layer
2. In DOMWindowUtils::GetOMTAStyle, if isAnimated is false, call GetTransform
The problem is GetTransform returns the transform with the translation from the reference frame incorporated. This is set in nsDisplayTransform::GetTransform. I'm not sure yet how to recover this reference frame so we can pull out that translation component.
Assignee | ||
Comment 6•11 years ago
|
||
I'm having trouble making these tests deterministic. The problem is knowing how long to wait for values to appear on the compositor thread.
The problem is further complicated by the fact that I have the refresh driver under test control so it doesn't tick unless I tell it to.
For an animation that begins right away I can:
a) Call elem.clientTop to force style resolution which will trigger nsAnimationManager::BuildAnimations
b) Then I can wait for pending paints to complete by making the test into a chrome test and using paint_listener.js.
For animations that start with a delay, however, I'm not sure what to wait on. BuildAnimations creates the animations during the delay phase but won't add them to the layer until the end of the delay. Once we advance the refresh driver to that point there are no pending paints so I can't wait on that. I can just keep polling the compositor thread but that will probably lead to random failures. Should there be a scheduled paint? Is there something I can tickle to trigger a paint to be scheduled? Can I force a synchronous paint in GetOMTAStyle or when I advance the refresh driver?
I've been digging into this all week but I could do with some pointers.
Flags: needinfo?(ncameron)
Assignee | ||
Comment 7•11 years ago
|
||
David, if you have any suggestions regarding comment 6 they'd be very welcome.
Flags: needinfo?(dzbarsky)
Comment 8•11 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #6)
Is there
> something I can tickle to trigger a paint to be scheduled? Can I force a
> synchronous paint in GetOMTAStyle or when I advance the refresh driver?
>
Does calling Invalidate on the element's frame in GetOMTAStyle help? I think if you invalidate and then wait for a setTimeout(0) you should get a paint
Flags: needinfo?(dzbarsky)
Assignee | ||
Comment 9•11 years ago
|
||
(In reply to David Zbarsky (:dzbarsky) from comment #8)
> Does calling Invalidate on the element's frame in GetOMTAStyle help? I think
> if you invalidate and then wait for a setTimeout(0) you should get a paint
Thanks David, that seems to do the trick.
It's a bit odd I guess--with this approach you have to call getOMTAStyle twice and spin the event loop in-between to be sure you're seeing the latest result. But I don't have any better ideas short of making getOMTAStyle return a promise.
I'll check tomorrow to see if this removes the need for paint_listener.js (previously just using SimpleTest.executeSoon wasn't enough to ensure the paint happened but with this explicit invalidation it may work).
Assignee | ||
Comment 10•11 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #9)
> (In reply to David Zbarsky (:dzbarsky) from comment #8)
> > Does calling Invalidate on the element's frame in GetOMTAStyle help? I think
> > if you invalidate and then wait for a setTimeout(0) you should get a paint
>
> Thanks David, that seems to do the trick.
This seems like a bug. It seems like we should be already be invalidating this elsewhere, i.e. I should only have to tick the refresh driver. I filed bug 975261 which shows that in some cases we're not correctly starting animations with a delay. This may be the same bug I'm seeing here.
Flags: needinfo?(ncameron)
Assignee | ||
Comment 11•11 years ago
|
||
Every other exposed method in nsDOMWindowUtils except getViewPortInfo and
getViewId performs this check. This patch makes getOMTAOrComputedStyle check the
called is chrome as well.
Assignee | ||
Comment 12•11 years ago
|
||
nsDOMWindowUtils.getOMTAOrComputedStyle falls back to using getComputedStyle
when an OMTA style is not available. However, in order to be sure we are testing
OMTA this patch adds getOMTAStyle that returns an empty string if no OMTA style
is available.
This patch also includes some minor stylistic tweaks. The method signature for
getOMTAOrComputedStyle now takes an nsIDOMElement parameter rather than
nsIDOMNode in order to simplify error-checking. (When we support OMTA of
pseudo-elements we will have to adjust the method signature but for now we only
support elements.) Also, some lines have been wrapped, ErrorResult is
declared closer to where it is used, and the return value aResult is only
truncated when returning NS_OK.
Assignee | ||
Updated•11 years ago
|
Attachment #8366479 -
Attachment is obsolete: true
Assignee | ||
Comment 13•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #8366482 -
Attachment is obsolete: true
Assignee | ||
Comment 14•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #8366484 -
Attachment is obsolete: true
Assignee | ||
Comment 15•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #8379517 -
Attachment description: part 4 - Add OMTA version of test_animations.html fill mode tests (Chrome test) → part 5 - Add OMTA version of test_animations.html fill mode tests (Chrome test)
Assignee | ||
Updated•11 years ago
|
Attachment #8366485 -
Attachment is obsolete: true
Assignee | ||
Comment 16•11 years ago
|
||
The tests in part 5 currently fail on Linux (try run here: https://tbpl.mozilla.org/?tree=Try&rev=dd4a8c97d9df) not sure why.
Also, the call to 'renderDocument' shouldn't be necessary. Hopefully bug 975261 will shed some light on that.
Assignee | ||
Updated•11 years ago
|
Attachment #8379512 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #8379513 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #8379514 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #8379516 -
Attachment is obsolete: true
Assignee | ||
Comment 17•11 years ago
|
||
Moved GetOMTAStyle refactoring to bug 979658
Assignee | ||
Comment 18•11 years ago
|
||
This patch adds an additional mochitest for specifically targetting CSS
Animations that run on the compositor thread. The content of the test mimicks
test_animations.html but using properties whose animations are expected to run
on the compositor thread.
For platforms where OMTC is not enabled by default the test is skipped since
OMTC cannot be enabled at run-time. For platforms where OMTC is enabled by
default, OMTA is switched on as necessary.
Assignee | ||
Updated•11 years ago
|
Attachment #8379517 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #8387255 -
Attachment description: Add OMTA version of test_animations.html fill mode tests (Chrome test) → Add OMTA version of test_animations.html fill mode tests
Updated•11 years ago
|
Blocks: enable-omt-animations
Comment 19•11 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #18)
> For platforms where OMTC is not enabled by default the test is skipped since
> OMTC cannot be enabled at run-time. For platforms where OMTC is enabled by
> default, OMTA is switched on as necessary.
It seems like this is something we're going to want to do in quite a few places (I was thinking about similar things for tests I want to write as part of bug 960465), so perhaps the code needed to do this could be refactored into a separate JS file that multiple tests can use?
I'd imagine having an API something like this:
test_if_omt_animations(function() {
// tests to run if OMT animations are supported
});
Then, test_if_omt_animations could, internally:
* call pushPrefEnv to set up the appropriate prefs
* do a brief internal test that getOMTAStyle works as expected
- if that test fails, use either is() or todo() depending on
platform to report a single test failure (TEST-KNOWN-FAIL on
some platforms, TEST-UNEXPECTED-FAIL on platforms where we
expect OMTA)
- if that test passes, call the callback function
This would let us have mochitests that:
* run on tinderbox on platforms where we can expect OMTA to be
supported
* can be run by developers on other platforms if they're using a
configuration with OMTA supported
Assignee | ||
Comment 20•11 years ago
|
||
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #19)
> (In reply to Brian Birtles (:birtles) from comment #18)
> > For platforms where OMTC is not enabled by default the test is skipped since
> > OMTC cannot be enabled at run-time. For platforms where OMTC is enabled by
> > default, OMTA is switched on as necessary.
>
> It seems like this is something we're going to want to do in quite a few
> places (I was thinking about similar things for tests I want to write as
> part of bug 960465), so perhaps the code needed to do this could be
> refactored into a separate JS file that multiple tests can use?
Sounds great.
> I'd imagine having an API something like this:
>
> test_if_omt_animations(function() {
> // tests to run if OMT animations are supported
> });
>
> Then, test_if_omt_animations could, internally:
> * call pushPrefEnv to set up the appropriate prefs
> * do a brief internal test that getOMTAStyle works as expected
> - if that test fails, use either is() or todo() depending on
> platform to report a single test failure (TEST-KNOWN-FAIL on
> some platforms, TEST-UNEXPECTED-FAIL on platforms where we
> expect OMTA)
> - if that test passes, call the callback function
Yes, sounds good. I've noticed that after calling pushPrefEnv the first call to getOMTAStyle fails. I think subsequent calls occur after a waitForAllPaints so there may be some asynchronous overhead required for setting up OMTA. This method may need to spin the event loop before calling getOMTAStyle.
Also, I'm seeing test failures in bug 975261 on B2G ICS despite the fact that OMTA is enabled and OMTC is available. I'm not sure why yet but there may be further requirements to ensuring OMTA is running.
Assignee | ||
Comment 21•11 years ago
|
||
Common test runner as described in comment 19
Attachment #8396171 -
Flags: review?(dbaron)
Assignee | ||
Comment 22•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #8387255 -
Attachment is obsolete: true
Assignee | ||
Comment 23•11 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #21)
> Created attachment 8396171 [details] [diff] [review]
> part 1 - Add common OMTA test runner to animation_utils.js
>
> Common test runner as described in comment 19
On further thought, I think the automatic calling of SimpleTest.finish is probably not necessary and may confuse authors. It's probably better to require explicitly doing this, as in:
SimpleTest.waitForExplicitFinish();
runOMTATest(testFunc, SimpleTest.finish);
Assignee | ||
Comment 24•11 years ago
|
||
Depends on: 986367
Assignee | ||
Comment 25•11 years ago
|
||
Add the tweak mentioned in comment 23
Attachment #8396861 -
Flags: review?(dbaron)
Assignee | ||
Updated•11 years ago
|
Attachment #8396171 -
Attachment is obsolete: true
Attachment #8396171 -
Flags: review?(dbaron)
Assignee | ||
Comment 26•11 years ago
|
||
Reworked to explicitly test values on compositor thread in all cases
Attachment #8396862 -
Flags: review?(dbaron)
Assignee | ||
Updated•11 years ago
|
Attachment #8396173 -
Attachment is obsolete: true
Assignee | ||
Comment 27•11 years ago
|
||
Updated try run: https://tbpl.mozilla.org/?tree=Try&rev=4f37e604d4a0
Assignee | ||
Comment 28•11 years ago
|
||
Attachment #8396887 -
Flags: review?(dbaron)
Assignee | ||
Updated•11 years ago
|
Attachment #8396862 -
Attachment is obsolete: true
Attachment #8396862 -
Flags: review?(dbaron)
Assignee | ||
Comment 29•11 years ago
|
||
Assignee | ||
Comment 30•11 years ago
|
||
Assignee | ||
Comment 31•11 years ago
|
||
Tests for animation list handling on the compositor thread. Some checks are
currently marked todo_is because they depend on Bug 980769 in order to pass.
Attachment #8397516 -
Flags: review?(dbaron)
Assignee | ||
Updated•11 years ago
|
Attachment #8396957 -
Attachment is obsolete: true
Assignee | ||
Comment 32•11 years ago
|
||
This patch takes the compareTransform utility methods and makes them more
generic so they can be used for testing opacity too (the other property
currently animated on the compositor thread).
The naming omta_is and omta_is_approx is intended to mirror is and is_approx in
test_animations.html. We don't include omta_todo_is since there are multiple
ways the test could fail (compositor thread value is not set, compositor thread
value doesn't match computed style etc.) and so defining a suitable todo method
is difficult (we'd have to run all tests and assert that at least one failed).
Attachment #8397629 -
Flags: review?(dbaron)
Assignee | ||
Comment 33•11 years ago
|
||
I thought I'd take the chance to rebase these patches on top of some fixes to the test framework
Attachment #8397630 -
Flags: review?(dbaron)
Assignee | ||
Updated•11 years ago
|
Attachment #8397516 -
Attachment is obsolete: true
Attachment #8397516 -
Flags: review?(dbaron)
Assignee | ||
Comment 34•11 years ago
|
||
Attachment #8397632 -
Flags: review?(dbaron)
Assignee | ||
Comment 35•11 years ago
|
||
Sorry for the noise with all the patch updates. I won't go adding any more tests until I get review feedback for these first few patches in case there's something fundamentally amiss with the approach I've taken.
Assignee | ||
Comment 36•11 years ago
|
||
Try run with current queue: https://tbpl.mozilla.org/?tree=Try&rev=dba30c824d14
Assignee | ||
Updated•11 years ago
|
Attachment #8397630 -
Flags: review?(dbaron)
Assignee | ||
Updated•11 years ago
|
Attachment #8397632 -
Flags: review?(dbaron)
Assignee | ||
Comment 37•11 years ago
|
||
Cancelling review for parts 4 and 5 due to test failures on B2G. Opacity appears to be handled differently to transforms. Will debug next week.
Assignee | ||
Comment 38•11 years ago
|
||
Fix tests so they pass on B2G
Attachment #8400294 -
Flags: review?(dbaron)
Assignee | ||
Updated•11 years ago
|
Attachment #8397630 -
Attachment is obsolete: true
Assignee | ||
Comment 39•11 years ago
|
||
Attachment #8400295 -
Flags: review?(dbaron)
Assignee | ||
Updated•11 years ago
|
Attachment #8397632 -
Attachment is obsolete: true
Assignee | ||
Comment 40•11 years ago
|
||
Parts 4 and 5 now pass on B2G: https://tbpl.mozilla.org/?tree=Try&rev=e2da2b2e05c8
For my own reference, the test failed on B2G because:
* they failed to flush styles after changing specified style when waiting for paints (although I'm still not exactly sure why this worked on other platforms with OMTC enabled)
* they positioned things outside the viewport on B2G (left: 300px is outside the viewport so we never generated display list items for those things)
Comment 41•11 years ago
|
||
Comment on attachment 8396861 [details] [diff] [review]
part 1 - Add common OMTA test runner to animation_utils.js
>+// This function also does an internal test to verify that OMTA is working at
>+// all so that if OMTA is not functioning correctly only a single failure is
>+// produced.
maybe add "when it is expected to function" after the "not functioning
correctly" (probably with some commas)
>+ var onSkip = aOnSkip || function() {};
>+ utils = SpecialPowers.DOMWindowUtils,
>+ expectOMTA = utils.layerManagerRemote &&
>+ // ^ Off-main thread animation cannot be used if off-main
>+ // thread composition (OMTC) is not available
>+ SpecialPowers.getBoolPref(OMTAPrefKey);
Put var before each of the three, and change the , after the second
to a ;. (Note you had a ; after the first, so you were introducing
globals!)
Also, I think you should require an onSkip function rather than
having the "|| function() {}" since every caller has to at least
call SimpleTest.finish.
I'm new to reading promises code, so if you want somebody to review
who isn't, you should ask -- but I think it's probably fine if you've
tested that it works as you expect.
r=dbaron
Attachment #8396861 -
Flags: review?(dbaron) → review+
Comment 42•11 years ago
|
||
Comment on attachment 8396887 [details] [diff] [review]
part 2 - Add OMTA version of test_animations.html fill mode tests
I'm not reviewing these (patches 2-5) all that closely, but I think that's fine. (Sorry, I should have decided to not review them all that closely a few days ago..)
Attachment #8396887 -
Flags: review?(dbaron) → review+
Comment 43•11 years ago
|
||
Comment on attachment 8397629 [details] [diff] [review]
part 3 - Refactor OMTA test methods to include opacity too
>+ return convertArrayTo3dMatrix([1, 0, 0, 1, 0 ,0]);
fix the misplaced final comma
If you need a todo mechanism, I expect you'll want to pass some sort
of complex parameter to the function. But if you don't need it yet,
don't bother inventing it yet.
Attachment #8397629 -
Flags: review?(dbaron) → review+
Updated•11 years ago
|
Attachment #8400294 -
Flags: review?(dbaron) → review+
Comment 44•11 years ago
|
||
Comment on attachment 8400295 [details] [diff] [review]
part 5 - Add OMTA version of test_animations.html keyframe tests
Have you gone back and tested any of the recently-fixed OMTA bugs to see if they're covered by these tests? If so, it would be useful to comment both here and in the fixed bugs. (And I think it probably would be useful to check, at least assuming the patches can be reverted or disabled in a straightforward way today, and assuming the patches fix things that should be covered by these tests, since it will validate that the tests are testing what is expected.)
Attachment #8400295 -
Flags: review?(dbaron) → review+
Comment 45•11 years ago
|
||
Comment on attachment 8396861 [details] [diff] [review]
part 1 - Add common OMTA test runner to animation_utils.js
>+// Since this function relies on various asynchronous operations, the caller is
>+// responsible for calling SimpleTest.waitForExplicitFinish() before calling
>+// this and SimpleTest.finish() within aTestFunction and aOnSkip.
>+function runOMTATest(aTestFunction, aOnSkip) {
>+ const OMTAPrefKey = "layers.offmainthreadcomposition.async-animations";
>+ var onSkip = aOnSkip || function() {};
>+ utils = SpecialPowers.DOMWindowUtils,
>+ expectOMTA = utils.layerManagerRemote &&
>+ // ^ Off-main thread animation cannot be used if off-main
>+ // thread composition (OMTC) is not available
>+ SpecialPowers.getBoolPref(OMTAPrefKey);
One other comment here -- it seems like if we have OMTC, we ought to be able to change the OMTA pref and run the tests. Probably best to do that as a followup patch after landing these, though.
Assignee | ||
Comment 46•11 years ago
|
||
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #44)
> Comment on attachment 8400295 [details] [diff] [review]
> part 5 - Add OMTA version of test_animations.html keyframe tests
>
> Have you gone back and tested any of the recently-fixed OMTA bugs to see if
> they're covered by these tests? If so, it would be useful to comment both
> here and in the fixed bugs. (And I think it probably would be useful to
> check, at least assuming the patches can be reverted or disabled in a
> straightforward way today, and assuming the patches fix things that should
> be covered by these tests, since it will validate that the tests are testing
> what is expected.)
Yes, the issues in bug 975261 were all identified using these tests (as were the issues in bug 988161 which turned out to be a dupe of bug 980769). I decided to file those as a separate bug with separate reduced test cases since they arose largely as a product of the way the tests were structured such that if test_animations_omta.html were refactored in such a way that inadvertently introduced an additional style flush, the bug could be masked.
I have just now tested that reverting the changes in bug 975261 causes these tests to fail.
I'm fairly sure if we revert bug 828173 these tests will fail but I haven't yet tried reverting those patches. Will try next week.
Thanks for all the reviews!
Assignee | ||
Comment 47•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc8ad1175324
https://hg.mozilla.org/integration/mozilla-inbound/rev/da72761ebf82
https://hg.mozilla.org/integration/mozilla-inbound/rev/33a00dec3dbd
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5f9279dbaf4
https://hg.mozilla.org/integration/mozilla-inbound/rev/8bdb40bd3103
Keywords: leave-open
Comment 48•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/cc8ad1175324
https://hg.mozilla.org/mozilla-central/rev/da72761ebf82
https://hg.mozilla.org/mozilla-central/rev/33a00dec3dbd
https://hg.mozilla.org/mozilla-central/rev/f5f9279dbaf4
https://hg.mozilla.org/mozilla-central/rev/8bdb40bd3103
Flags: in-testsuite+
Assignee | ||
Comment 49•11 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #46)
> I'm fairly sure if we revert bug 828173 these tests will fail but I haven't
> yet tried reverting those patches. Will try next week.
Verified that these tests fail when reverting bug 828173 and added a comment to that bug to that effect.
Assignee | ||
Comment 50•11 years ago
|
||
This patch converts the tests in test_animations.html under the heading,
"css3-animations: 3.1. Timing functions for keyframes" to an equivalent version
for testing animations that run on the compositor thread.
Assignee | ||
Comment 51•11 years ago
|
||
The test harness code for normalizing transform inputs to a standard form for
comparison fails to detect the case where the input is a string such as
{ tx: "20px" }
instead of:
{ tx: 20 }
When we go to compare matrix components we fail if:
Math.abs(a.comp - b.comp) > tolerance
But if a.comp or b.comp is a string, we'll get NaN on the LHS and
"NaN > tolerance" will return false so we'll skip the failure handling and
continue onto the next component. That means if we have input { tx: "30px" } and
we get "20" as the x-translation component we'll pass the test.
This patch fixes this condition to check for isNaN.
We *could* also just drop a few .map(parseFloat) calls into
convertObjectTo3dMatrix and convertArrayTo3dMatrix to ensure "20px" becomes 20
but there may be situations where that masks bugs (since "20px" and "20em" turn
into the same thing) so for now this minimal fix should be enough.
Assignee | ||
Comment 52•11 years ago
|
||
This patch adds a version of the tests in test_animations.html under the
heading, "css3-animations: 3.2. The 'animation-name' Property" that tests the
same features when animations are running on the compositor thread.
Assignee | ||
Comment 53•11 years ago
|
||
This patch adds a version of tests in test_animations.html under the heading,
"css3-animation: 3.5 The 'animation-iteration-count' Property for animations
that run on the compositor thread.
These tests surface two issues:
a) Animations cut short by the iteration count are not removed immediately from
the compositor (filed as bug 998162).
b) In some cases precision errors lead to discrepencies between the OMTA style
and computed style (to be fixed in a subsequent patch).
Assignee | ||
Comment 54•11 years ago
|
||
This patch addresses and issue where the OMTA style and computed style were not
comparing equal in one particular case.
In this case AddTransformTranslate in nsStyleAnimation would give us
a translate-y value of 94.331673 in both cases (i.e. when calculating the
animated value on the compositor thread or when fetching computed style).
For the OMTA case, hoever, after we apply additional layer transformations and
then reverse them (so we can query the CSS value) we'd end up with 94.331642,
a difference of 0.000031. The reversing procedure is only used for testing so
the actual error introduced here by the additional layer transformations is
probably less.
Unfortunately, when we pass 94.331642 this along to MatrixToCSSValue we get back
matrix(1, 0, 0, 1, 94.3316) since it only outputs 6 digits of precision.
On the other hand, on the computed style end we'd pass 94.331673 to
MatrixToCSSValue which gives us matrix(1, 0, 0, 1, 94.3317), so the error swells
from 0.000031 to 0.0001.
Then when we subtract 94.3316 from 94.3317 in Javascript we get
0.00010000000000331966 due to floating-point precision issues which compares
greater than the default tolerance of 0.0001.
This patch simply adjusts the default tolerance to 0.00011 to accommodate
these floating-point differences.
Assignee | ||
Comment 55•11 years ago
|
||
This patch also ensures that when we have an animation running on the compositor
when it should not that we still compare the values produced on the compositor
and on the main thread so that the visual result is correct even if the
performance characteristics are not.
Assignee | ||
Comment 56•11 years ago
|
||
Try run for parts 6-11: https://tbpl.mozilla.org/?tree=Try&rev=5ee32e8e89af
Assignee | ||
Comment 57•11 years ago
|
||
Comment on attachment 8408835 [details] [diff] [review]
part 6 - Add OMTA tests for timing functions on keyframes
Here is the next round of OMTA tests. There are still more to come. I'll probably add them in batches over the next few weeks.
Attachment #8408835 -
Flags: review?(dbaron)
Assignee | ||
Updated•11 years ago
|
Attachment #8408837 -
Flags: review?(dbaron)
Assignee | ||
Updated•11 years ago
|
Attachment #8408838 -
Flags: review?(dbaron)
Assignee | ||
Comment 58•11 years ago
|
||
Attachment #8409542 -
Flags: review?(dbaron)
Assignee | ||
Updated•11 years ago
|
Attachment #8408839 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #8408841 -
Flags: review?(dbaron)
Assignee | ||
Comment 59•11 years ago
|
||
Attachment #8409543 -
Flags: review?(dbaron)
Assignee | ||
Updated•11 years ago
|
Attachment #8408842 -
Attachment is obsolete: true
Assignee | ||
Comment 60•11 years ago
|
||
Follow-up try run: https://tbpl.mozilla.org/?tree=Try&rev=afcfe6e51ba6
Assignee | ||
Comment 61•11 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #60)
> Follow-up try run: https://tbpl.mozilla.org/?tree=Try&rev=afcfe6e51ba6
Actually, it's still broken due to my incompetence with understanding B2G build scripts.
Assignee | ||
Comment 62•11 years ago
|
||
Comment on attachment 8409542 [details] [diff] [review]
part 9 - Add OMTA tests for animation-iteration-count
Cancelling review for now while I fix this.
Attachment #8409542 -
Flags: review?(dbaron)
Assignee | ||
Comment 63•11 years ago
|
||
Comment on attachment 8409543 [details] [diff] [review]
part 11 - Add RunningOn.TodoMainThread for marking animations that are known to run on the compositor when they should not
This part too will change so cancelling review on this patch for now too.
Attachment #8409543 -
Flags: review?(dbaron)
Assignee | ||
Comment 64•11 years ago
|
||
Attachment #8410753 -
Flags: review?(dbaron)
Assignee | ||
Updated•11 years ago
|
Attachment #8409542 -
Attachment is obsolete: true
Assignee | ||
Comment 65•11 years ago
|
||
Attachment #8410754 -
Flags: review?(dbaron)
Assignee | ||
Comment 66•11 years ago
|
||
Attachment #8410755 -
Flags: review?(dbaron)
Assignee | ||
Comment 67•11 years ago
|
||
This is no longer needed so I'm not putting it up for review, but just leaving it here in case we need this feature later.
Assignee | ||
Updated•11 years ago
|
Attachment #8409543 -
Attachment is obsolete: true
Assignee | ||
Comment 68•11 years ago
|
||
Parts 6-12 look ok on try: https://tbpl.mozilla.org/?tree=Try&rev=b9031b54367c
Assignee | ||
Comment 69•11 years ago
|
||
Attachment #8416255 -
Flags: review?(dbaron)
Assignee | ||
Comment 70•11 years ago
|
||
Attachment #8416257 -
Flags: review?(dbaron)
Assignee | ||
Comment 71•11 years ago
|
||
Attachment #8416258 -
Flags: review?(dbaron)
Assignee | ||
Comment 72•11 years ago
|
||
Attachment #8416259 -
Flags: review?(dbaron)
Assignee | ||
Comment 73•11 years ago
|
||
Attachment #8416260 -
Flags: review?(dbaron)
Assignee | ||
Updated•11 years ago
|
Attachment #8410757 -
Attachment is obsolete: true
Assignee | ||
Comment 74•11 years ago
|
||
Attachment #8416261 -
Flags: review?(dbaron)
Assignee | ||
Comment 75•11 years ago
|
||
Attachment #8416262 -
Flags: review?(dbaron)
Assignee | ||
Comment 76•11 years ago
|
||
Attachment #8416263 -
Flags: review?(dbaron)
Assignee | ||
Comment 77•11 years ago
|
||
Attachment #8416264 -
Flags: review?(dbaron)
Assignee | ||
Comment 78•11 years ago
|
||
That should be all the patches now.
Currently running through try but it passes for me on Windows and B2G emulator
https://tbpl.mozilla.org/?tree=Try&rev=5b00e4459249
Some possible tweaks I could make:
* Add comments explaining when and why we use waitForPaints vs waitForPaintsFlushed
(We could drop the trigger to flush styles in new_div and then just use waitForPaintsFlushed everyhwere)
* Change the order of parameters to omta_is_approx so that the tolerance comes before the "where is it animating" parameter. That would probably make more sense.
Comment 79•11 years ago
|
||
This is some exceptionally fine testing work. I have been following along with a child-like sense of wonder. Thanks a lot for all the effort here. Nice set of fixes for the corner cases you discovered, too. Thanks!
Updated•11 years ago
|
Attachment #8408835 -
Flags: review?(dbaron) → review+
Updated•11 years ago
|
Attachment #8408837 -
Flags: review?(dbaron) → review+
Updated•11 years ago
|
Attachment #8408838 -
Flags: review?(dbaron) → review+
Updated•11 years ago
|
Attachment #8408841 -
Flags: review?(dbaron) → review+
Updated•11 years ago
|
Attachment #8410753 -
Flags: review?(dbaron) → review+
Updated•11 years ago
|
Attachment #8410754 -
Flags: review?(dbaron) → review+
Updated•11 years ago
|
Attachment #8410755 -
Flags: review?(dbaron) → review+
Updated•11 years ago
|
Attachment #8416255 -
Flags: review?(dbaron) → review+
Updated•11 years ago
|
Attachment #8416257 -
Flags: review?(dbaron) → review+
Updated•11 years ago
|
Attachment #8416258 -
Flags: review?(dbaron) → review+
Comment 80•11 years ago
|
||
Comment on attachment 8416261 [details] [diff] [review]
part 18 - Add omta_todo_is for marking OMTA animations that are known to fail.
>+const ExpectComparisonTo = {
>+ Pass: 0,
>+ Fail: 1
>+};
Add a comment that many callers will pass undefined for this parameter.
And maybe make these 1 and 2 instead of 0 and 1, to be clearer that ! isn't safe.
r=dbaron with that
Attachment #8416261 -
Flags: review?(dbaron) → review+
Comment 81•11 years ago
|
||
Comment on attachment 8416259 [details] [diff] [review]
part 16 - Add OMTA tests for !important rules and animations
Might have been nice to do 18 before 16, but ok as is (although you're welcome to reorder as well).
Attachment #8416259 -
Flags: review?(dbaron) → review+
Comment 82•11 years ago
|
||
Comment on attachment 8416260 [details] [diff] [review]
part 17 - Add RunningOn.TodoMainThread for marking animations that are known to run on the compositor when they should not
And I guess that reordering would be moving 16 after both 17 and 18, then...
Attachment #8416260 -
Flags: review?(dbaron) → review+
Comment 83•11 years ago
|
||
Comment on attachment 8416262 [details] [diff] [review]
part 19 - Add OMTA tests for restyling interaction
Not sure whether this test tests anything interesting when the animation is running on the compositor, but may as well keep it now that you have it.
Attachment #8416262 -
Flags: review?(dbaron) → review+
Updated•11 years ago
|
Attachment #8416263 -
Flags: review?(dbaron) → review+
Comment 84•11 years ago
|
||
Comment on attachment 8416264 [details] [diff] [review]
part 21 - Add OMTA tests for animation list lengths and dynamic style rule changes
sorry for the delay getting to these; they were easier than I thought to review
Attachment #8416264 -
Flags: review?(dbaron) → review+
Assignee | ||
Comment 85•11 years ago
|
||
Thanks for the reviews David and the encouragement Andreas!
I haven't landed these yet because I'm hitting an assertion in RestyleTracker.cpp on emulator debug builds:
http://dxr.mozilla.org/mozilla-central/source/layout/base/RestyleTracker.cpp#75
Try run: https://tbpl.mozilla.org/?tree=Try&rev=58e4e6e0d5e7
Assignee | ||
Comment 86•11 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #85)
> I haven't landed these yet because I'm hitting an assertion in
> RestyleTracker.cpp on emulator debug builds:
>
> http://dxr.mozilla.org/mozilla-central/source/layout/base/RestyleTracker.
> cpp#75
Reduced and filed as bug 1012527
Assignee | ||
Comment 87•11 years ago
|
||
After annotating the test for the assertions filed as bug 1012527, these tests look good on try:
https://tbpl.mozilla.org/?tree=Try&rev=e93be63c852e
I'm going to carry forward the r+ to the updated test with the assertion annotations added.
Assignee | ||
Comment 88•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2215666b9dbc
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8345c6d0f78
https://hg.mozilla.org/integration/mozilla-inbound/rev/20d451095cf9
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e4c3d7de7d6
https://hg.mozilla.org/integration/mozilla-inbound/rev/be00b4cb3997
https://hg.mozilla.org/integration/mozilla-inbound/rev/0606fc7897e6
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d34520be880
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f18555a6468
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8794c1163b6
https://hg.mozilla.org/integration/mozilla-inbound/rev/67d88958c18c
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee2fc0c54c81
https://hg.mozilla.org/integration/mozilla-inbound/rev/f179bf862af5
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a19b4d3255b
https://hg.mozilla.org/integration/mozilla-inbound/rev/406c8dfdf0ab
https://hg.mozilla.org/integration/mozilla-inbound/rev/90a9471d2ec7
https://hg.mozilla.org/integration/mozilla-inbound/rev/38bb31689d42
https://hg.mozilla.org/mozilla-central/rev/2215666b9dbc
https://hg.mozilla.org/mozilla-central/rev/a8345c6d0f78
https://hg.mozilla.org/mozilla-central/rev/20d451095cf9
https://hg.mozilla.org/mozilla-central/rev/2e4c3d7de7d6
https://hg.mozilla.org/mozilla-central/rev/be00b4cb3997
https://hg.mozilla.org/mozilla-central/rev/0606fc7897e6
https://hg.mozilla.org/mozilla-central/rev/9d34520be880
https://hg.mozilla.org/mozilla-central/rev/5f18555a6468
https://hg.mozilla.org/mozilla-central/rev/c8794c1163b6
https://hg.mozilla.org/mozilla-central/rev/67d88958c18c
https://hg.mozilla.org/mozilla-central/rev/ee2fc0c54c81
https://hg.mozilla.org/mozilla-central/rev/f179bf862af5
https://hg.mozilla.org/mozilla-central/rev/0a19b4d3255b
https://hg.mozilla.org/mozilla-central/rev/406c8dfdf0ab
https://hg.mozilla.org/mozilla-central/rev/90a9471d2ec7
https://hg.mozilla.org/mozilla-central/rev/38bb31689d42
Assignee | ||
Updated•11 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in
before you can comment on or make changes to this bug.
Description
•