Closed
Bug 980207
Opened 9 years ago
Closed 9 years ago
When the refresh driver is under test control, don't force updates to the compositor thread unless they would run otherwise
Categories
(Core :: Graphics: Layers, defect)
Core
Graphics: Layers
Tracking
()
RESOLVED
FIXED
mozilla30
People
(Reporter: birtles, Assigned: birtles)
References
Details
Attachments
(1 file)
I've noticed some bugs in regular usage of OMTA. However, making a reproducible test case is difficult because code in CompositorParent forces updates on the compositor thread when the refresh driver is under test control that do not occur in regular usage. I would like to alter the code in CompositorParent to better match the behavior in regular usage. For example, by only doing an update if there is already a composite scheduled. (Arguably these sort of bugs could be reproduced using reftests but we are currently unable to control the refresh driver in reftests due to our policy of making reftests, as best as is possible, cross-browser. We could try and set up each test so that it reaches the bug scenario within a few milliseconds and then snapshot it then but that approach is often flaky and time-consuming since we have to allow a generous window of time to be sure the animation has actually started. So, I think it's appropriate to adapt the testing methods here to allow testing these sort of scenarios using mochitests.)
Assignee | ||
Comment 1•9 years ago
|
||
In order to make test behavior better match real-world sampling behavior, this patch updates the test code in CompositorParent so that it only updates the layer when a composite is scheduled. This enables creating mochitests that reproduce bugs observed in regular usage.
Assignee | ||
Updated•9 years ago
|
Attachment #8386592 -
Flags: review?(dzbarsky)
Comment 2•9 years ago
|
||
Comment on attachment 8386592 [details] [diff] [review] When the refresh driver is under test control, make compositor thread only update when a composite is scheduled Review of attachment 8386592 [details] [diff] [review]: ----------------------------------------------------------------- LGMT, but do you think it's worth adding another method or a boolean param that would force a flush? Probably not worth worrying about until we need it... ::: gfx/layers/ipc/CompositorParent.cpp @@ +384,5 @@ > + mCurrentCompositeTask->Cancel(); > + mCurrentCompositeTask = nullptr; > + } > + } > + You repeat this pattern twice, factor it out?
Attachment #8386592 -
Flags: review?(dzbarsky) → review+
Assignee | ||
Comment 3•9 years ago
|
||
(In reply to David Zbarsky (:dzbarsky) from comment #2) > Comment on attachment 8386592 [details] [diff] [review] > ... > LGMT, but do you think it's worth adding another method or a boolean param > that would force a flush? Probably not worth worrying about until we need > it... Yeah, I'd rather leave this until we need it. I think it's best if, by default, we test in conditions that are as close as possible to regular sampling behavior. If we need to force a flush we should first stop and ask why. (i.e. why isn't just waiting for the next paint enough?) > ::: gfx/layers/ipc/CompositorParent.cpp > @@ +384,5 @@ > > + mCurrentCompositeTask->Cancel(); > > + mCurrentCompositeTask = nullptr; > > + } > > + } > > + > > You repeat this pattern twice, factor it out? Sounds good. I'll add that next week.
Assignee | ||
Comment 4•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/96f0f09c23c0
Comment 5•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/96f0f09c23c0
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in
before you can comment on or make changes to this bug.
Description
•