Closed Bug 1421476 Opened 7 years ago Closed 7 years ago

Prerequisite fixes for test_restyles.html with conformant Promise handling

Categories

(Core :: DOM: Animation, enhancement)

Product:
Core
Component:
DOM: Animation
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

Details

Attachments

(5 files)

Bug 1421476 - Add waitForNextFrame() that ensures the state in the next frame.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1421476 - Make waitForAnimationFrames wait for the correct number of consecutive animation frames for the given count.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1421476 - Use waitForNextFrame() instead of waitForFrame() in file_restyles.html.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1421476 - Change style before waiting for a single requestAnimationFrame.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1421476 - Wait for the next frame after waitForWheelEvent().
59 bytes, text/x-review-board-request
birtles
: review+
Details 
Split out from bug 1413817.

I am going to land some of patches for test_restyles.html that can be work without the conformant Promise handling.
Comment on attachment 8932666 [details]
Bug 1421476 - Add waitForNextFrame() that ensures the state in the next frame.

https://reviewboard.mozilla.org/r/203724/#review209206

::: dom/animation/test/testcommon.js:215
(Diff revision 1)
>      window.requestAnimationFrame(resolve);
>    });
>  }
>  
>  /**
> + * Waits for a requestAnimationFrame that have happened in the next tick.

"Waits for a requestAnimationFrame callback in the next refresh driver tick."

?

::: dom/animation/test/testcommon.js:218
(Diff revision 1)
>  
>  /**
> + * Waits for a requestAnimationFrame that have happened in the next tick.
> + */
> +function waitForNextFrame() {
> +  var timeAtStart = document.timeline.currentTime;

This requires the full API to be enabled. Do we need to document that?

Also, while we're using ES6, let's do s/var/const here.
Attachment #8932666 - Flags: review?(bbirtles) → review+
Comment on attachment 8932667 [details]
Bug 1421476 - Make waitForAnimationFrames wait for the correct number of consecutive animation frames for the given count.

https://reviewboard.mozilla.org/r/203726/#review209208

::: commit-message-1cf49:1
(Diff revision 1)
> +Bug 1421476 - Make waitForAnimationFrames waiting for correct consective animation frames for the given count. r?birtles

Nit: s/waiting for/wait for/
s/correct/the correct number of/

::: commit-message-1cf49:4
(Diff revision 1)
> +are cases that we don't get to the next frame even we got a
> +requestAnimationFrame.  So we should't decrease the given count if we are still
> +the same frame.

...are cases where a requestAnimationFrame callback can happen in the same tick of the refresh driver, that is, the same animation frame. We should only count callbacks that occur in frames subsequent to the current one.
Attachment #8932667 - Flags: review?(bbirtles) → review+
Comment on attachment 8932668 [details]
Bug 1421476 - Use waitForNextFrame() instead of waitForFrame() in file_restyles.html.

https://reviewboard.mozilla.org/r/203728/#review209210
Attachment #8932668 - Flags: review?(bbirtles) → review+
Comment on attachment 8932669 [details]
Bug 1421476 - Change style before waiting for a single requestAnimationFrame.

https://reviewboard.mozilla.org/r/203730/#review209212

Excellent explanation, thank you!
Attachment #8932669 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles) from comment #6)

> ::: dom/animation/test/testcommon.js:218
> (Diff revision 1)
> >  
> >  /**
> > + * Waits for a requestAnimationFrame that have happened in the next tick.
> > + */
> > +function waitForNextFrame() {
> > +  var timeAtStart = document.timeline.currentTime;
> 
> This requires the full API to be enabled. Do we need to document that?

Oh right, good catch!  For now, we don't use it without the pref, but someone might try to use it without the pref somewhere.  I will leave a comment there.  Thanks! 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f48e07d4f88a04ab2b0c3ee108b9617be5bba79
Comment on attachment 8932670 [details]
Bug 1421476 - Wait for the next frame after waitForWheelEvent().

https://reviewboard.mozilla.org/r/203732/#review209214

::: commit-message-607f0:5
(Diff revision 1)
> +received.  The MozAfterPaint is processed after we did a paint process.  That
> +means that we have no chance to process restyling between MozAfterPaint and
> +requestAnimationFrame.

Perhaps drop the sentence that begins, "That means that..." and add:

However, observeStyling counts the number of requestAnimationFrame callbacks and yet there will be no opportunity to process restyles between the MozAfterPaint callback and the next call to requestAnimationFrame. As a result, if we are expecting restyles to happen on every frame, our count will be off by one. To avoid this, we wait until the next requestAnimationFrame callback before calling observeStyling.

Note that we *could* try to adjust observeStyling to automatically do this for us but sometimes we want observeStyling to observe restyles in the *current* frame. Since there's no obvious way to detect what we are trying to do it's easier to just let each test decide from which point it wants to count restyles.

::: dom/animation/test/mozilla/file_restyles.html:383
(Diff revision 1)
>         'Animations running on the main-thread for elements ' +
>         'which are scrolled out should never cause restyles');
>  
>      await waitForWheelEvent(parentElement);
>  
> +    await waitForNextFrame();

As per the suggested commit message, it might be more accurate to actually write waitForFrame here?

Perhaps we could add a comment:

// Make sure we are ready to restyle before counting restyles

?

I don't think we necessarily need to add that comment each time we call waitForFrame (or waitForNextframe) but if we added it before the first time we call it, it might make it easier in future to work out why we are calling it later in the test too.
Attachment #8932670 - Flags: review?(bbirtles) → review+
Thanks for the revised explanation!

(In reply to Brian Birtles (:birtles) from comment #11)
> ::: dom/animation/test/mozilla/file_restyles.html:383
> (Diff revision 1)
> >         'Animations running on the main-thread for elements ' +
> >         'which are scrolled out should never cause restyles');
> >  
> >      await waitForWheelEvent(parentElement);
> >  
> > +    await waitForNextFrame();
> 
> As per the suggested commit message, it might be more accurate to actually
> write waitForFrame here?
> 
> Perhaps we could add a comment:
> 
> // Make sure we are ready to restyle before counting restyles
> 
> ?
> 
> I don't think we necessarily need to add that comment each time we call
> waitForFrame (or waitForNextframe) but if we added it before the first time
> we call it, it might make it easier in future to work out why we are calling
> it later in the test too.

Agree.  Sounds reasonable to me.  Thanks!

A final try;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=36ffc1762392b800c20f8b8e4b3a1022a3f81d4a
Forgot to use const instead of var.:/
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/71f71ba8908a
Add waitForNextFrame() that ensures the state in the next frame. r=birtles
https://hg.mozilla.org/integration/autoland/rev/f3ab006af534
Make waitForAnimationFrames wait for the correct number of consecutive animation frames for the given count. r=birtles
https://hg.mozilla.org/integration/autoland/rev/dcb000f97be8
Use waitForNextFrame() instead of waitForFrame() in file_restyles.html. r=birtles
https://hg.mozilla.org/integration/autoland/rev/027cc4283112
Change style before waiting for a single requestAnimationFrame. r=birtles
https://hg.mozilla.org/integration/autoland/rev/bcb4ebc272ca
Wait for the next frame after waitForWheelEvent(). r=birtles
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: