Don't build ThebesLayers for elements with no displayports

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: stechz, Assigned: stechz)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

The compositing process needs to know about all scrollable elements, but it is expensive to build ThebesLayers for every one of them. We can save a lot of memory by not building a ThebesLayer for elements without displayports. This means that at first a scroll frame will not be able to scroll asynchronously but as soon as the displayport is updated, it will work fine.
Summary: Allow for nsDisplayScrollLayers to be empty for empty displayports → Don't build ThebesLayers for elements with no displayports
The first step is just to build an empty nsDisplayScrollLayer with no items
inside it. Since we already have a FORCE LAYER flag, it was easy to let the
container layer be built even when nothing was inside of it. This has the
consequence of making ContainerLayers able to have no children, so I had to
add an AsContainerLayer method. Everything else worked magically for Fennec!

Important thing to consider: is there any other code that makes assumptions
about ContainerLayers always having children?
Attachment #519748 - Flags: review?(tnikkel)
Attachment #519748 - Flags: review?(jones.chris.g)
Assignee: nobody → ben
Comment on attachment 519748 [details] [diff] [review]
Don't build ThebesLayers for elements with no displayports

Hm, actually before I ask for reviews. Roc: do you think this approach is a good idea?
Attachment #519748 - Flags: review?(tnikkel)
Attachment #519748 - Flags: review?(jones.chris.g)
Attachment #519748 - Flags: review?
Attachment #519748 - Flags: feedback?(roc)
Attachment #519748 - Flags: review?
Depends on: 642205
(In reply to comment #1)
> Important thing to consider: is there any other code that makes assumptions
> about ContainerLayers always having children?

I hope not. If there is, we should fix it.
This approach looks OK.

+      if (itemVisibleRect.IsEmpty() && layerState != LAYER_ACTIVE_FORCE) {

Do you really want to create a layer even if the scrollframe is completely covered by other opaque content?
(In reply to comment #4)
> This approach looks OK.
> 
> +      if (itemVisibleRect.IsEmpty() && layerState != LAYER_ACTIVE_FORCE) {
> 
> Do you really want to create a layer even if the scrollframe is completely
> covered by other opaque content?

Oh, probably not. I added this because the itemVisibleRect is of course empty with no items inside it.
I see. I think you should arrange for the visible rect of your item to be correctly computed to be the whole scrollable area instead.
Attachment #519748 - Attachment is obsolete: true
Attachment #519748 - Flags: feedback?(roc)
(In reply to comment #6)
> I see. I think you should arrange for the visible rect of your item to be
> correctly computed to be the whole scrollable area instead.

Hm, I'm not sure that's what we want since those display items are going to also be in another layer. The point of this layer is to only have the metadata for the scroll frame.

I've made a new layer type called LAYER_ACTIVE_EMPTY that is explicitly for metadata, so that we have the expectation that its visible rect should always be empty. Now it's easy to tell the difference between an occluded layer and an intentionally empty layer. What do you think?
Attachment #524452 - Flags: feedback?(roc)
Comment on attachment 524452 [details] [diff] [review]
Don't build ThebesLayers for elements with no displayports

I like it!

Do you need to do something to ensure that when a displayport is set on an element, we invalidate it so that the nsDisplayScrollLayer is created?
Attachment #524452 - Flags: feedback?(roc) → feedback+
(In reply to comment #9)
> Comment on attachment 524452 [details] [diff] [review]
> Don't build ThebesLayers for elements with no displayports
> 
> I like it!
> 
> Do you need to do something to ensure that when a displayport is set on an
> element, we invalidate it so that the nsDisplayScrollLayer is created?

We already do that, so we're set there. :) So, time to get a review on this. Maybe Timothy is the best?
Attachment #524452 - Flags: review?(tnikkel)
Comment on attachment 524452 [details] [diff] [review]
Don't build ThebesLayers for elements with no displayports

Actually, roc, you okay with formally reviewing this since you sort of know the patch?
Attachment #524452 - Flags: review?(tnikkel) → review?(roc)
Comment on attachment 524452 [details] [diff] [review]
Don't build ThebesLayers for elements with no displayports

+    if (!usingDisplayport) {

Can you flip this around so that it's "if (usingDisplayport)"?
Attachment #524452 - Flags: review?(roc) → review+
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Backed out due to massive mobile unittest orange following cedar merge (as the most suspicious candidate):
http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=fe3f7889918b
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Re-landed on 2011-04-11: http://hg.mozilla.org/mozilla-central/rev/ede9a23ec516
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Depends on: 649583
You need to log in before you can comment on or make changes to this bug.