Closed Bug 788877 Opened 8 years ago Closed 8 years ago

GMail inbox get rendered as component alpha layer

Categories

(Core :: Web Painting, defect)

16 Branch
x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18
Tracking Status
firefox16 --- fixed
firefox17 --- fixed

People

(Reporter: bas.schouten, Assigned: roc)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

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.
Component: Layout → Layout: View Rendering
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.
Maybe treat root scroll frames that have overflow: hidden like any other scroll frame? Active only when scrolled.
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); >]
Attached patch fix (obsolete) — Splinter Review
This should help, at least when you're not scrolling.
Assignee: nobody → roc
Attachment #660705 - Flags: review?(matspal)
(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 on attachment 660705 [details] [diff] [review]
fix

Oops, this doesn't help.
Attachment #660705 - Attachment is obsolete: true
Attachment #660705 - Flags: review?(matspal)
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.
Attached patch fix (obsolete) — Splinter Review
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.
Attachment #660714 - Flags: review?(chrislord.net)
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.
Looks good to me, but I want to try it out on mobile first... Doing so now.
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.
Attached patch fix v2Splinter Review
It was not intentional, good catch!
Attachment #660714 - Attachment is obsolete: true
Attachment #660714 - Flags: review?(chrislord.net)
Attachment #661053 - Flags: review?(chrislord.net)
Comment on attachment 661053 [details] [diff] [review]
fix v2

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

Looks good to me.
Attachment #661053 - Flags: review?(chrislord.net) → review+
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.
Attachment #661053 - Flags: approval-mozilla-beta?
Attachment #661053 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/5c68c842a1fc
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Triage drive-by, will wait until this has been on Nightly a bit longer before approving.
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.
Attachment #661053 - Flags: approval-mozilla-beta?
Attachment #661053 - Flags: approval-mozilla-beta+
Attachment #661053 - Flags: approval-mozilla-aurora?
Attachment #661053 - Flags: approval-mozilla-aurora+
Keywords: regression
This is a regression from bug 758620.
Blocks: 758620
Version: unspecified → 16 Branch
Depends on: 934705
Component: Layout: View Rendering → Layout: Web Painting
You need to log in before you can comment on or make changes to this bug.