Closed Bug 964646 Opened 10 years ago Closed 10 years ago

Add tests for async CSS Animations (OMTA)

Categories

(Core :: Graphics: Layers, defect)

Product:
Core
Component:
Graphics: Layers
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla32

People

(Reporter: birtles, Assigned: birtles)

References

Details

Attachments

(21 files, 22 obsolete files)

part 1 - Add common OMTA test runner to animation_utils.js
7.28 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
part 2 - Add OMTA version of test_animations.html fill mode tests
19.67 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
part 3 - Refactor OMTA test methods to include opacity too
13.18 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
part 4 - Add OMTA version of test_animations.html animation list tests
10.28 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
part 5 - Add OMTA version of test_animations.html keyframe tests
11.31 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
part 6 - Add OMTA tests for timing functions on keyframes
7.56 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
part 7 - Handle NaN values when comparing matrices
2.83 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
part 8 - Add OMTA tests for animation-name
3.04 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
part 10 - Fix floating point precision issues when comparing matrices
3.35 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
part 9 - Add OMTA tests for animation-iteration-count
8.70 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
part 11 - Add OMTA tests for animation-direction
6.75 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
part 12 - Add OMTA tests for animation-play-state
10.24 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
part 13 - Add OMTA tests for animation-delay
7.04 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
part 14 - Add OMTA tests for multi-property animations
5.18 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
part 15 - Add OMTA tests for sampling animations with same timestamp
1.96 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
part 16 - Add OMTA tests for !important rules and animations
3.23 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
part 17 - Add RunningOn.TodoMainThread for marking animations that are known to run on the compositor when they should not
3.42 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
part 18 - Add omta_todo_is for marking OMTA animations that are known to fail.
4.82 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
part 19 - Add OMTA tests for restyling interaction
3.12 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
part 20 - Add OMTA tests for cascading between keyframe rules
7.29 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
part 21 - Add OMTA tests for animation list lengths and dynamic style rule changes
5.77 KB, patch
dbaron
: review+
Details | Diff | Splinter Review 
Splitting this off from bug 788549 which I'll mark as specifically covering CSS Transitions only.
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.
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.
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.
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.
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.
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)
David, if you have any suggestions regarding comment 6 they'd be very welcome.
Flags: needinfo?(dzbarsky)
(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)
(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).
(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)
Every other exposed method in nsDOMWindowUtils except getViewPortInfo and
getViewId performs this check. This patch makes getOMTAOrComputedStyle check the
called is chrome as well.
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.
Attachment #8366479 - Attachment is obsolete: true
Attachment #8366482 - Attachment is obsolete: true
Attachment #8366484 - Attachment is obsolete: true
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)
Attachment #8366485 - Attachment is obsolete: true
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.
Attachment #8379512 - Attachment is obsolete: true
Attachment #8379513 - Attachment is obsolete: true
Attachment #8379514 - Attachment is obsolete: true
Attachment #8379516 - Attachment is obsolete: true
Moved GetOMTAStyle refactoring to bug 979658
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.
Attachment #8379517 - Attachment is obsolete: true
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
(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
(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.
Common test runner as described in comment 19
Attachment #8396171 - Flags: review?(dbaron)
Attachment #8387255 - Attachment is obsolete: true
(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);
Attachment #8396171 - Attachment is obsolete: true
Attachment #8396171 - Flags: review?(dbaron)
Reworked to explicitly test values on compositor thread in all cases
Attachment #8396862 - Flags: review?(dbaron)
Attachment #8396173 - Attachment is obsolete: true
Attachment #8396862 - Attachment is obsolete: true
Attachment #8396862 - Flags: review?(dbaron)
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)
Attachment #8396957 - Attachment is obsolete: true
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)
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)
Attachment #8397516 - Attachment is obsolete: true
Attachment #8397516 - Flags: review?(dbaron)
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.
Attachment #8397630 - Flags: review?(dbaron)
Attachment #8397632 - Flags: review?(dbaron)
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.
Attachment #8397630 - Attachment is obsolete: true
Attachment #8397632 - Attachment is obsolete: true
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 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 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 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+
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 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.
(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!
(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.
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.
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.
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.
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).
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.
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.
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)
Attachment #8408837 - Flags: review?(dbaron)
Attachment #8408838 - Flags: review?(dbaron)
Attachment #8408839 - Attachment is obsolete: true
Attachment #8408841 - Flags: review?(dbaron)
Attachment #8408842 - Attachment is obsolete: true
(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.
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)
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)
Attachment #8409542 - Attachment is obsolete: true
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.
Attachment #8409543 - Attachment is obsolete: true
Attachment #8410757 - Attachment is obsolete: true
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.
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!
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 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 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 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+
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+
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
(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
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.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Depends on: 1021766
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: