Closed Bug 1298571 Opened 8 years ago Closed 8 years ago

Bug 1293806 need test cases

Categories

(Core :: DOM: Animation, defect, P3)

Product:
Core
Component:
DOM: Animation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: skoji, Assigned: skoji)

Details

Attachments

(1 file, 1 obsolete file)

Bug 1298571 - Add test cases for bug 1293806
58 bytes, text/x-review-board-request
hiro
: review+
Details 
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_6) AppleWebKit/601.7.7 (KHTML, like Gecko) Version/9.1.2 Safari/601.7.7

Steps to reproduce:

Bug 1293806 needs some test cases.
Assignee: nobody → skoji
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Summary: Write test cases for Bug 1293806 → Bug 1293806 need test cases
Comment on attachment 8785688 [details]
Bug 1298571 - Add test cases for bug 1293806

https://reviewboard.mozilla.org/r/74794/#review72636

This is really good.
r=me with the following changes.

::: dom/animation/test/chrome/test_restyles.html:697
(Diff revision 1)
>         'to remove the previous animated style');
>  
>      yield ensureElementRemoval(div);
>    });
>  
> +  add_task(function* unnecessary_update_is_not_invoked() {

Nit:

function* no_restyling_when_animation_style_when_re_setting_same_animation_property()

Oops!  I just notice that we should use add_task_if_omta_enabled() instead of add_task() because the opacity animation runs on the compositor.

::: dom/animation/test/chrome/test_restyles.html:700
(Diff revision 1)
> +    yield animation.ready;
> +    yield waitForAnimationFrames(5);

Let's do the same thing just like other opacity animation test case does;

http://hg.mozilla.org/mozilla-central/file/1a5b53a831e5/dom/animation/test/chrome/test_restyles.html#l102

yield animation.ready;
ok(animation.isRunningOnCompositor);

::: dom/animation/test/chrome/test_restyles.html:702
(Diff revision 1)
> +  add_task(function* unnecessary_update_is_not_invoked() {
> +    var div = addDiv(null, { style: 'animation: opacity 10s' });
> +    var animation = div.getAnimations()[0];
> +    yield animation.ready;
> +    yield waitForAnimationFrames(5);
> +    div.style.animation = 'opacity 10s';

Nit: Please leave a comment just before re-setting the same property like this;

 // Apply the same animation style

::: dom/animation/test/chrome/test_restyles.html:705
(Diff revision 1)
> +    yield animation.ready;
> +    yield waitForAnimationFrames(5);
> +    div.style.animation = 'opacity 10s';
> +    var markers = yield observeStyling(5);
> +    is(markers.length, 0,
> +       'Bug 1293806: Applying same animation style '  +

Nit: Please remove this bug number here.

::: dom/animation/test/chrome/test_restyles.html:715
(Diff revision 1)
> +  add_task(function* necessary_update_should_be_invoked() {
> +    var div = addDiv(null, { style: 'animation: background-color 10s' });
> +    var animation = div.getAnimations()[0];
> +    yield animation.ready;
> +    yield waitForAnimationFrames(5);
> +    div.style.animation = 'background-color 8s';

Nit: Please leave a comment here too.

::: dom/animation/test/chrome/test_restyles.html:717
(Diff revision 1)
> +    var animation = div.getAnimations()[0];
> +    yield animation.ready;
> +    yield waitForAnimationFrames(5);
> +    div.style.animation = 'background-color 8s';
> +    var animation = div.getAnimations()[0];
> +    yield animation.ready;

Nit: We don't need to wait for animation.ready here.

::: dom/animation/test/chrome/test_restyles.html:720
(Diff revision 1)
> +    div.style.animation = 'background-color 8s';
> +    var animation = div.getAnimations()[0];
> +    yield animation.ready;
> +    var markers = yield observeStyling(5);
> +    is(markers.length, 5,
> +       'Bug 1293806: Applying animation style with different duration '  +

Nit: Please remove the bug number.

::: dom/animation/test/chrome/test_restyles.html:721
(Diff revision 1)
> +    var animation = div.getAnimations()[0];
> +    yield animation.ready;
> +    var markers = yield observeStyling(5);
> +    is(markers.length, 5,
> +       'Bug 1293806: Applying animation style with different duration '  +
> +       'should cause restyles');

Nit: should cause restyles on every frame.
Attachment #8785688 - Flags: review?(hiikezoe) → review+
Comment on attachment 8785688 [details]
Bug 1298571 - Add test cases for bug 1293806

https://reviewboard.mozilla.org/r/74794/#review72640

::: dom/animation/test/chrome/test_restyles.html:698
(Diff revision 1)
>  
>      yield ensureElementRemoval(div);
>    });
>  
> +  add_task(function* unnecessary_update_is_not_invoked() {
> +    var div = addDiv(null, { style: 'animation: opacity 10s' });

Oops! I fotgot to mention about animation duration. 10s causes sometimes a timeout on slow mathine,  so we should use 100s.
Comment on attachment 8785855 [details]
Bug 1298571 - Fix for review comments

https://reviewboard.mozilla.org/r/74906/#review72738

Thanks for the fix!

Could you please fold these two patches into a changeset.  You can do it by histedit with fold.
Attachment #8785855 - Flags: review?(hiikezoe) → review+
Attachment #8785855 - Attachment is obsolete: true
Comment on attachment 8785688 [details]
Bug 1298571 - Add test cases for bug 1293806

https://reviewboard.mozilla.org/r/74794/#review72944

::: dom/animation/test/chrome/test_restyles.html:712
(Diff revision 2)
> +       'should never cause restyles');
> +    yield ensureElementRemoval(div);
> +  });
> +
> +  add_task(function* necessary_update_should_be_invoked() {
> +    var div = addDiv(null, { style: 'animation: background-color 10s' });

I am sorry that I missed '10s' here.  Please change it to 100s too.
https://hg.mozilla.org/mozilla-central/rev/02a345df379c
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: