Closed
Bug 1024148
Opened 10 years ago
Closed 10 years ago
Flatten Opacity and BackgroundColor display items
Categories
(Core :: Layout, defect)
Tracking
()
People
(Reporter: BenWa, Assigned: BenWa)
References
Details
Attachments
(3 files, 4 obsolete files)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
BenWa
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mattwoodrow
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•10 years ago
|
Attachment #8438768 -
Flags: review?(matt.woodrow) → review+
Assignee | ||
Comment 3•10 years ago
|
||
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.
Attachment #8439260 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Attachment #8438768 -
Attachment is obsolete: true
Assignee | ||
Comment 4•10 years ago
|
||
Note that the error can accumulate every time we merge.
Attachment #8439279 -
Flags: review?(matt.woodrow)
Assignee | ||
Comment 5•10 years ago
|
||
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
Updated•10 years ago
|
Attachment #8439279 -
Flags: review?(matt.woodrow) → review+
Assignee | ||
Comment 7•10 years ago
|
||
Address try failure, non trivial changes so re-requesting review: https://tbpl.mozilla.org/?tree=Try&rev=a8f5cf586813
Attachment #8439279 -
Attachment is obsolete: true
Attachment #8439489 -
Flags: review?(matt.woodrow)
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8439489 -
Attachment is obsolete: true
Attachment #8439489 -
Flags: review?(matt.woodrow)
Attachment #8439490 -
Flags: review?(matt.woodrow)
Updated•10 years ago
|
Attachment #8439490 -
Flags: review?(matt.woodrow) → review+
Comment 9•10 years ago
|
||
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?
Comment 10•10 years ago
|
||
(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?
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(tnikkel)
Comment 11•10 years ago
|
||
(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.
Flags: needinfo?(tnikkel)
Comment 12•10 years ago
|
||
Let's track what kinds of results we get here and consider this for Aurora (32) uplift.
Assignee | ||
Comment 13•10 years ago
|
||
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.
Assignee | ||
Comment 14•10 years ago
|
||
Previous run is looking surprising good. Pushing a full try run to be safe: https://tbpl.mozilla.org/?tree=Try&rev=670f96615b89
Assignee | ||
Comment 16•10 years ago
|
||
All done!
Attachment #8439490 -
Attachment is obsolete: true
Attachment #8440308 -
Flags: review?(matt.woodrow)
Assignee | ||
Comment 17•10 years ago
|
||
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.
Updated•10 years ago
|
Attachment #8440308 -
Flags: review?(matt.woodrow) → review+
Assignee | ||
Comment 18•10 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/c5f6188b4794 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/1218c119a5d4
Comment 19•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c5f6188b4794 https://hg.mozilla.org/mozilla-central/rev/1218c119a5d4
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Assignee | ||
Comment 20•10 years ago
|
||
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.
Attachment #8439260 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 21•10 years ago
|
||
Comment on attachment 8440308 [details] [diff] [review] Part 2: Flatten nsDisplayBackgroundColor v3 (Fixed invalidation) Approval Request Comment Same as part 1
Attachment #8440308 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 22•10 years ago
|
||
Comment on attachment 8439260 [details] [diff] [review] Part 1: Flatten nsDisplayOpacity together Wrong approval
Attachment #8439260 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•10 years ago
|
Attachment #8440308 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 23•10 years ago
|
||
Requesting blocking per Bug 1038884 Comment 104.
blocking-b2g: --- → 2.0?
Updated•10 years ago
|
blocking-b2g: 2.0? → 2.0+
Updated•10 years ago
|
Updated•10 years ago
|
status-b2g-v2.0:
--- → affected
status-b2g-v2.1:
--- → fixed
status-firefox31:
--- → wontfix
status-firefox32:
--- → wontfix
status-firefox33:
--- → fixed
Comment 24•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/fee2d2f0ece5 https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/a94ff8d9f9af
You need to log in
before you can comment on or make changes to this bug.
Description
•