Enable 'layers.advanced.background-color' by default

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ethlin, Assigned: ethlin)

Tracking

(Depends on 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

Assignee

Description

2 years ago
I suppose some failures will be similar with bug 1354463 ('layers.advanced.canvas-background-color'). But we still should have a bug for background-color.
The current try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=24b21095b276c3cb9a2b06af2dbe038c2cd669c3&selectedJob=92249781
Assignee

Updated

2 years ago
Depends on: 1359314
Assignee

Comment 1

2 years ago
Posted patch fuzzy_wip (obsolete) — Splinter Review
Assignee

Comment 2

2 years ago
I add fuzz to ignore some failures which is caused by layer tree change. The current try result is: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5c740502f38660a955d16e4afb937fed4ff9fa6&selectedJob=94701423.

The remaining failures aren't too many. Looks like there are about 4~5 issues. I can create bugs for these issues first.
Comment on attachment 8862297 [details] [diff] [review]
fuzzy_wip

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

::: layout/reftests/reftest-sanity/reftest.list
@@ +155,5 @@
>  # When using 565 fuzzy.html and fuzzy-ref.html will compare as equal
>  fails fuzzy-if(false,2,1) random-if(Android) == fuzzy.html fuzzy-ref.html
>  
>  # Test that reftest-no-paint fails correctly
> +fails random-if(webrender) == reftest-no-paint.html reftest-no-paint-ref.html

FYI the "reftest-sanity" suite is mostly intended to make sure that the reftest harness itself is working properly. Marking a test here as random-if is undesirable because it means that we don't know if the harness itself is working properly or not. We should look at this test and see if it makes sense to even run with webrender. If it does, we should make sure it passes; if it doesn't, then we can just skip-if(webrender) it with an explanation.
Assignee

Updated

2 years ago
Depends on: 1361662
Assignee

Updated

2 years ago
Depends on: 1361668
Assignee

Updated

2 years ago
Depends on: 1151016
Assignee

Updated

2 years ago
Depends on: 1365511
mask-gradient-translucent-end-color-1.html failure would be fixed by bug 1350663.
Depends on: 1350663
Assignee

Updated

2 years ago
Depends on: 1366692
Assignee

Comment 5

2 years ago
Almost all failures are fixed or identified. I think we can enable the background color layer by default. I added flags based on the try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5c740502f38660a955d16e4afb937fed4ff9fa6&selectedJob=94701423.
Attachment #8869959 - Flags: review?(bugmail)
Assignee

Comment 6

2 years ago
Attachment #8862297 - Attachment is obsolete: true
Attachment #8869960 - Flags: review?(bugmail)
Attachment #8869959 - Flags: review?(bugmail) → review+
Comment on attachment 8869960 [details] [diff] [review]
Part2. Enable "layers.advanced.background-color" by default

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

Please clean up commit messages to not have the file delta info
Attachment #8869960 - Flags: review?(bugmail) → review+
(In reply to Ethan Lin[:ethlin] from comment #5)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=e5c740502f38660a955d16e4afb937fed4ff9fa6&selectedJob=9
> 4701423.

Just noticed, this try push is a month old. Please verify with a recent try push that nothing has changed.
Comment on attachment 8869959 [details] [diff] [review]
Part1. Add reftest flags for background color layer to make try passed

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

::: layout/reftests/reftest-sanity/reftest.list
@@ +155,5 @@
>  # When using 565 fuzzy.html and fuzzy-ref.html will compare as equal
>  fails-if(!stylo) fuzzy-if(false,2,1) random-if(Android) == fuzzy.html fuzzy-ref.html
>  
> +# Test that reftest-no-paint fails correctly. We always create color layers with WR enabled and color layers will paint after the color changed.
> +fails skip-if(webrender) == reftest-no-paint.html reftest-no-paint-ref.html

The comment you added here seems a bit wrong. The reftest is expected to fail, because the test does a repaint. Presumably with webrender enabled we don't do a repaint because we generate the color layer instead and just update the color on that. So saying "color layers will paint after the color changed" is misleading.

::: layout/reftests/writing-mode/reftest.list
@@ +146,5 @@
>  # tests involving sideways-lr mode
>  == 1193519-sideways-lr-1.html 1193519-sideways-lr-1-ref.html
>  == 1193519-sideways-lr-2.html 1193519-sideways-lr-2-ref.html
>  fuzzy-if(winWidget,3,84) == 1193519-sideways-lr-3.html 1193519-sideways-lr-3-ref.html
> +fails-if(webrender) fuzzy-if(winWidget,3,112) == 1193519-sideways-lr-4.html 1193519-sideways-lr-4-ref.html # see bug 1366692. Rounding error with WR enabled.

Also here - I think you need the fails-if(webrender) to be after the fuzzy-if(winWidget). Otherwise on windows with webrender enabled it will expect a fuzzy pass instead of a failure. (The last matched condition overrides previous ones)
Assignee

Comment 10

2 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #8)
> (In reply to Ethan Lin[:ethlin] from comment #5)
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=e5c740502f38660a955d16e4afb937fed4ff9fa6&selectedJob=9
> > 4701423.
> 
> Just noticed, this try push is a month old. Please verify with a recent try
> push that nothing has changed.

Okay, I sent another try to check.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1ba9958a495d481211ddd6fb524a005bec8aaea&selectedJob=101427063
Assignee

Comment 11

2 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #9)
> Comment on attachment 8869959 [details] [diff] [review]
> Part1. Add reftest flags for background color layer to make try passed
> 
> Review of attachment 8869959 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/reftests/reftest-sanity/reftest.list
> @@ +155,5 @@
> >  # When using 565 fuzzy.html and fuzzy-ref.html will compare as equal
> >  fails-if(!stylo) fuzzy-if(false,2,1) random-if(Android) == fuzzy.html fuzzy-ref.html
> >  
> > +# Test that reftest-no-paint fails correctly. We always create color layers with WR enabled and color layers will paint after the color changed.
> > +fails skip-if(webrender) == reftest-no-paint.html reftest-no-paint-ref.html
> 
> The comment you added here seems a bit wrong. The reftest is expected to
> fail, because the test does a repaint. Presumably with webrender enabled we
> don't do a repaint because we generate the color layer instead and just
> update the color on that. So saying "color layers will paint after the color
> changed" is misleading.
> 
To my understanding, the test changes the background to black to match the reference test file. However, the test expects that the color change will not cause invalidation region change since the painted layer doesn't check the color value. So originally it will not repaint and it will fail. For WR, we use color layer so it will repaint after the color changed.

> ::: layout/reftests/writing-mode/reftest.list
> @@ +146,5 @@
> >  # tests involving sideways-lr mode
> >  == 1193519-sideways-lr-1.html 1193519-sideways-lr-1-ref.html
> >  == 1193519-sideways-lr-2.html 1193519-sideways-lr-2-ref.html
> >  fuzzy-if(winWidget,3,84) == 1193519-sideways-lr-3.html 1193519-sideways-lr-3-ref.html
> > +fails-if(webrender) fuzzy-if(winWidget,3,112) == 1193519-sideways-lr-4.html 1193519-sideways-lr-4-ref.html # see bug 1366692. Rounding error with WR enabled.
> 
> Also here - I think you need the fails-if(webrender) to be after the
> fuzzy-if(winWidget). Otherwise on windows with webrender enabled it will
> expect a fuzzy pass instead of a failure. (The last matched condition
> overrides previous ones)

Okay, I will change the flag order.
Assignee

Comment 12

2 years ago
According to the try result[1] with the patches, there are some new failures[2] about mask. I'll investigate these failures.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=be7d359a395a02bf7ee3274220cdf6fa843a342f&selectedJob=101485157
[2] layout/reftests/invalidation/mask-invalidation-2a.html, layout/reftests/invalidation/mask-invalidation-2b.html, layout/reftests/w3c-css/submitted/masking/mask-image-1a.html
(In reply to Ethan Lin[:ethlin] from comment #11)
> To my understanding, the test changes the background to black to match the
> reference test file. However, the test expects that the color change will
> not cause invalidation region change since the painted layer doesn't check
> the color value.

But the test expects that the color change *will* cause invalidation. That's why the reftest.list has a "fails" annotation. That is, the test is expected to fail the "no-paint" check, which means it's expected to do a repaint. If with WR the test is producing an UNEXPECTED-PASS, that must be because with WR we're not doing a repaint, and so the "no-paint" check is passing. Or am I missing something?
Assignee

Comment 14

2 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #13)
> (In reply to Ethan Lin[:ethlin] from comment #11)
> > To my understanding, the test changes the background to black to match the
> > reference test file. However, the test expects that the color change will
> > not cause invalidation region change since the painted layer doesn't check
> > the color value.
> 
> But the test expects that the color change *will* cause invalidation. That's
> why the reftest.list has a "fails" annotation. That is, the test is expected
> to fail the "no-paint" check, which means it's expected to do a repaint. If
> with WR the test is producing an UNEXPECTED-PASS, that must be because with
> WR we're not doing a repaint, and so the "no-paint" check is passing. Or am
> I missing something?

Oh..I think I misunderstood the meaning of the test. I just studied the flag "reftest-no-paint" again. You are right. I should change the comment for the test.
Assignee

Comment 15

2 years ago
(In reply to Ethan Lin[:ethlin] from comment #12)
> According to the try result[1] with the patches, there are some new
> failures[2] about mask. I'll investigate these failures.
> 
> [1]
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=be7d359a395a02bf7ee3274220cdf6fa843a342f&selectedJob=1
> 01485157
> [2] layout/reftests/invalidation/mask-invalidation-2a.html,
> layout/reftests/invalidation/mask-invalidation-2b.html,
> layout/reftests/w3c-css/submitted/masking/mask-image-1a.html

These new failures are due to the patch in bug 1350663. The patch will fix "mask-gradient-translucent-end-color-1.html". I would like to mark the reftest as fails-if before it's landed.
Assignee

Updated

2 years ago
Depends on: 1367991
Assignee

Updated

2 years ago
Blocks: 1367994
Assignee

Updated

2 years ago
No longer blocks: 1367994
Depends on: 1367994
Assignee

Comment 16

2 years ago
I got two more reftest failures based on the try result[1]. One is "dom/animation/test/crashtests/1272475-1.html" and other one is 
"layout/reftests/w3c-css/submitted/masking/mask-image-1-ref.html".
The 1272475-1.html is about NAN handling problem in rust and Morris is working on it. I created a bug 1367994 to track this issue and mark it as fails-if for now. The mask-image-1-ref.html is about mask layer invalidation and I will fix it in bug 1367991.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=9402f210d1a1fc3f227669dca27c69cdf7cbe0f9&selectedJob=102209235
Assignee

Comment 17

2 years ago
I adjusted some flags based on the new try result. Kats, could you have a review again? Thanks.
[1] is the try with the patches, including the fix in bug 1367991.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=160dbda62cafe7c98b50f06a2851fdbda7e4a876
Attachment #8869959 - Attachment is obsolete: true
Attachment #8871657 - Flags: review?(bugmail)
Comment on attachment 8871657 [details] [diff] [review]
Part1. Add reftest flags for background color layer to make try passed

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

::: dom/animation/test/crashtests/crashtests.list
@@ +6,5 @@
>  pref(dom.animations-api.core.enabled,true) load 1216842-4.html
>  pref(dom.animations-api.core.enabled,true) load 1216842-5.html
>  pref(dom.animations-api.core.enabled,true) load 1216842-6.html
> +skip-if(webrender) pref(dom.animations-api.core.enabled,true) load 1272475-1.html # see bug 1367994
> +skip-if(webrender) pref(dom.animations-api.core.enabled,true) load 1272475-2.html # see bug 1367994

According to your try push in comment 16 only 1272475-1 is failing, the second one seems to run ok. Can we leave 1272475-2 without any annotation?
Attachment #8871657 - Flags: review?(bugmail) → review+
Assignee

Comment 19

2 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #18)
> Comment on attachment 8871657 [details] [diff] [review]
> Part1. Add reftest flags for background color layer to make try passed
> 
> Review of attachment 8871657 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/animation/test/crashtests/crashtests.list
> @@ +6,5 @@
> >  pref(dom.animations-api.core.enabled,true) load 1216842-4.html
> >  pref(dom.animations-api.core.enabled,true) load 1216842-5.html
> >  pref(dom.animations-api.core.enabled,true) load 1216842-6.html
> > +skip-if(webrender) pref(dom.animations-api.core.enabled,true) load 1272475-1.html # see bug 1367994
> > +skip-if(webrender) pref(dom.animations-api.core.enabled,true) load 1272475-2.html # see bug 1367994
> 
> According to your try push in comment 16 only 1272475-1 is failing, the
> second one seems to run ok. Can we leave 1272475-2 without any annotation?

Okay, I will remove it.
Assignee

Comment 20

2 years ago
There is two new reftest failures[1] object-position-webm-001.html and object-position-webm-002.html in R8. It looks like we should set 'fails-if(webrender&&browserIsRemote)' for the two test, same as other webm tests. But there is a 'fails' for these two tests. Per discussion with Jerry, the annotation for webm tests will become fails-if(webrender) with his patches. So for now I would like skip these two tests.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=92aae8ae3c741ee29ec090157cfa0570f957fc71&selectedJob=103952160
[2] https://dxr.mozilla.org/mozilla-central/source/layout/reftests/webm-video/reftest.list#44
Assignee

Comment 21

2 years ago
Attachment #8873780 - Flags: review?(howareyou322)
Attachment #8873780 - Flags: review?(howareyou322) → review+
Assignee

Updated

2 years ago
See Also: → 1369678

Comment 23

2 years ago
Pushed by ethlin@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb164b643bdb
Add reftest flags for background color layer to make try passed. r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/57c514102ec3
Enable background color layer by default for webrender. r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/21f37512b62a
Add webm reftest flags for webrender. r=pchang
(In reply to Ethan Lin[:ethlin] from comment #20)
> There is two new reftest failures[1] object-position-webm-001.html and
> object-position-webm-002.html in R8. It looks like we should set
> 'fails-if(webrender&&browserIsRemote)' for the two test, same as other webm
> tests. But there is a 'fails' for these two tests. Per discussion with
> Jerry, the annotation for webm tests will become fails-if(webrender) with
> his patches. So for now I would like skip these two tests.

In general I'm not a fan of skip-if when fails or fails-if will do. With skip-if the test is skipped entirely, so if the test exposes a crash regression we will miss it. If it fails randomly, then even random-if is better than skip-if.

Comment 25

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bb164b643bdb
https://hg.mozilla.org/mozilla-central/rev/57c514102ec3
https://hg.mozilla.org/mozilla-central/rev/21f37512b62a
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Assignee

Comment 26

2 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #24)
> (In reply to Ethan Lin[:ethlin] from comment #20)
> > There is two new reftest failures[1] object-position-webm-001.html and
> > object-position-webm-002.html in R8. It looks like we should set
> > 'fails-if(webrender&&browserIsRemote)' for the two test, same as other webm
> > tests. But there is a 'fails' for these two tests. Per discussion with
> > Jerry, the annotation for webm tests will become fails-if(webrender) with
> > his patches. So for now I would like skip these two tests.
> 
> In general I'm not a fan of skip-if when fails or fails-if will do. With
> skip-if the test is skipped entirely, so if the test exposes a crash
> regression we will miss it. If it fails randomly, then even random-if is
> better than skip-if.

The reftest expects always failed and my patches make it unexpected-passed on e10s, so I have to skip it for now. I created another bug 1369678 to tracking this issue. According to the try push[1] in bug 1366502, we can remove the skip-if after the patches in bug 1366502 are landed.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=17a699c3a79b6f0695df8202ed111f023afa4280
Assignee

Comment 27

2 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #24)
> (In reply to Ethan Lin[:ethlin] from comment #20)
> > There is two new reftest failures[1] object-position-webm-001.html and
> > object-position-webm-002.html in R8. It looks like we should set
> > 'fails-if(webrender&&browserIsRemote)' for the two test, same as other webm
> > tests. But there is a 'fails' for these two tests. Per discussion with
> > Jerry, the annotation for webm tests will become fails-if(webrender) with
> > his patches. So for now I would like skip these two tests.
> 
> In general I'm not a fan of skip-if when fails or fails-if will do. With
> skip-if the test is skipped entirely, so if the test exposes a crash
> regression we will miss it. If it fails randomly, then even random-if is
> better than skip-if.

Right, random-if should be better. I will use it next time. Thanks.
Assignee

Updated

2 years ago
Blocks: 1373530
You need to log in before you can comment on or make changes to this bug.