Last Comment Bug 788877 - GMail inbox get rendered as component alpha layer
: GMail inbox get rendered as component alpha layer
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: Layout: View Rendering (show other bugs)
: 16 Branch
: x86_64 Windows 7
: -- normal (vote)
: mozilla18
Assigned To: Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
:
Mentors:
Depends on: 934705
Blocks: tabswitch 758620
  Show dependency treegraph
 
Reported: 2012-09-05 16:06 PDT by Bas Schouten (:bas.schouten)
Modified: 2013-11-04 20:54 PST (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed


Attachments
fix (1.48 KB, patch)
2012-09-12 21:30 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details | Diff | Review
fix (2.26 KB, patch)
2012-09-12 22:10 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details | Diff | Review
fix v2 (2.30 KB, patch)
2012-09-13 17:02 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
chrislord.net: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Review

Description Bas Schouten (:bas.schouten) 2012-09-05 16:06:03 PDT
When playing around with my new 2D gfx recording and replaying tools I noticed gmail is rendered as a single, large, component alpha layer. This will be very expensive particularly when software rendering with D3D9 acceleration (or on Mac for example). What seems to be the transparent part of the page in the recording is the folder list on the left and the headers on the top. The rest appears to actually be opaque.
Comment 1 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-05 22:43:14 PDT
We can probably fix this quite easily by using a heuristic to make the GMail page's root scroll frame non-active, since it can never be scrolled.
Comment 2 Timothy Nikkel (:tnikkel) 2012-09-06 12:04:20 PDT
Maybe treat root scroll frames that have overflow: hidden like any other scroll frame? Active only when scrolled.
Comment 3 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-06 18:00:13 PDT
Yeah.
Comment 4 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-12 21:24:09 PDT
FWIW I don't see this on D3D10 in my GMail configuration. I see just one component alpha layer, and it's not very big:
D3D10ThebesLayer (0x1b1bec00) [transform=[ 1 0; 0 1; 1 123; ]] [visible=< (x=0, y=0, w=1229, h=15); (x=0, y=15, w=1229, h=87); (x=0, y=116, w=1229, h=567); >] [componentAlpha] [isFixedPosition] [valid=< (x=0, y=0, w=1229, h=15); (x=0, y=15, w=1229, h=87); (x=0, y=116, w=1229, h=567); >]
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-12 21:30:39 PDT
Created attachment 660705 [details] [diff] [review]
fix

This should help, at least when you're not scrolling.
Comment 6 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-12 21:31:15 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #4)
> FWIW I don't see this on D3D10 in my GMail configuration. I see just one
> component alpha layer, and it's not very big:
> D3D10ThebesLayer (0x1b1bec00) [transform=[ 1 0; 0 1; 1 123; ]] [visible=<
> (x=0, y=0, w=1229, h=15); (x=0, y=15, w=1229, h=87); (x=0, y=116, w=1229,
> h=567); >] [componentAlpha] [isFixedPosition] [valid=< (x=0, y=0, w=1229,
> h=15); (x=0, y=15, w=1229, h=87); (x=0, y=116, w=1229, h=567); >]

This comment is, of course, completely wrong. That layer is plenty big.
Comment 7 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-12 21:36:08 PDT
Comment on attachment 660705 [details] [diff] [review]
fix

Oops, this doesn't help.
Comment 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-12 21:56:19 PDT
GMail's inner IFRAME has a white background on the body, which isn't a problem, but it also has a DIV (class="wl" in my instance) which is position:fixed; height:100%; width:100%; background:white ... and no content. I'm not sure what it's supposed to be for, but we give it its own layer and then we have to draw the rest of the content on top with component alpha.
Comment 9 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-12 22:10:14 PDT
Created attachment 660714 [details] [diff] [review]
fix

This fixes it.

Note that the root scroll frame for the root content document normally is active. So this only affects subframes that aren't actively being scrolled. In such a case the page content's root active scrollframe is the viewport (or higher) anyway, so putting fixed position content in its own layer is pointless.
Comment 10 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-12 22:16:19 PDT
The good news is, with this patch, even when we scroll (in the main view anyway, I didn't test the others), we don't get component-alpha layers. In fact, the layer setup is entirely sane:

    D3D10ContainerLayer (0x8523540) [clip=(x=1, y=123, w=1229, h=683)] [visible=< (x=1, y=123, w=1229, h=683); >] [opaqueContent] [isFixedPosition]
      D3D10ThebesLayer (0x8523700) [transform=[ 1 0; 0 1; 1 123; ]] [visible=< (x=0, y=0, w=1229, h=683); >] [opaqueContent] [isFixedPosition] [valid=< (x=0, y=0, w=1229, h=683); >]
      D3D10ThebesLayer (0x18e45480) [transform=[ 1 0; 0 1; 205 -85; ]] [visible=< (x=0, y=368, w=991, h=523); >] [opaqueContent] [isFixedPosition] [valid=< (x=0, y=368, w=991, h=523); >]

This is because the scrollable pane has its own background:white so the scrollable content is not transparent.
Comment 11 Chris Lord [:cwiiis] 2012-09-13 02:16:35 PDT
Looks good to me, but I want to try it out on mobile first... Doing so now.
Comment 12 Chris Lord [:cwiiis] 2012-09-13 02:47:25 PDT
Comment on attachment 660714 [details] [diff] [review]
fix

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

::: layout/generic/nsFrame.cpp
@@ +2160,5 @@
> +  // Don't build an nsDisplayFixedPosition if our root scroll frame is not
> +  // active, that's pointless and the extra layer(s) created may be wasteful.
> +  bool buildFixedPositionItem = disp->mPosition == NS_STYLE_POSITION_FIXED &&
> +    !child->GetParent()->GetParent() && !isSVG &&
> +    IsRootScrollFrameActive(PresContext()->PresShell());

Why is the IsInFixedPosition() check removed? If that's purposeful, the comment above needs to be altered. If it's not purposeful, I'd suggest moving the !isSVG check to be last, as it's likely to always be true.

Will r+ with this clarified.
Comment 13 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-13 17:02:47 PDT
Created attachment 661053 [details] [diff] [review]
fix v2

It was not intentional, good catch!
Comment 14 Chris Lord [:cwiiis] 2012-09-14 01:31:44 PDT
Comment on attachment 661053 [details] [diff] [review]
fix v2

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

Looks good to me.
Comment 15 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-16 22:46:31 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c68c842a1fc
Comment 16 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-17 02:37:28 PDT
Comment on attachment 661053 [details] [diff] [review]
fix v2

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

We need this patch to fix a regression in GMail rendering performance in aurora/beta. The patch is simple and safe; it just turns off nsDisplayFixedPosition in more cases.
Comment 17 Ed Morley [:emorley] 2012-09-17 12:25:50 PDT
https://hg.mozilla.org/mozilla-central/rev/5c68c842a1fc
Comment 18 Lukas Blakk [:lsblakk] use ?needinfo 2012-09-17 15:19:30 PDT
Triage drive-by, will wait until this has been on Nightly a bit longer before approving.
Comment 19 Alex Keybl [:akeybl] 2012-09-18 12:02:04 PDT
Comment on attachment 661053 [details] [diff] [review]
fix v2

[Triage Comment]
Approving for Aurora 17 regardless.

This a+ for Beta 16 is *only* if this is a new regression (FF15 or 16) given where we are in the cycle (please include a regressing bug # if so). Decided to approve instead of ask the question to speed the process up.
Comment 20 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-18 17:13:18 PDT
This is a regression from bug 758620.
Comment 21 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-18 20:03:15 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/a06f7b317f78

https://hg.mozilla.org/releases/mozilla-beta/rev/a0213afa1514

Note You need to log in before you can comment on or make changes to this bug.