1329609 - Enable some reftests for WebRenderBorderLayer

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect)

Description

[Ethan Lin[:ethlin]]

Per bug 1322079 comment 18, I will turn perf on for some reftest. And we should also reduce the limitation for using BorderLayer.

Comment 1

[Ethan Lin[:ethlin]]

I tried to enable pref by default, but I got many failures[1]. There seems to be some repainting problem when style changing. I'll create another bug for it. For this bug, I'll turn on the pref for some reftests. Almost every reftest has border layers.


[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f9960a71f821da59977a507055de96fdede4094&selectedJob=67919141

Comment 2

[Ethan Lin[:ethlin]]

Attachment: enable pref for some reftests

Set pref(layers.advanced.border-layers, true) for some reftests. There are still many reftests failures when enabling it. Turning the pref on for some reftests is just for preventing other patches breaking it.

Comment 3

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

Note that setting this pref on individual tests will also set it for non-webrender builds. Did you do a try push to see if it causes those to fail? Your try push in comment 1 only has reftests for the QR builds, not regular linux/mac/win.

Comment 4

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

Comment on attachment 8827354 [details] [diff] [review]
enable pref for some reftests

Review of attachment 8827354 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed. Please make sure it doesn't cause non-QR reftests to fail.

::: layout/reftests/box-sizing/reftest.list
@@ +1,2 @@
> +pref(layers.advanced.border-layers,true) == intrinsic-1a.html intrinsic-1-ref.html
> +pref(layers.advanced.border-layers,true) == intrinsic-1b.html intrinsic-1-ref.html

Instead of just modifying these reftests to have the pref, can you run them twice - once with the pref set and once without? That way we don't lose any reftest coverage.

::: layout/tools/reftest/reftest-preferences.js
@@ +130,5 @@
>  
>  user_pref("media.openUnsupportedTypeWithExternalApp", false);
> +
> +// For WebRenderBorderLayer
> +user_pref("layers.advanced.border-layers", false);

I'd rather put this in modules/libpref/init/all.js instead.

Comment 5

[Ethan Lin[:ethlin]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #4)
> Comment on attachment 8827354 [details] [diff] [review]
> enable pref for some reftests
> 
> Review of attachment 8827354 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with comments addressed. Please make sure it doesn't cause non-QR
> reftests to fail.
> 
> ::: layout/reftests/box-sizing/reftest.list
> @@ +1,2 @@
> > +pref(layers.advanced.border-layers,true) == intrinsic-1a.html intrinsic-1-ref.html
> > +pref(layers.advanced.border-layers,true) == intrinsic-1b.html intrinsic-1-ref.html
> 
> Instead of just modifying these reftests to have the pref, can you run them
> twice - once with the pref set and once without? That way we don't lose any
> reftest coverage.
> 

I will add another line for running each test twice.   

> ::: layout/tools/reftest/reftest-preferences.js
> @@ +130,5 @@
> >  
> >  user_pref("media.openUnsupportedTypeWithExternalApp", false);
> > +
> > +// For WebRenderBorderLayer
> > +user_pref("layers.advanced.border-layers", false);
> 
> I'd rather put this in modules/libpref/init/all.js instead.

Okay!

Comment 6

[Ethan Lin[:ethlin]]

try looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f348b78ebe215df1de31cd98dfc6298b3b0e6d9b

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/graphics/rev/e18248e42bfc
Turn on layers.advanced.border-layers for some reftests. r=kats

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/graphics/rev/e0dc054dad00
Move new pref outside ifdef so it is available to non-QR reftests. r=ethlin?

Comment 9

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

Backed out for non-QR reftest failures. I take it you didn't check those passed? :)

https://hg.mozilla.org/projects/graphics/rev/8fcc95d1411532306d08b49471214e0f0fcd7a8c

Comment 10

[Ethan Lin[:ethlin]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #9)
> Backed out for non-QR reftest failures. I take it you didn't check those
> passed? :)
> 
> https://hg.mozilla.org/projects/graphics/rev/
> 8fcc95d1411532306d08b49471214e0f0fcd7a8c

oops, I see. Thanks!

Comment 11

[Ethan Lin[:ethlin]]

Attachment: enable pref for some reftests

Looks like it crashed when doing layer transaction. I add skip-if(!webrender) to prevent it. Should we support advanced layer for non-QR?

Comment 12

[Ethan Lin[:ethlin]]

(In reply to Ethan Lin[:ethlin] from comment #11)
> Created attachment 8828248 [details] [diff] [review]
> enable pref for some reftests
> 
> Looks like it crashed when doing layer transaction. I add
> skip-if(!webrender) to prevent it. Should we support advanced layer for
> non-QR?

If we should, I can have another bug for it. I guess we don't handle some IPC attributes correctly.

Comment 13

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

Comment on attachment 8828248 [details] [diff] [review]
enable pref for some reftests

Review of attachment 8828248 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah I guess this is OK for now. Change the commit message to "... for some reftests when running with webrender."

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/graphics/rev/309035ec7be2
Turn on layers.advanced.border-layers for some reftests with webrender. r=kats