David Zbarsky (:dzbarsky) Assignee
	Description • 12 years ago

      No description provided.

	David Zbarsky (:dzbarsky) Assignee
	Comment 1 • 12 years ago

I plan to modify test_transitions_per_property.html to call a function in DOMWindowUtils which will sample the animations on the compositor at the specified time and makes sure that the interpolated nsCSSValueList is the same as the one computed on the main thread.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 2 • 12 years ago

But the main thread isn't going to be interpolating all async animations after your other patch, and anyway this would be a racy strategy, right?  Or maybe I misunderstand what you propose.

	David Zbarsky (:dzbarsky) Assignee
	Comment 3 • 12 years ago

Even with throttling animations, we still sample the animations.  We just don't flush style.  This shouldn't be racy because we can manually update the time on the main thread, send a sync messages to the compositor to update time, and have the compositor send back a sync message with the interpolated value.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 4 • 12 years ago

I thought we weren't going to sample animations at all in common cases, like elements within overflow:hidden (except when input events etc. force that).  Throttling isn't much of a solution to the problems we discussed.

It sounds pretty tricky to me to try to manually force both the refresh driver time and compositor time.  I'm afraid this will hit subtleties like clocks going backwards, or time being forced after animations complete.

What if we took the existing tests, but simply changed the way they checked the computed style to be this new magical compositor request?

We also need to deal with animations that we don't async-ify yet, so there may be value in having two separate test runs, and moving tests back and forth between the async/not-async buckets as we extend the platform support.

	David Zbarsky (:dzbarsky) Assignee
	Comment 5 • 12 years ago

Yes, that's what I mean by throttling.  We still sample in any case.  Sampling is not the slow part - flushing is.
We already manually set refresh driver times on the main thread for animation tests.  On the compositor, we pass in a time to sample animations, so we can just pass this time from the main thread, and this time will be set in the test script.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 6 • 12 years ago

Really need an owner here, especially as my recommendation is to leave async animation goodness disabled for fennec until we test it better.  b2g has been flying by the seat of its pants and dealing with fallout and regressions as they occur, because the feature is so critical for b2g.  Time to be mature engineers! ;)

	Jet Villegas (inactive)
	Comment 7 • 12 years ago

We've got the testing requirements from nrc, roc, and dbaron brainstorming. Pasting this here while I have another think at who to assign this to...

nrc>>>

We need to be able to query properties of layers from JS in tests and get the composited values. To do that we need to read a bunch of properties from layers on the compositor (opacity, transform and/or absolute position, others in the future). We then need to pass that back to the main thread and propagate it (probably via the layer manager) to the DOM and then expose it as special powers only stuff. I'm not sure exactly how to talk about layers from js/dom land, but I guess we could query on an element and return 0 if it did not get layerised. I think getting the values on the compositor thread and passing back to main thread is not too hard, either via the layer and then up to the manager, or up to the shadow manager/compositor and then back to main thread. I'm not sure how to get that stuff back to DOM, but I don't suppose it is too hard. I think this infrastructure would be useful for doing other tests for OMTC stuff.

Secondly, we need to be able to spoof the time on the compositor. We can do this for the refresh driver, but CompositorParent does it's own thing using Timestamp::Now, we would need to abstract that a bit and add in the ability to supply our own 'time'. I guess we would need a fair bit of wiring to pass that into layers and across IPC. Both things would need new API on layers and web-facing API, but the latter only for tests with special powers.

roc>>>

Then I guess we would need a single nsDOMWindowUtils method for that, which takes an element and a property name, finds the primary frame for the element, then calls FrameLayerBuilder::GetDedicatedLayer(frame, itemtype) where the itemtype is deduced from the property name. Then we can query some new API on the layer to fetch the data from the compositor.

	David Baron :dbaron:
	Comment 8 • 12 years ago

There's also one simpler piece, which is that we should have a way of forcing values to run through the value conversion code and interpolation code.  In other words, we should be able to test that the codepath that sends the values to the compositor (converting from nsStyleAnimation::Value to the ipdl structures in AddAnimationsForProperty and AddTransformFunctions in nsDisplayList.cpp, and then back in Layer::SetAnimations and CreateCSSValueList) and invokes nsStyleAnimation::Interpolate from SampleValue (in CompositorParent) produces the same results as the non-OMTA codepath that just invokes nsStyleAnimation::Interpolate directly on the results of nsStyleAnimation::ExtractComputedValue.
  
With that, we can have layout/style/test/test_transitions_per_property.html test (for properties that we animate on the compositor) both codepaths rather than just the latter.

	David Zbarsky (:dzbarsky) Assignee
	Comment 9 • 11 years ago

It's incorrect to update the style rule refresh time without updating the style rule.  Because of this, we can run into issues if we update the refresh time and then try to flush on the same tick and don't update the style rule.

	David Zbarsky (:dzbarsky) Assignee
	Comment 10 • 11 years ago

Pretty useless with infallible new

	David Baron :dbaron:
	Comment 12 • 11 years ago

Comment on attachment 753104 [details] [diff] [review]
Part 1: Fix animation throttling

This seems right, though also a bit scary.  Might want to check with :nrc as well.

And did you test this in a debug build and make sure you didn't hit any assertions?

	David Zbarsky (:dzbarsky) Assignee
	Comment 13 • 11 years ago

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #12)
> And did you test this in a debug build and make sure you didn't hit any
> assertions?

Yup, things look good.

	David Zbarsky (:dzbarsky) Assignee
	Comment 14 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/68cc545b9106
https://hg.mozilla.org/integration/mozilla-inbound/rev/be938f823e77

	Nick Cameron [:nrc]
	Comment 15 • 11 years ago

Comment on attachment 753104 [details] [diff] [review]
Part 1: Fix animation throttling

Review of attachment 753104 [details] [diff] [review]:
-----------------------------------------------------------------

This is the change we discussed the other day, right? If so r=me, otherwise I should think about it some more.

	David Zbarsky (:dzbarsky) Assignee
	Comment 16 • 11 years ago

(In reply to Nick Cameron [:nrc] from comment #15)
> Comment on attachment 753104 [details] [diff] [review]
> Part 1: Fix animation throttling
> 
> Review of attachment 753104 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is the change we discussed the other day, right?

Yes. https://hg.mozilla.org/integration/mozilla-inbound/rev/0f1c87602308

Also pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/9bbe372ab8e0 to make MatrixToCSSValue public.

	Phil Ringnalda (:philor)
	Comment 17 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/68cc545b9106
https://hg.mozilla.org/mozilla-central/rev/be938f823e77
https://hg.mozilla.org/mozilla-central/rev/9bbe372ab8e0
https://hg.mozilla.org/mozilla-central/rev/0f1c87602308

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 19 • 11 years ago

Comment on attachment 753700 [details] [diff] [review]
Part 4: Add the ability to pause and sample animations on compositors at a specified time

Review of attachment 753700 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/CompositorParent.h
@@ +162,5 @@
>     * the compositor thread.
>     */
>    static const LayerTreeState* GetIndirectShadowTree(uint64_t aId);
>  
> +  static void SetTimeAndSampleAnimations(TimeStamp aTime, bool aIsTesting);

Document this method and its parameters.

	David Zbarsky (:dzbarsky) Assignee
	Comment 20 • 11 years ago

Part 4: https://hg.mozilla.org/integration/mozilla-inbound/rev/6c1cf4694a13

	David Zbarsky (:dzbarsky) Assignee
	Comment 21 • 11 years ago

Backed out for not building on windows and android. https://hg.mozilla.org/integration/mozilla-inbound/rev/aae356806829

	David Zbarsky (:dzbarsky) Assignee
	Comment 23 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d8a969777c7

	David Baron :dbaron:
	Comment 24 • 11 years ago

Comment on attachment 754631 [details] [diff] [review]
Part 5: Add a function to get opacity and transform property values from layers

The GetAttributeValue thing on nsIDOMWindowUtils seems weird -- why not just add two methods?  That's basically what methods are for.  And I'm not crazy about using nsIDOMCSSValue -- we're really trying to avoid adding new uses of those interfaces.

You also need to rev the IID of nsIDOMWindowUtils.

Otherwise this seems fine to me, though I'm not sure what sort of feedback you were looking for.

	David Baron :dbaron:
	Comment 25 • 11 years ago

Although if it's specifically about getting CSS properties, maybe a GetOMTCCSSPropertyValue would make sense?  (I'd rather it return a string than nsIDOMCSSValue, if that makes sense.)

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 26 • 11 years ago

Comment on attachment 754631 [details] [diff] [review]
Part 5: Add a function to get opacity and transform property values from layers

Review of attachment 754631 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/interfaces/base/nsIDOMWindowUtils.idl
@@ +1398,5 @@
>     void runBeforeNextEvent(in nsIRunnable runnable);
> +
> +   /*
> +    * Returns the value of a given attribute.  If the attribute is animated off the
> +    * main thread, this function will fetch the correct value from the compositor.

Document the valid attribute names!

	David Zbarsky (:dzbarsky) Assignee
	Comment 27 • 11 years ago

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #24)
> Comment on attachment 754631 [details] [diff] [review]
> Part 5: Add a function to get opacity and transform property values from
> layers
> 
> The GetAttributeValue thing on nsIDOMWindowUtils seems weird -- why not just
> add two methods?  That's basically what methods are for.  

The idea is that test code can call this method without worrying about which properties are OMTA'ed, and it will always return the right thing because it falls back to getComputedStyle.

> And I'm not crazy
> about using nsIDOMCSSValue -- we're really trying to avoid adding new uses
> of those interfaces.

OK, I can just return the value's cssText property.

> Otherwise this seems fine to me, though I'm not sure what sort of feedback
> you were looking for.

I mostly figured you would be interested in taking a look.

	David Baron :dbaron:
	Comment 28 • 11 years ago

(In reply to David Zbarsky (:dzbarsky) from comment #27)
> (In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from
> comment #24)
> > Comment on attachment 754631 [details] [diff] [review]
> > Part 5: Add a function to get opacity and transform property values from
> > layers
> > 
> > The GetAttributeValue thing on nsIDOMWindowUtils seems weird -- why not just
> > add two methods?  That's basically what methods are for.  

OK, then I guess a single method is fine, but it needs a better name.  CSS properties are properties, not attributes.

How about calling the method nsIDOMWindowUtils.getOMTAOrComputedStyle() ?  That describes what it does much more clearly.

And just returning the .cssText sounds great.  (And don't forget to rev the IID.)

	David Baron :dbaron:
	Comment 29 • 11 years ago

And please don't use the term "attribute" for CSS properties in the comments, either.

	Ed Morley [:emorley]
	Comment 30 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/2d8a969777c7

	David Zbarsky (:dzbarsky) Assignee
	Comment 32 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a2289396667

	David Baron :dbaron:
	Comment 33 • 11 years ago

Comment on attachment 755685 [details] [diff] [review]
Part 6: Remove a redundant if

r=dbaron

(Though in general I'd like to see commit messages as part of the review request.  "remove a redundant if" is fine as a commit message here; using the summary of this bug is not.)

	Ryan VanderMeulen [:RyanVM]
	Comment 34 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/4a2289396667

	David Zbarsky (:dzbarsky) Assignee
	Comment 35 • 11 years ago

Turns out I wasn't testing the right thing.  This works better, modulo some rounding error in the 10^-6 spot for numbers that should be 0.  Not sure if there's much we can do about that.

	David Zbarsky (:dzbarsky) Assignee
	Comment 36 • 11 years ago

(In reply to David Zbarsky (:dzbarsky) from comment #35)
> Created attachment 756141 [details] [diff] [review]
> Part 7: Fix RecvGetTransform
> 
> Turns out I wasn't testing the right thing.  This works better, modulo some
> rounding error in the 10^-6 spot for numbers that should be 0.  Not sure if
> there's much we can do about that.

The other option is to save the untranslated transform on the layer, but I don't think we want to do that since this is just testing code.

	David Zbarsky (:dzbarsky) Assignee
	Comment 37 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/0bf84683f9e1
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e6f8502d1af

	David Baron :dbaron:
	Comment 41 • 11 years ago

Comment on attachment 756342 [details] [diff] [review]
Part 9: Fix matrix() functions

Yeah, the old code here is clearly wrong.

But if we assume the matrix3d code is right (i.e., assume that we don't need to flip row-major and column-major here), then I think this is also wrong.

I think what you should end up doing is assigning:
  Item(1) -> matrix._11 (as you do)
  Item(2) -> matrix._12 (as you do)
  Item(3) -> matrix._21 (as you do)
  Item(4) -> matrix._22 (as you do)
  Item(5) -> matrix._41
  Item(6) -> matrix._42

I tested that the behavior of certain components of matrix() and matrix3d() matches each other by fiddling with this testcase in Firefox:
http://software.hixie.ch/utilities/js/live-dom-viewer/?%3C!DOCTYPE%20html%3E%0A%3Cstyle%3E%0Abody%3Ediv%20%7B%20border%3A%20thin%20solid%20blue%3B%20width%3A%20100px%3B%20margin%3A%205px%200%3B%20%7D%0Abody%3Ediv%3Ediv%20%7B%20border%3A%20thin%20solid%20green%3B%20height%3A%2020px%3B%20transform-origin%3A%200%200%3B%20%7D%0Abody%3Ediv%3Anth-child(1)%3Ediv%20%7B%20transform%3A%20matrix(1%2C%200%2C%200.5%2C%201%2C%2020%2C%203)%3B%20%7D%0Abody%3Ediv%3Anth-child(2)%3Ediv%20%7B%20transform%3A%20matrix3d(1%2C%200%2C%200%2C%200%2C%200.5%2C%201%2C%200%2C%200%2C%200%2C%200%2C%201%2C%200%2C%2020%2C%203%2C%200%2C%201)%20%7D%0A%3C%2Fstyle%3E%0A%3Cdiv%3E%3Cdiv%3E%3C%2Fdiv%3E%3C%2Fdiv%3E%0A%3Cdiv%3E%3Cdiv%3E%3C%2Fdiv%3E%3C%2Fdiv%3E
and this one in Chrome:
http://software.hixie.ch/utilities/js/live-dom-viewer/?%3C!DOCTYPE%20html%3E%0A%3Cstyle%3E%0Abody%3Ediv%20%7B%20border%3A%20thin%20solid%20blue%3B%20width%3A%20100px%3B%20margin%3A%205px%200%3B%20%7D%0Abody%3Ediv%3Ediv%20%7B%20border%3A%20thin%20solid%20green%3B%20height%3A%2020px%3B%20-webkit-transform-origin%3A%200%200%3B%20%7D%0Abody%3Ediv%3Anth-child(1)%3Ediv%20%7B%20-webkit-transform%3A%20matrix(1%2C%200%2C%200%2C%201%2C%2020%2C%203)%3B%20%7D%0Abody%3Ediv%3Anth-child(2)%3Ediv%20%7B%20-webkit-transform%3A%20matrix3d(1%2C%200%2C%200%2C%200%2C%200%2C%201%2C%200%2C%200%2C%200%2C%200%2C%201%2C%200%2C%2020%2C%203%2C%200%2C%201)%20%7D%0A%3C%2Fstyle%3E%0A%3Cdiv%3E%3Cdiv%3E%3C%2Fdiv%3E%3C%2Fdiv%3E%0A%3Cdiv%3E%3Cdiv%3E%3C%2Fdiv%3E%3C%2Fdiv%3E


So, not sure what tests you're currently debugging... but they're not good enough.  Could you add a test that catches the translation components of matrix() being wrong too?

r=dbaron if you (a) agree that what I'm suggesting is correct and (b) fix the code to do it and (c) have a test that catches it, though.

	David Baron :dbaron:
	Comment 42 • 11 years ago

(And matrix._33 and matrix._44 should be 1, as they are now, and all the others not mentioned in the previous comment or this one should be 0.)

	David Zbarsky (:dzbarsky) Assignee
	Comment 43 • 11 years ago

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #41)
> Comment on attachment 756342 [details] [diff] [review]
> Part 9: Fix matrix() functions
> 
> Yeah, the old code here is clearly wrong.
> 
> But if we assume the matrix3d code is right (i.e., assume that we don't need
> to flip row-major and column-major here), then I think this is also wrong.
> 
> I think what you should end up doing is assigning:
>   Item(1) -> matrix._11 (as you do)
>   Item(2) -> matrix._12 (as you do)
>   Item(3) -> matrix._21 (as you do)
>   Item(4) -> matrix._22 (as you do)
>   Item(5) -> matrix._41
>   Item(6) -> matrix._42

I don't understand why the matrix is flipped here, but assuming that's the case, this makes sense.  And it fixes the rest of the test failures.

	David Zbarsky (:dzbarsky) Assignee
	Comment 44 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b31ac7a1bb4
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0e5cb861643
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9e151e7eb99

	Ryan VanderMeulen [:RyanVM]
	Comment 45 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/0bf84683f9e1
https://hg.mozilla.org/mozilla-central/rev/3e6f8502d1af

	Phil Ringnalda (:philor)
	Comment 49 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/1b31ac7a1bb4
https://hg.mozilla.org/mozilla-central/rev/c0e5cb861643
https://hg.mozilla.org/mozilla-central/rev/d9e151e7eb99

	David Zbarsky (:dzbarsky) Assignee
	Comment 50 • 11 years ago

Lost this hunk earlier: https://hg.mozilla.org/integration/mozilla-inbound/rev/5103d37775ce

	David Zbarsky (:dzbarsky) Assignee
	Comment 51 • 11 years ago

Comment on attachment 756770 [details] [diff] [review]
Part 11: Refactor test_transitions_per_property

Ignore this, it's failing intermittently on try.

	David Baron :dbaron:
	Comment 52 • 11 years ago

Is part 11 in preparation for tests that aren't attached yet (rather than the tests in part 12)?

Also, preparation is misspelled in the commit message in part 11.  And is Reuben the correct patch author of part 12?  (I don't see him involved in this bug at all.)

So should I wait to review part 12 as well given comment 51?  (I was just getting to the review requests here...)

	David Zbarsky (:dzbarsky) Assignee
	Comment 53 • 11 years ago

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #52)
> Is part 11 in preparation for tests that aren't attached yet (rather than
> the tests in part 12)?
> 

Part 11 prepares for part 12.

> Also, preparation is misspelled in the commit message in part 11.  And is
> Reuben the correct patch author of part 12?  (I don't see him involved in
> this bug at all.)

No, he was testing something for me and I guess the commit message got changed when he imported the patch.

> So should I wait to review part 12 as well given comment 51?  (I was just
> getting to the review requests here...)

I obsoleted the wrong patch.  11 should be fine, 12 needs more work.

	David Baron :dbaron:
	Comment 54 • 11 years ago

Comment on attachment 756770 [details] [diff] [review]
Part 11: Refactor test_transitions_per_property

>+SimpleTest.waitForExplicitFinish();
>+

>+  SimpleTest.finish();
>+

I don't see any reason for these two additions; the entire test still runs synchronously.


>+var transformTestIndex = 0;

Don't add this unused variable.


r=dbaron on the rest

	David Zbarsky (:dzbarsky) Assignee
	Comment 55 • 11 years ago

My test was failing intermittently on try because if we push a shadow layers update with a throttled animation, the compositor will think the animation is at the first frame until there is another Composite.  If we query the layer's transform during that time, we get an identity transform.

	Ryan VanderMeulen [:RyanVM]
	Comment 56 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/5103d37775ce

	David Baron :dbaron:
	Comment 58 • 11 years ago

Comment on attachment 757587 [details] [diff] [review]
Part 12: Add tests for async transforms

>+  var test = transformTests[transformTestIndex];
>+
>+  if (!test) {
>+    transformDiv.style.removeProperty("transition");
>+    SimpleTest.finish();
>+    return;
>+  }

Probably cleaner (is there a strict warning?) to do:

+  if (transformTestIndex >= transformTests.length) {
+    transformDiv.style.removeProperty("transition");
+    SimpleTest.finish();
+    return;
+  }
+
+  var test = transformTests[transformTestIndex];



So the combination of:

 * the fact that you don't set the transition-timing-function to linear
   anywhere (which means you get 'ease', the default)

 * the fact that you're looking at 1/3 of the way through the transition
   (100s of 300s) rather than 1/4 of the way through like the tests are
   meant to be used

is a little confusing; it means the OMTA tests are testing a point
57.59% of the way through the animation whereas the other tests are
testing a point 25% of the way through.  While I suppose there's value
testing something different, some comments to that effect would at
least be useful, and it might be worth testing something a little further
from 50%.



In nextAsyncTransformTest:

>+  var computed_transform = transformCs.getPropertyValue("transform");

You don't need the "var computed_transform = " here.  (You do in
the later function, of course.)



Are you planning to investigate big_omta_round_error as a followup?

r=dbaron

	David Zbarsky (:dzbarsky) Assignee
	Comment 59 • 11 years ago

(In reply to David Baron [:dbaron] (in W3C meetings through June 11) (don't cc:, use needinfo? instead) from comment #58)
> Comment on attachment 757587 [details] [diff] [review]
> Part 12: Add tests for async transforms
> 

> 
> is a little confusing; it means the OMTA tests are testing a point
> 57.59% of the way through the animation whereas the other tests are
> testing a point 25% of the way through.  While I suppose there's value
> testing something different, some comments to that effect would at
> least be useful, and it might be worth testing something a little further
> from 50%.
> 

I changed it to linear 200s and commented.

> 
> Are you planning to investigate big_omta_round_error as a followup?
> 

Yes, but as far as I can tell we're just getting errors from lots of floating point math.  I have an idea that may work, but it's better as a followup.

	David Zbarsky (:dzbarsky) Assignee
	Comment 60 • 11 years ago

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1b8b4e6a986a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/411b2e15a286
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e7cf4aeef44d

Still need to write opacity tests.

	Ed Morley [:emorley]
	Comment 61 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/1b8b4e6a986a
https://hg.mozilla.org/mozilla-central/rev/411b2e15a286
https://hg.mozilla.org/mozilla-central/rev/e7cf4aeef44d

	David Zbarsky (:dzbarsky) Assignee
	Comment 62 • 11 years ago

Might be worth renaming the transformDiv.

	David Baron :dbaron:
	Comment 64 • 11 years ago

Comment on attachment 765665 [details] [diff] [review]
Part 14: Add test for opacity

>+  is(async_opacity, computed_opacity,
>+     "Async animated opacity does not much computed opacity");

The test message should make sense when both passing or failing, so you should s/does not much/should match/.

And renaming transformDiv, transformCs, to something like OMTADiv, OMTACs, might make sense as well.

r=dbaron with that

	David Zbarsky (:dzbarsky) Assignee
	Comment 65 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c3acc2030b0

	Ed Morley [:emorley]
	Comment 66 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/6c3acc2030b0

	Ed Morley [:emorley]
	Comment 67 • 11 years ago

This landed on mozilla-central whilst it was still mozilla24, however mcMerge references the bugzilla target milestone field ordering, which was updated pre-emptively before the merge had occurred.

	David Zbarsky (:dzbarsky) Assignee
	Comment 69 • 11 years ago

Comment on attachment 820413 [details] [diff] [review]
Part 15: Add tests for async animations

Review of attachment 820413 [details] [diff] [review]:
-----------------------------------------------------------------

You don't need the -moz prefix.

::: layout/style/test/test_animations.html
@@ +247,5 @@
> +    return undefined;
> +  }
> +  return undefined;
> +}
> +

You can just call getOMTAOrComputedStyle here.  It automatically falls back to cs.getPropertyValue if omta is off.

Also, I'm surprised all these tests pass.  Are you actually getting OMTA?  You can try changing http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/AsyncCompositionManager.cpp#400 to pass in the wrong values and see if this starts failing.

	Jet Villegas (inactive)
	Comment 70 • 11 years ago

These tests are in rough shape but I finally got OMTA code paths to get triggered. Unfortunately, it's turned flakey and crashy. Over to dzbarsky for a look at why and how to modify the tests/code as needed.

	David Zbarsky (:dzbarsky) Assignee
	Comment 71 • 11 years ago

Jet, what did you do to trigger the crashes?  I added test_OMTA.html as a mochitest and switched on the async animations pref, but the test runs fine.

	Jet Villegas (inactive)
	Comment 72 • 11 years ago

Just enabling layers.offmainthreadcomposition.async-animations to true got asserts and a crash. I'll see if I can get a stack up.

	Brian Birtles (:birtles)
	Comment 73 • 11 years ago

I've split off a separate bug (bug 964646) to track testing CSS Animations and making this bug specific to CSS Transitions.