1425771 - Tweak test_restyles.html for the conformant Promise handling

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

This is the final piece to make animation tests pass with the conformant Promise handling (other than test_event-dispatch.html (bug 1423066)).

Remaining parts are;

1) Tweak for the test that checks a transform animation in scrolled out element is unthrottled every 200ms
2) Tweak expected restyle count for the case where the animation begins at the current time 
3) Tweak the redundant restyle for Animation.finish() in a micro task for Animation.ready (bug 1415457)

Comment 1

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ae4e755d273e55c1ba4ba2de717723a49d3a9bf

I haven't finished writing commit messages though.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

The try result in comment 1 looks good.

Here is a try with the conformant Promise handling.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f0ab2b11f7ba6badc96bc8475319319227a63d3a

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1425771 - Add a function to check detect whether have conformant Promise handling and set the flag to represent it.

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

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1425771 - Add a another variant of restyling_transform_animations_in_scrolled_out_element for the conformant Promise handling.

The test case checks document timeline's currentTime to make sure that we are
in the first frame which happened after 200ms since the animation started.  It
is unfortunate for us from the point of view of restyling that the first frame
includes the restyling process in the frame without the conformant Promise
handling, doesn't include the process with the Promise handling.  So we need
to tweak there depends on the Promise handling.

I did intentionally add the forked version of the function with the same name
since branching depending on the conformant flag inside the original function
will be hard to understand and also we can easily remove the original version
once we have the conformant Promise handling.

FWIW, here is the diff between them:

@@ -365,6 +365,9 @@ waitForAllPaints(() => {

       await SpecialPowers.pushPrefEnv({ set: [["ui.showHideScrollbars", 1]] });

+      // Make sure we start from the state right after requestAnimationFrame.
+      await waitForFrame();
+
       var parentElement = addDiv(null,
         { style: 'overflow-y: scroll; height: 20px;' });
       var div = addDiv(null,
@@ -379,13 +382,17 @@ waitForAllPaints(() => {
       var markers;
       var now;
       while (true) {
-        markers = await observeStyling(1);
-        // Check restyle markers until 200ms is elapsed.
         now = document.timeline.currentTime;
         if ((now - timeAtStart) >= 200) {
+          // If the current time has elapsed over 200ms since the animation was
+          // created, it means that the animation should have already
+          // unthrottled in this tick, let's see what observes in this tick's
+          // restyling process.
+          markers = await observeStyling(1);
           break;
         }

+        markers = await observeStyling(1);
         is(markers.length, 0,
            'Transform animation running on the element which is scrolled out ' +
            'should be throttled until 200ms is elapsed');

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

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1425771 - Tweak expected restyle count for the case where the animation begins at the current time.

The expected restyle count depends both on animation state and micro task
handling.

If we have the conformant Promise handling and if the animation begins at the
current time, we will observe 1 less restyles than observed frames since we
skip the first restyle in the initial frame.  This represents correctly what
we do and what we *should* do.

If we don't have the conformant Promise handling and if the animation doesn't
begin at the current time, we will observe 1 more restyles than obsered frames
since the Promise in observeStyling (precisely the Promise is inside the
callback for requestAnimationFrame) is fulfilled once after a restyling process
followed by the requestAnimationFrame.

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

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1425771 - Add a branch in only_one_restyling_after_finish_is_called for the conformant Promise handling.

Animation.finish() in a micro task for Animation.ready leads to a restyle and
a redundant restyle due to bug 1415457.  The redundant restyle has been observed
in a single frame without the conformant Promise handling.  But once we have the
conformant Promise handling we can tell it in a later frame.
(see https://bugzilla.mozilla.org/show_bug.cgi?id=1415457#c1 to know what's
going on there)

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

Comment 7

[Hiroyuki Ikezoe (:hiro)]

The first two patches have been already reviewed in bug 1413817 though, I did modify the commit message a bit.

Comment 8

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

Comment on attachment 8937375 [details]
Bug 1425771 - Add a function to check detect whether have conformant Promise handling and set the flag to represent it.

https://reviewboard.mozilla.org/r/208052/#review213854

::: dom/animation/test/mozilla/file_restyles.html:118
(Diff revision 1)
>      return;
>    }
>    add_task(test);
>  }
>  
> +async function isConformant() {

I don't think you need 'async' here

Comment 9

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

Comment on attachment 8937376 [details]
Bug 1425771 - Add a another variant of restyling_transform_animations_in_scrolled_out_element for the conformant Promise handling.

https://reviewboard.mozilla.org/r/208054/#review213856

r=me but please try to verify this will not fail intermittently

::: dom/animation/test/mozilla/file_restyles.html:452
(Diff revision 1)
> +          // unthrottled in this tick, let's see what observes in this tick's
> +          // restyling process.

Nit: /see what observes/see what we observe/

Comment 10

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

Comment on attachment 8937377 [details]
Bug 1425771 - Tweak expected restyle count for the case where the animation begins at the current time.

https://reviewboard.mozilla.org/r/208056/#review213858

r=me with comment tweaks

::: dom/animation/test/mozilla/file_restyles.html:111
(Diff revision 1)
> +    // If we don't have the conformant Promise handling and |aAnimation|
> +    // doesn't begin at the current timeline time, we will see an additional
> +    // restyling in the last frame.

This comment begs the question "Why?". I think that is answered by the comment below, so perhaps if we move that comment before this method we can drop this comment and the earlier comment in this function?

::: dom/animation/test/mozilla/file_restyles.html:165
(Diff revision 1)
>      // Normally we expect one restyling for each requestAnimationFrame (as
>      // called by observeRestyling) PLUS one for the last frame becasue of bug
>      // 1193394.  However, we won't observe that initial restyling unless BOTH of
>      // the following two conditions hold:
>      //
>      // 1. We are running *before* restyling happens.
>      // 2. The animation actually needs a restyle because it started prior to
>      //    this frame.  Even if (1) is true, in some cases due to aligning with
>      //    the refresh driver, the animation fame in which the ready promise is
>      //    resolved happens to coincide perfectly with the start time of the
>      //    animation.  In this case no restyling is needed so we won't observe
>      //    an additional restyle.

Should this comment be moved to be with tweakExpectedRestyleCount?

Comment 11

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

Comment on attachment 8937378 [details]
Bug 1425771 - Add a branch in only_one_restyling_after_finish_is_called for the conformant Promise handling.

https://reviewboard.mozilla.org/r/208058/#review213860

Comment 12

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #9)
> Comment on attachment 8937376 [details]
> Bug 1425771 - Add a another variant of
> restyling_transform_animations_in_scrolled_out_element for the conformant
> Promise handling.
> 
> https://reviewboard.mozilla.org/r/208054/#review213856
> 
> r=me but please try to verify this will not fail intermittently

Yeah, I've verified it on the try in comment 1 and comment 2.  This test case did not fail at all there, but I am not 100% sure the test case never fails.  Anyway I think the conformant Promise handling makes these tests more robust (less flaky) and shows us clearly what happens in failure cases.  So in any failure cases I will tackle to make them more reliable. :)

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8937375 [details]
Bug 1425771 - Add a function to check detect whether have conformant Promise handling and set the flag to represent it.

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

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8937376 [details]
Bug 1425771 - Add a another variant of restyling_transform_animations_in_scrolled_out_element for the conformant Promise handling.

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

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8937377 [details]
Bug 1425771 - Tweak expected restyle count for the case where the animation begins at the current time.

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

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8937378 [details]
Bug 1425771 - Add a branch in only_one_restyling_after_finish_is_called for the conformant Promise handling.

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

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/14c5ca99af0c
Add a function to check detect whether have conformant Promise handling and set the flag to represent it. r=birtles
https://hg.mozilla.org/integration/autoland/rev/a28293b06afe
Add a another variant of restyling_transform_animations_in_scrolled_out_element for the conformant Promise handling. r=birtles
https://hg.mozilla.org/integration/autoland/rev/848047469b27
Tweak expected restyle count for the case where the animation begins at the current time. r=birtles
https://hg.mozilla.org/integration/autoland/rev/984d714aa987
Add a branch in only_one_restyling_after_finish_is_called for the conformant Promise handling. r=birtles

Comment 18

[Cosmin Sabou [:CosminS]]

Backed out for ESlint failure

https://treeherder.mozilla.org/logviewer.html#?job_id=152066833&repo=autoland&lineNumber=565

https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=984d714aa987dd15ffb9dbde66f8a06559b49077&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&selectedJob=152066833

https://hg.mozilla.org/integration/autoland/rev/a3e33b6c77ea49907cef904c43efa08c3138279b

Comment 19

[Hiroyuki Ikezoe (:hiro)]

Was the ESlint failure really caused by these changes?

I did run "./mach lint -l eslint dom/animation" locally just in case, and there is no error reports.
 
I am wondering the failure is just a known timeout (bug 1405388), isn't it?

Comment 20

[Cosmin Sabou [:CosminS]]

Yes, it looks like it was caused by that bug and I asked myself the same question but the policy from what I know is to backout for every eslint failure. I was erring on the caution side. Maybe :Aryx could help and shed some light on what to do for future issues like this. Thanks!

Comment 21

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

Also, do we no longer ping developers on #developers before backing them out? Perhaps that doesn't apply to autoland? It seems like this could have been resolved on IRC, avoiding the unnecessary backout.

Comment 22

[Hiroyuki Ikezoe (:hiro)]

Though we haven't received the answer yet, but I believe these change set does pass ESlint.  So I will land them soon.

I think what we should do is to notice the fact that bug 1405388 is harmless to all sheriffs.  It would be nice to re-trigger the ESlint job when the timeout happens.  Of course we should fix bug 1405388 as soon as possible.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/23541a04b924
Add a function to check detect whether have conformant Promise handling and set the flag to represent it. r=birtles
https://hg.mozilla.org/integration/autoland/rev/defd32520915
Add a another variant of restyling_transform_animations_in_scrolled_out_element for the conformant Promise handling. r=birtles
https://hg.mozilla.org/integration/autoland/rev/4f15412de9e8
Tweak expected restyle count for the case where the animation begins at the current time. r=birtles
https://hg.mozilla.org/integration/autoland/rev/251fb7b39413
Add a branch in only_one_restyling_after_finish_is_called for the conformant Promise handling. r=birtles

Comment 24

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/23541a04b924
https://hg.mozilla.org/mozilla-central/rev/defd32520915
https://hg.mozilla.org/mozilla-central/rev/4f15412de9e8
https://hg.mozilla.org/mozilla-central/rev/251fb7b39413

Comment 25

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

(In reply to Cosmin Sabou [:cosmin_sabou] from comment #20)
> Yes, it looks like it was caused by that bug and I asked myself the same
> question but the policy from what I know is to backout for every eslint
> failure. I was erring on the caution side. Maybe :Aryx could help and shed
> some light on what to do for future issues like this. Thanks!
This was an infrastructure issue (see the suggested bug 1405388) and should not have been backed out. Issues detected by Eslint which justify a backout mention files, lines and issue types (rarely the failure summary might be empty and you will have to check the raw log for this).