We sometimes get a sequence of display item of the form: Opacity 12ad0a1c0(Block(div)(1)) bounds(480,5220,960,960) visible(480,5220,960,960) componentAlpha(0,0,0,0) clip(0,4740,57600,62040) (opacity 0.800000) layer=12a091800 Opacity 12ad0a5e0(Block(div)(1)) bounds(480,5220,960,960) visible(480,5220,960,960) componentAlpha(0,0,0,0) clip() (opacity 0.300000) layer=12a095c00 BackgroundColor 12ad0a928(Block(div)(1)) bounds(480,5220,960,960) visible(480,5220,960,960) componentAlpha(0,0,0,0) clip() uniform (opaque 480,5220,960,960) (rgba 255,0,0,255) layer=12a096c00 These end up creating a separate ThebesLayer for something that could either get a color layer or be folded into the parent ThebesLayer.

As I discussed with matt we actually can't report nsDisplayOpacity as opaque if we have will-change. Updating patch and comment to reflect that.

Note that the error can accumulate every time we merge.

Opacity 1331ac1c0(Block(div)(1)) bounds(480,5220,960,960) visible(0,0,0,0) componentAlpha(0,0,0,0) clip(0,4740,57600,62040) (opacity 0.800000) Opacity 1331ac5e0(Block(div)(1)) bounds(480,5220,960,960) visible(0,0,0,0) componentAlpha(0,0,0,0) clip() (opacity 0.300000) Opacity 1331ac7d8(Block(div)(1)) bounds(480,5220,960,960) visible(0,0,0,0) componentAlpha(0,0,0,0) clip() (opacity 0.300000) BackgroundColor 1331acb20(Block(div)(1)) bounds(480,5220,960,960) visible(0,0,0,0) componentAlpha(0,0,0,0) clip() uniform (opaque 480,5220,960,960) (rgba 255,0,0,255) layer=130939800 -> BackgroundColor 1331acb20(Block(div)(1)) bounds(480,5220,960,960) visible(480,5220,960,960) componentAlpha(0,0,0,0) clip(0,4740,57600,62040) uniform (rgba 255,0,0,18) layer=130939800

Address try failure, non trivial changes so re-requesting review: https://tbpl.mozilla.org/?tree=Try&rev=a8f5cf586813

Comment on attachment 8439490 [details] [diff] [review] Part 2: Flatten nsDisplayBackgroundColor v2 >@@ -2881,21 +2883,21 @@ nsCSSRendering::PaintBackgroundColorWith > // Determine whether we are drawing background images and/or > // background colors. > bool drawBackgroundImage; > bool drawBackgroundColor; >- nscolor bgColor = DetermineBackgroundColor(aPresContext, >- aBackgroundSC, >- aForFrame, >- drawBackgroundImage, >- drawBackgroundColor); >+ DetermineBackgroundColor(aPresContext, >+ aBackgroundSC, >+ aForFrame, >+ drawBackgroundImage, >+ drawBackgroundColor); Ignoring the return value of DetermineBackgroundColor means this is probably going to screw up printing. > void > nsDisplayBackgroundColor::Paint(nsDisplayListBuilder* aBuilder, >- nsRenderingContext* aCtx) >+ nsRenderingContext* aCtx) > { > if (mColor == NS_RGBA(0, 0, 0, 0)) { > return; > } Comparing gfxRGBA and nscolor here?

(In reply to Timothy Nikkel (:tn) from comment #9) > Comment on attachment 8439490 [details] [diff] [review] > Part 2: Flatten nsDisplayBackgroundColor v2 > > >@@ -2881,21 +2883,21 @@ nsCSSRendering::PaintBackgroundColorWith > > // Determine whether we are drawing background images and/or > > // background colors. > > bool drawBackgroundImage; > > bool drawBackgroundColor; > >- nscolor bgColor = DetermineBackgroundColor(aPresContext, > >- aBackgroundSC, > >- aForFrame, > >- drawBackgroundImage, > >- drawBackgroundColor); > >+ DetermineBackgroundColor(aPresContext, > >+ aBackgroundSC, > >+ aForFrame, > >+ drawBackgroundImage, > >+ drawBackgroundColor); > > Ignoring the return value of DetermineBackgroundColor means this is probably > going to screw up printing. Can you explain more here please. Why would DetermineBackgroundColor return something different to what our display list code thinks the background color is? Isn't that potentially buggy in other ways?

(In reply to Matt Woodrow (:mattwoodrow) from comment #10) > Can you explain more here please. Why would DetermineBackgroundColor return > something different to what our display list code thinks the background > color is? Isn't that potentially buggy in other ways? We create nsDisplayBackgroundColor's with the color that comes back from DetermineBackgroundColor, so this should be fine after all. Sorry for the confusion. I was didn't know that we had already called DetermineBackgroundColor and this call is essentially redundant already.

Let's track what kinds of results we get here and consider this for Aurora (32) uplift.

Alright I fixed the invalidation problems: https://tbpl.mozilla.org/?tree=Try&rev=bbf5fe755e99 But now we're exposing ourselves to all sorts of rounding issues with reftest.

Previous run is looking surprising good. Pushing a full try run to be safe: https://tbpl.mozilla.org/?tree=Try&rev=670f96615b89

All done!

Comment on attachment 8440308 [details] [diff] [review] Part 2: Flatten nsDisplayBackgroundColor v3 (Fixed invalidation) Review of attachment 8440308 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/reftests/bugs/759036-1.html @@ +6,5 @@ > height: 300px; > width: 300px; > overflow:hidden; > border-radius:30px; > + opacity: 0.6; I adjusted these values to have better rounding behavior.

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/c5f6188b4794 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/1218c119a5d4

https://hg.mozilla.org/mozilla-central/rev/c5f6188b4794 https://hg.mozilla.org/mozilla-central/rev/1218c119a5d4

Comment on attachment 8439260 [details] [diff] [review] Part 1: Flatten nsDisplayOpacity together Approval Request Comment [Feature/regressing bug #]: In this bug only. This is not a regression. [User impact if declined]: See bug Comment 104. This saves two fullscreen buffer on the b2g homescreen and will save memory for apps that use css opacity in some very specific ways. [Describe test coverage new/current, TBPL]: Has been on central for over a month. This change is enabled on all platforms and has integration coverage. [Risks and why]: Even given all the exposure of this change there's still some risk. The alternative is we can remove the opacity effect on the gaia homescreen to get the memory back without this change and make a lower risk gaia said change. Of course this means we can't get the desired effect for v2.0. [String/UUID change made/needed]: None I didn't request an uplift before because I didn't think it would make such a difference.

Comment on attachment 8440308 [details] [diff] [review] Part 2: Flatten nsDisplayBackgroundColor v3 (Fixed invalidation) Approval Request Comment Same as part 1

Comment on attachment 8439260 [details] [diff] [review] Part 1: Flatten nsDisplayOpacity together Wrong approval

Requesting blocking per Bug 1038884 Comment 104.

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/fee2d2f0ece5 https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/a94ff8d9f9af