Closed Bug 1425771 Opened 3 years ago Closed 3 years ago

Tweak test_restyles.html for the conformant Promise handling

Categories

(Core :: DOM: Animation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

Details

Attachments

(4 files)

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)
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
The first two patches have been already reviewed in bug 1413817 though, I did modify the commit message a bit.
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
Attachment #8937375 - Flags: review?(bbirtles) → review+
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/
Attachment #8937376 - Flags: review?(bbirtles) → review+
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?
Attachment #8937377 - Flags: review?(bbirtles) → review+
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
Attachment #8937378 - Flags: review?(bbirtles) → review+
(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. :)
Pushed by hikezoe@mozilla.com:
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
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?
Flags: needinfo?(hikezoe) → needinfo?(csabou)
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!
Flags: needinfo?(csabou) → needinfo?(aryx.bugmail)
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.
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.
Pushed by hikezoe@mozilla.com:
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
(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).
Flags: needinfo?(aryx.bugmail)
You need to log in before you can comment on or make changes to this bug.