Last Comment Bug 642246 - Don't build ThebesLayers for elements with no displayports
: Don't build ThebesLayers for elements with no displayports
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Benjamin Stover (:stechz)
:
Mentors:
Depends on: 642205 649583
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-16 12:57 PDT by Benjamin Stover (:stechz)
Modified: 2011-04-13 16:22 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Don't build ThebesLayers for elements with no displayports (8.36 KB, patch)
2011-03-16 13:04 PDT, Benjamin Stover (:stechz)
no flags Details | Diff | Splinter Review
Don't build ThebesLayers for elements with no displayports (13.38 KB, patch)
2011-04-07 12:41 PDT, Benjamin Stover (:stechz)
roc: review+
roc: feedback+
Details | Diff | Splinter Review

Description Benjamin Stover (:stechz) 2011-03-16 12:57:17 PDT
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.
Comment 1 Benjamin Stover (:stechz) 2011-03-16 13:04:44 PDT
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?
Comment 2 Benjamin Stover (:stechz) 2011-03-16 13:06:25 PDT
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?
Comment 3 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-16 14:29:36 PDT
(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.
Comment 4 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-16 14:45:45 PDT
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?
Comment 5 Benjamin Stover (:stechz) 2011-03-16 14:56:08 PDT
(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.
Comment 6 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-16 16:20:29 PDT
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.
Comment 7 Benjamin Stover (:stechz) 2011-04-07 12:41:18 PDT
Created attachment 524452 [details] [diff] [review]
Don't build ThebesLayers for elements with no displayports
Comment 8 Benjamin Stover (:stechz) 2011-04-07 12:44:53 PDT
(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?
Comment 9 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-08 10:29:56 PDT
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?
Comment 10 Benjamin Stover (:stechz) 2011-04-08 10:53:26 PDT
(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?
Comment 11 Benjamin Stover (:stechz) 2011-04-08 12:45:14 PDT
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?
Comment 12 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-08 14:25:21 PDT
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)"?
Comment 13 Benjamin Stover (:stechz) 2011-04-08 14:57:22 PDT
Try server: http://tbpl.mozilla.org/?tree=MozillaTry&rev=e6347821d785
Comment 14 Benjamin Stover (:stechz) 2011-04-09 12:45:57 PDT
Pushed http://hg.mozilla.org/mozilla-central/rev/1edfae12015b
Comment 15 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-04-09 18:27:05 PDT
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
Comment 16 Matt Brubeck (:mbrubeck) 2011-04-13 13:52:31 PDT
Re-landed on 2011-04-11: http://hg.mozilla.org/mozilla-central/rev/ede9a23ec516

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