1310605 - Split some test cases that can be rewritten to synchronous test in test_animation_observers.html

Status: RESOLVED FIXED

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

Description

[Hiroyuki Ikezoe (:hiro)]

+++ This bug was initially created as a clone of Bug #1283754 +++

Unfortunately MozReview can handle additional patches once some patches has been landed. This is a bug for what I commented in bug 1283754 comment 36.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1310605 - Part 1: Rname test_observers_for_script_animation.html to test_observers_for_sync_api.html.

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

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1310605 - Part 2: Move test cases for CSS animation that can be rewriten to synchronus test. .

All of cleaning up code were dropped. Also some trivial comments were dropped.

Note that test cases can not be rewritten to synchronous test.
* Waiting for events, e.g. transitionend.
 * single_transition
 * single_animation

* Manipulated by style, e.g. style.animationDuraton = "0.5s".
 * single_transition_cancelled_property
 * single_transition_cancelled_value
 * single_transition_cancelled_noninterpolable
 * single_transition_reversed
 * multiple_transitions
 * single_animation_cancelled_name
 * single_animation_cancelled_duration
 * single_animation_cancelled_delay
 * single_animation_cancelled_fill
 * single_animation_cancelled_iteration_count
 * single_animation_change_XXX
 * coalesce_change_cancel

* Falling into a pending state, e.g. play() or pause().
 * finish_from_pause
 * finish_from_pause_pending
 * play
 * play_filling_forwards
 * play_after_finish
 * pause
 * pause_while_pause_pending
 * aborted_pause

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

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1310605 - Part 3: Move test case for pseudo element that can be rewritten to synchronous test.

tree_ordering test can not be moved because the test waits for animationend
event.

createStyle() and createPseudo() were picked from
testing/web-platform/tests/web-animations/testcommon.js .

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

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1310605 - Part 4: Drop gRecordPromiseResolvers.

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

Comment 5

[Hiroyuki Ikezoe (:hiro)]

These patches reduces elapsed time of test_animation_observers.html roughly 30%.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #0)
> +++ This bug was initially created as a clone of Bug #1283754 +++
> 
> Unfortunately MozReview can handle additional patches once some patches has

'can not handle'

Comment 7

[Boris Chiou [:boris]]

Comment on attachment 8801648 [details]
Bug 1310605 - Part 1: Rname test_observers_for_script_animation.html to test_observers_for_sync_api.html.

https://reviewboard.mozilla.org/r/86318/#review85392

Comment 8

[Boris Chiou [:boris]]

Comment on attachment 8801649 [details]
Bug 1310605 - Part 2: Move test cases for CSS animation that can be rewriten to synchronus test. .

https://reviewboard.mozilla.org/r/86320/#review85432

The order of these tests is a little different from the original one, but I think it is still ok to me.
Thanks for rewriting these tests.

::: dom/animation/test/chrome/test_observers_for_sync_api.html:17
(Diff revision 1)
> +div {
> +  width: 100px;
> +  height: 100px;
> +  background-color: yellow;
> +}

Is this necessary or for debugging?

Comment 9

[Boris Chiou [:boris]]

Comment on attachment 8801650 [details]
Bug 1310605 - Part 3: Move test case for pseudo element that can be rewritten to synchronous test.

https://reviewboard.mozilla.org/r/86322/#review85438

r=me with the following comments addressed.

::: dom/animation/test/chrome/test_observers_for_sync_api.html:63
(Diff revision 1)
> +function createStyle(test, rules, doc) {
> +  if (!doc) {
> +    doc = document;
> +  }
> +  var extraStyle = doc.createElement('style');
> +  doc.head.appendChild(extraStyle);
> +  if (rules) {
> +    var sheet = extraStyle.sheet;
> +    for (var selector in rules) {
> +      sheet.insertRule(selector + '{' + rules[selector] + '}',
> +                       sheet.cssRules.length);
> +    }
> +  }
> +  test.add_cleanup(function() {
> +    extraStyle.remove();
> +  });
> +}

I guess this function is from wpt testcommon.js. However, we have an equivalent function in dom/animation/tests/testcommon.js, addStyle() [1]. Could we reuse it?

[1] http://searchfox.org/mozilla-central/rev/d96317a351af8aa78ab9847e7feed964bbaac7d7/dom/animation/test/testcommon.js#87-113

::: dom/animation/test/chrome/test_observers_for_sync_api.html:83
(Diff revision 1)
> +  createStyle(test, { '@keyframes anim': '',
> +                      ['.pseudo::' + type]: 'animation: anim 10s;' });

If we reuse addStyle, this might be:

addStyle(test, {...});

I think the format of |rules| is the same.

::: dom/animation/test/chrome/test_observers_for_sync_api.html:864
(Diff revision 1)
> +    "records after duration set \"auto\"");
> +
> +  anim.effect.timing.duration = "auto";
> +  assert_equals_records(observer.takeRecords(),
> +    [], "records after assigning same value \"auto\"");
> +}, "change_duration_and_currenttime");

I think it's better to rename this because we already have a test whose name is "change_duration_and_currenttime".

How about: "change_duration_and_currenttime_on_pseudo_elements"?

Comment 10

[Boris Chiou [:boris]]

Comment on attachment 8801651 [details]
Bug 1310605 - Part 4: Drop gRecordPromiseResolvers.

https://reviewboard.mozilla.org/r/86324/#review85446

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8801648 [details]
Bug 1310605 - Part 1: Rname test_observers_for_script_animation.html to test_observers_for_sync_api.html.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86318/diff/1-2/

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8801649 [details]
Bug 1310605 - Part 2: Move test cases for CSS animation that can be rewriten to synchronus test. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86320/diff/1-2/

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8801650 [details]
Bug 1310605 - Part 3: Move test case for pseudo element that can be rewritten to synchronous test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86322/diff/1-2/

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8801651 [details]
Bug 1310605 - Part 4: Drop gRecordPromiseResolvers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86324/diff/1-2/

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8801650 [details]
Bug 1310605 - Part 3: Move test case for pseudo element that can be rewritten to synchronous test.

https://reviewboard.mozilla.org/r/86322/#review85438

> I guess this function is from wpt testcommon.js. However, we have an equivalent function in dom/animation/tests/testcommon.js, addStyle() [1]. Could we reuse it?
> 
> [1] http://searchfox.org/mozilla-central/rev/d96317a351af8aa78ab9847e7feed964bbaac7d7/dom/animation/test/testcommon.js#87-113

Thanks! Fixed it!

> I think it's better to rename this because we already have a test whose name is "change_duration_and_currenttime".
> 
> How about: "change_duration_and_currenttime_on_pseudo_elements"?

Good catch! I did not notice it at all.

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8801649 [details]
Bug 1310605 - Part 2: Move test cases for CSS animation that can be rewriten to synchronus test. .

https://reviewboard.mozilla.org/r/86320/#review85432

> Is this necessary or for debugging?

Not at all!  I just copied it. Thanks!

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/504ca901dd74
Part 1: Rname test_observers_for_script_animation.html to test_observers_for_sync_api.html. r=boris
https://hg.mozilla.org/integration/autoland/rev/2b3a5735d669
Part 2: Move test cases for CSS animation that can be rewriten to synchronus test. r=boris.
https://hg.mozilla.org/integration/autoland/rev/2204c326ef7d
Part 3: Move test case for pseudo element that can be rewritten to synchronous test. r=boris
https://hg.mozilla.org/integration/autoland/rev/600b012bb435
Part 4: Drop gRecordPromiseResolvers. r=boris

Comment 18

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/504ca901dd74
https://hg.mozilla.org/mozilla-central/rev/2b3a5735d669
https://hg.mozilla.org/mozilla-central/rev/2204c326ef7d
https://hg.mozilla.org/mozilla-central/rev/600b012bb435

Comment 19

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=mozilla-aurora&revision=94597102b4d9aa35a15a8fb52553a6c0b1409b8f