1272204 - Rewrite tests in test_animation_performance_warning.html to use setFrames (to be setKeyframes)

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Brian Birtles (:birtles, Feb 26~ parental leave)]

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.)

Comment 1

[Ryo Kato [:r_kato]]

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

Comment 2

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Thank you!

Comment 3

[Hiroyuki Ikezoe (:hiro)]

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

Comment 4

[Ryo Kato [:r_kato]]

Attachment: 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.

Review commit: https://reviewboard.mozilla.org/r/53088/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53088/

Comment 5

[Hiroyuki Ikezoe (:hiro)]

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?

Comment 6

[Ryo Kato [:r_kato]]

(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 7

[Ryo Kato [:r_kato]]

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 8

[Hiroyuki Ikezoe (:hiro)]

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.

Comment 9

[Ryo Kato [:r_kato]]

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 10

[Ryo Kato [:r_kato]]

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/

Comment 11

[Ryo Kato [:r_kato]]

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

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/64c305e296c8

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/64c305e296c8