The default bug view has changed. See this FAQ.

Don't build ThebesLayers for elements with no displayports

RESOLVED FIXED

Status

()

Core
Graphics
RESOLVED FIXED
6 years ago
6 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)

(Assignee)

Description

6 years ago
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.
(Assignee)

Updated

6 years ago
Summary: Allow for nsDisplayScrollLayers to be empty for empty displayports → Don't build ThebesLayers for elements with no displayports
(Assignee)

Comment 1

6 years ago
Created attachment 519748 [details] [diff] [review]
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)

Updated

6 years ago
Assignee: nobody → ben
(Assignee)

Comment 2

6 years ago
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)
(Assignee)

Updated

6 years ago
Attachment #519748 - Flags: review?
(Assignee)

Updated

6 years ago
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?
(Assignee)

Comment 5

6 years ago
(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.
(Assignee)

Comment 7

6 years ago
Created attachment 524452 [details] [diff] [review]
Don't build ThebesLayers for elements with no displayports
(Assignee)

Updated

6 years ago
Attachment #519748 - Attachment is obsolete: true
Attachment #519748 - Flags: feedback?(roc)
(Assignee)

Comment 8

6 years ago
(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?
(Assignee)

Updated

6 years ago
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+
(Assignee)

Comment 10

6 years ago
(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?
(Assignee)

Updated

6 years ago
Attachment #524452 - Flags: review?(tnikkel)
(Assignee)

Comment 11

6 years ago
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+
(Assignee)

Comment 13

6 years ago
Try server: http://tbpl.mozilla.org/?tree=MozillaTry&rev=e6347821d785
(Assignee)

Updated

6 years ago
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Comment 14

6 years ago
Pushed http://hg.mozilla.org/mozilla-central/rev/1edfae12015b
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
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Depends on: 649583
You need to log in before you can comment on or make changes to this bug.