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

Status: RESOLVED INCOMPLETE

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

Description

[Ethan Lin[:ethlin]]

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.

Comment 1

[Ethan Lin[:ethlin]]

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.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Though I don't quite understand what you are trying to do is, we don't use PostRestyleEvent for animations, we use PostRestyleEventForAnimations instead.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

(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).

Comment 4

[Ethan Lin[:ethlin]]

(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.

Comment 5

[Ethan Lin[:ethlin]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 6

[Ethan Lin[:ethlin]]

Attachment: Bug 1425315 - wip

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

Comment 7

[Ethan Lin[:ethlin]]

Comment on attachment 8942619 [details]
Bug 1425315 - wip

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

Comment 8

[Ethan Lin[:ethlin]]

Comment on attachment 8942619 [details]
Bug 1425315 - wip

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

Comment 9

[Hiroyuki Ikezoe (:hiro)]

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

Comment 10

[Kartikaya Gupta (email:[e-mail])]

(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?

Comment 11

[Hiroyuki Ikezoe (:hiro)]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 12

[Kartikaya Gupta (email:[e-mail])]

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!

Comment 13

[Hiroyuki Ikezoe (:hiro)]

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.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Thank you for doing all that!