In test_animation_performance_warning.html there are two comments saying we should rewrite tests once setFrames() is implemented.[1] [1] e.g. https://dxr.mozilla.org/mozilla-central/rev/3461f3cae78495f100a0f7d3d2e0b89292d3ec02/dom/animation/test/chrome/test_animation_performance_warning.html#176 setFrames() was implemented in bug 1244591 so we should rewrite these tests accordingly. (Note that setFrames is about to be renamed to setKeyframes in bug 1271904.)

I would like to take charge of this, so could you assign this bug to me...?

Thank you!

He is another 'Ryo'! Anyway thanks! I just notice this while reviewing a patch for bug 1271904.

We need individual expected values for both 'base' keyframes and 'wrap' ones, because setKeyframes() is used to update keyframes and, as a result, the length of anim.effect.getProperties() is changed. Review commit: https://reviewboard.mozilla.org/r/53088/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/53088/

Thanks! (In reply to Ryo Kato [:r_kato] from comment #4) > Created attachment 8753217 [details] > MozReview Request: Bug 1272204 - Rewrite tests in > test_animation_performance_warning.html to use setKeyframes r?hiro > > We need individual expected values for both 'base' keyframes and 'wrap' > ones, because setKeyframes() is used to update keyframes and, as a result, > the length of anim.effect.getProperties() is changed. I think we can drop the *two test cases* (I guess you missed opacity and transform animation case there) from gAnimationsTests. then write two new test cases something like below; 1. create a transform animation (or animation has transform and opacity property) 2. check there is no performance warnings 3. add 'width' keyframe 4. check the performance warning Note that the performance warning is only added to transform property. 5. remove 'width' keyframe 6. check there is no warnings. Can't we? Also we can't write tests without 'base' and 'wrap' in case of gMultipleAsyncAnimationsWithGeometricKeyframeTests somehow?

(In reply to Hiroyuki Ikezoe (:hiro) from comment #5) Thank you for reviewing! > I think we can drop the *two test cases* (I guess you missed opacity and > transform animation case there) from gAnimationsTests. then write two new > test cases something like below; OK, I will do that. > Also we can't write tests without 'base' and 'wrap' in case of > gMultipleAsyncAnimationsWithGeometricKeyframeTests somehow? I think so too. Now I have a problem that we need to meet the two lengths of arguments passed into assert_*** function. I will try to work out a better way.

Comment on attachment 8753217 [details] MozReview Request: Bug 1272204 - Rewrite tests in test_animation_performance_warning.html to use setKeyframes r=hiro Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53088/diff/1-2/

Comment on attachment 8753217 [details] MozReview Request: Bug 1272204 - Rewrite tests in test_animation_performance_warning.html to use setKeyframes r=hiro https://reviewboard.mozilla.org/r/53088/#review49960 Great! Once you are ready to go, I will push to try server. ::: dom/animation/test/chrome/test_animation_performance_warning.html:179 (Diff revision 2) > }, > +]; > + > +// Test cases that check results of adding/removing a 'width' property on the > +// same animation object. > +var gThreadSwitchingOnKeyframesTests = [ I think gAnimationWithGeometricKeyframeTests is suitable for the tests. 'ThreadSwitching' is somewhat misleading. It looks to me that the animation causes 'thread context switch'. ::: dom/animation/test/chrome/test_animation_performance_warning.html:212 (Diff revision 2) > + plain: [ > + { > + property: 'opacity', > + runningOnCompositor: true > + }, > + { > + property: 'transform', > + runningOnCompositor: true > + }, > + ], > + geometric: [ I'd prefer 'withoutGeometric' and 'withGeometric'. Instead, I thought we can use Array.map() to generate the expected array for 'with geometric property' from one expected array in test code, but I am OK with the current code. ::: dom/animation/test/chrome/test_animation_performance_warning.html:235 (Diff revision 2) > - }, > + }, > - { > + { > - property: 'transform', > + property: 'transform', > - runningOnCompositor: false, > + runningOnCompositor: false, > - warning: 'AnimationWarningTransformWithGeometricProperties' > + warning: 'AnimationWarningTransformWithGeometricProperties' > - } > + }, nit: an extra comma. ::: dom/animation/test/chrome/test_animation_performance_warning.html:598 (Diff revision 2) > }); > }, subtest.desc); > }); > > + gThreadSwitchingOnKeyframesTests.forEach(function(subtest) { > + promise_test(function(t) { I can see 4 spaces here. Right? If so, we should use 2 spaces. ::: dom/animation/test/chrome/test_animation_performance_warning.html:604 (Diff revision 2) > + var animation = addDivAndAnimate(t, > + { class: 'compositable' }, > + subtest.frames, 100 * MS_PER_SEC); > + > + return animation.ready.then(function() { > + // First, transform animations are running on compositor. nit: I think we have one transform animation here. ::: dom/animation/test/chrome/test_animation_performance_warning.html:608 (Diff revision 2) > + return animation.ready.then(function() { > + // First, transform animations are running on compositor. > + assert_animation_property_state_equals( > + animation.effect.getProperties(), > + subtest.expected.plain); > + nit: an unnecessary blank line here. There are several blank lines other places. ::: dom/animation/test/chrome/test_animation_performance_warning.html:627 (Diff revision 2) > + // Remove the 'width' property. > + animation.effect.setKeyframes(subtest.frames); I am curious why you don't use delete here as you do in another test. ::: dom/animation/test/chrome/test_animation_performance_warning.html:638 (Diff revision 2) > + assert_animation_property_state_equals( > + animation.effect.getProperties(), > + subtest.expected.plain); > + > + }); > + }, 'Thread switching: ' + subtest.desc); I think this should say that 'An animation has: '. ::: dom/animation/test/chrome/test_animation_performance_warning.html:709 (Diff revision 2) > var div = addDiv(t, { class: 'compositable' }); > var animations = subtest.animations.map(function(anim) { > var animation = div.animate(anim.frames, 100 * MS_PER_SEC); > > // Bind expected values to animation object. > - animation.expected = anim.expected; > + animation.expected = anim.expected nit: a semicolon is accidentaly dropped.

https://reviewboard.mozilla.org/r/53088/#review49960 > I can see 4 spaces here. Right? If so, we should use 2 spaces. There seems be 2 space indentations for me... Is it before `promise_test`? > I am curious why you don't use delete here as you do in another test. That was just because I thought it might be undesirable to use `delete` operator. I will fix that for consistency.

Comment on attachment 8753217 [details] MozReview Request: Bug 1272204 - Rewrite tests in test_animation_performance_warning.html to use setKeyframes r=hiro Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53088/diff/2-3/

I am ready to have this patch pushed to try server. Thank you for reviewing!