1132371 - /layout/reftests/bugs/1119117-1b.html fails on Windows/mac when event-regions are enabled

Status: RESOLVED FIXED

(Core :: Layout, defect)

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: patch

Without event regions this test had a background color item inside an opacity item, and the opacity item flattened away and passed it's opacity to the background color item and the background color item got a color layer. With event regions we get an event region items inside the opacity and the optimization no longer workers. And we get an-inactive opacity layer and painted layer. We get a 128 vs 127 difference in the result so I'm assuming that we round 127.5 ( = 255 * the opacity in the test of 0.5) differently depending of if it gets a color layer.

I extended the optimization to three child items because I thought I needed it for this testcase. But afterwards I checked and we only need two children for this particular case.

Comment 2

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8563277 [details] [diff] [review]
patch

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

::: layout/base/nsDisplayList.cpp
@@ +3865,5 @@
> +    }
> +  }
> +
> +  for (uint32_t i = 0; i < numChildren; i++) {
> +    if (children[i].item) {

How could this ever be false?

::: layout/base/nsDisplayList.h
@@ +1389,5 @@
>     */
>    virtual bool ApplyOpacity(nsDisplayListBuilder* aBuilder,
>                              float aOpacity,
>                              const DisplayItemClip* aClip) {
>      return false;

How about drop the bool retval and just assert that we don't run this function?

Comment 3

[Timothy Nikkel (:tnikkel)]

Attachment: patch v2

Good ideas, thank you.

Comment 4

[Timothy Nikkel (:tnikkel)]

Attachment: interdiff

Comment 5

[Timothy Nikkel (:tnikkel)]

So this patch actually causes the reftest for bug 1119117 to fail on b2g. I checked and the reftest fails with event regions turned off. The reftest landed after event regions were turned on so that explains that. I guess the rounding issue goes the other way on b2g from mac/windows. Changing the opacity from 0.5 to 0.50196078431 fixes the rounding issue, so I'm just going to change the test.

Comment 6

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5ef8e549a1d9

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5ef8e549a1d9

Comment 8

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

tn, can you request uplift to b2g37 for this bug? It is needed in order to fix bug 1139541 on 2.2. Thanks!

Comment 9

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8566222 [details] [diff] [review]
patch v2

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): need this to fix bug 1139541
User impact if declined: bug 1139541
Testing completed: has been landed for quite a while now, no issues
Risk to taking this patch (and alternatives if risky): should be pretty safe, just extends an optimization to more cases
String or UUID changes made by this patch: none

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/caa446aa7a1f