Closed Bug 1425315 Opened 4 years ago Closed 4 years ago

Change reftest harness to make it work for webrender's OMTA

Categories

(Core :: Graphics: WebRender, defect, P1)

defect

Tracking

()

RESOLVED INCOMPLETE

People

(Reporter: ethlin, Assigned: kats)

References

(Blocks 1 open bug)

Details

(Whiteboard: [wr-mvp] [triage][gfx-noted])

Attachments

(1 file)

59 bytes, text/x-review-board-request
Details
See https://bugzilla.mozilla.org/show_bug.cgi?id=1419851#c29. We need to fix it to make OMTA really work. Current solution is that we add another key word to replace some the reftest-wait in some testcases which will make test not wait for the schedule flush which is just triggered by animation.
Whiteboard: [wr-mvp] [triage][gfx-noted]
I'm still struggling to work on this. It's easy to break other tests or break the non-webrender tests. It looks like the RestyleManager doesn't know where the style changes come from. So I have to record the value for gecko/stylo restyle managers. The latest try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4475c1644032279c68d79e5d7835f27d2cc6a876&selectedJob=152510722. The good news is that the QR's R8 is green now. So I need to fix R4 and the non-webrender tests.
Assignee: nobody → ethlin
Summary: Change reftest harness to make it work with OMTA → Change reftest harness to make it work for webrender's OMTA
Though I don't quite understand what you are trying to do is, we don't use PostRestyleEvent for animations, we use PostRestyleEventForAnimations instead.
(In reply to Ethan Lin[:ethlin] from comment #1)
> So I need to fix
> R4 and the non-webrender tests.

The R4 failure is a test that is actually failing in current m-c. You removed the fails-if at https://hg.mozilla.org/try/rev/fba495157929#l11.12 but "fixing" it should be as simple as not removing the fails-if, since it's not really a regression.

I have patches in bug 1426386 which also fix this test (part 2 on that bug removes the fails-if).
See Also: → 1419226
(In reply to Hiroyuki Ikezoe (:hiro) from comment #2)
> Though I don't quite understand what you are trying to do is, we don't use
> PostRestyleEvent for animations, we use PostRestyleEventForAnimations
> instead.

Thanks. I'm not familiar with animation and restyle manager, though my patch seems to work. I will check the PostRestyleEventForAnimations path as well.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #3)
> The R4 failure is a test that is actually failing in current m-c. You
> removed the fails-if at https://hg.mozilla.org/try/rev/fba495157929#l11.12
> but "fixing" it should be as simple as not removing the fails-if, since it's
> not really a regression.
> 
> I have patches in bug 1426386 which also fix this test (part 2 on that bug
> removes the fails-if).

Right, I should keep the fails-if. I fixed the non-webrender tests, so I think this method works.
Assignee: demo99 → bugmail
So, the problem of reftest for OMTA is that after reftest-wait was removed, we do flush styles by getBoundingClientRect() in FlushRendering() [1] which is invoked by setTimeout(0) in the callback of MozAfterPaint and by updateLayerTree() in SynchronizeForSnapshot() [2] which is called from the callback of MozAfterPaint.  Meanwhile when we flush styles we also flush styles for OMTA, thus ScheduleViewManagerFlush() is called thus we do call NotifyInvalidation() in nsDisplay::PaintRoot() for WebRender.  It means there is no chance that isMozAfterPaintPending flag is cleared until the OMTA finished.   A mitigating way I can think of is to add reftest-no-flush flag to all reftests that has OMTA.

[1] https://hg.mozilla.org/mozilla-central/file/529754159078/layout/tools/reftest/reftest-content.js#l508
[2] https://hg.mozilla.org/mozilla-central/file/529754159078/layout/tools/reftest/reftest-content.js#l1101
(In reply to Hiroyuki Ikezoe (:hiro) from comment #9)
> ... Meanwhile when we flush styles we also flush
> styles for OMTA, thus ScheduleViewManagerFlush() is called thus we do call
> NotifyInvalidation() in nsDisplay::PaintRoot() for WebRender. ...

This was the part that I was planning to change eventually (per bug 1419851 comment 30 / comment 38) and you said it sounded reasonable. Do you still feel that approach is viable, or do you think it won't work?
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #10)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #9)
> > ... Meanwhile when we flush styles we also flush
> > styles for OMTA, thus ScheduleViewManagerFlush() is called thus we do call
> > NotifyInvalidation() in nsDisplay::PaintRoot() for WebRender. ...
> 
> This was the part that I was planning to change eventually (per bug 1419851
> comment 30 / comment 38) and you said it sounded reasonable. Do you still
> feel that approach is viable, or do you think it won't work?

I am now thinking we can tweak/make reftest harness more sane.  As for getBoundingClientRect() call, we shouldn't do it when we are already in the sate of STATE_WAITING_TO_FINISH.  As for updateLayerTree() I think it's not necessary in most cases, it seems to be just needed for synchronize plugin process.
Ok, fair enough. You understand all this way better than me, so I'm happy to let you work on it :) Let me know if you need me to do anything!
I believe we no longer need to fix this.  Bug 1442063, bug 1447870, bug 1447874 and bug 1441713 made reftest harness work on webrender now.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → INCOMPLETE
Thank you for doing all that!
You need to log in before you can comment on or make changes to this bug.