Closed
Bug 590468
Opened 14 years ago
Closed 14 years ago
Reduce size of chrome document layer due to status bar
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
People
(Reporter: roc, Assigned: roc)
References
Details
(Keywords: dev-doc-needed)
Attachments
(6 files, 8 obsolete files)
3.05 KB,
patch
|
tnikkel
:
review+
vlad
:
superreview+
|
Details | Diff | Splinter Review |
1.04 KB,
patch
|
dao
:
review+
|
Details | Diff | Splinter Review |
810 bytes,
patch
|
jaas
:
review+
|
Details | Diff | Splinter Review |
1.17 KB,
patch
|
jimm
:
review+
|
Details | Diff | Splinter Review |
14.81 KB,
patch
|
Details | Diff | Splinter Review | |
938 bytes,
patch
|
Details | Diff | Splinter Review |
Here's a typical layer tree with bug 130078 (and bug 574621 fixed): BasicLayerManager (0x1edb9340) BasicContainerLayer (0x1edb97a0) [visible=< (x=0, y=0, w=994, h=996); >] [opaqueContent] // ROOT LAYER BasicThebesLayer (0x1edb98a0) [visible=< (x=0, y=0, w=994, h=996); >] [opaqueContent] [valid=< (x=0, y=0, w=994, h=996); >] // CHROME BasicContainerLayer (0x1a72edb0) [clip=(x=0, y=63, w=994, h=915)] [visible=< (x=0, y=63, w=994, h=915); >] [opaqueContent] // CONTENT DOCUMENT BasicContainerLayer (0x1a72eed0) [visible=< (x=979, y=63, w=15, h=915); >] [opaqueContent] // SCROLLBAR BasicThebesLayer (0x1a72efc0) [visible=< (x=979, y=63, w=15, h=915); >] [opaqueContent] [valid=< (x=979, y=63, w=15, h=915); >] // CONTENTS OF SCROLLBAR BasicThebesLayer (0x1a72f120) [transform=[ 1 0; 0 1; 0 63; ]] [visible=< (x=0, y=0, w=979, h=915); >] [opaqueContent] [valid=< (x=0, y=0, w=979, h=915); >] // WEB PAGE The visible rect of the CHROME layer is the size of the whole window, because it's the bounding box of the chrome at the top of the window and the status bar at the bottom of the window. This has two undesirable effects: 1) we allocate a buffer the size of the whole window even though most of it is completely hidden by the Web page (this problem existed before bug 130078, since retained layers landed). 2) MayHaveOverlappingOrTransparentLayers is going to return true all the time, since the Web page layer overlays the chrome layer. This means on GDI and X, we will always have to double-buffer to avoid flickering as we draw the chrome layer, then draw the content layer on top. This is new with bug 130078. This may be the cause of performance regressions. We could try to use heuristics to avoid this problem automatically, by passing in a complex visible region and detecting that it's much smaller than its bounding box, and then doing some trickery in ThebesLayer implementations to allocate multiple buffers for different parts of the layer. But let's not do that right now. Instead, I think we should just define a new XUL attribute or CSS property that forces an element into its own layer, and apply that to the status bar. Probably a XUL attribute since that's lighter-weight to implement and we don't want Web authors to go near it anyway.
Assignee | ||
Comment 1•14 years ago
|
||
The 'layer' attribute forces all the descendant display items into a single list (so the element becomes like a CSS stacking context), and creates a container layer for the items. Typically this means that all the items will get their own ThebesLayer. This is a feature that we do not want to use much at all.
Attachment #469017 -
Flags: superreview?(vladimir)
Attachment #469017 -
Flags: review?(tnikkel)
Assignee | ||
Comment 2•14 years ago
|
||
This creates a layer for the statusbar contents, addressing the performance issues described in this bug.
Attachment #469018 -
Flags: review?
Assignee | ||
Updated•14 years ago
|
Attachment #469018 -
Flags: review? → review?(dao)
Assignee | ||
Comment 3•14 years ago
|
||
We know this, because we paint them.
Attachment #469019 -
Flags: review?(joshmoz)
Assignee | ||
Comment 4•14 years ago
|
||
I guess we don't know this for sure. But it had better be true! If it's not true, we should paint a background for those widgets so it is true.
Attachment #469020 -
Flags: review?(jmathies)
Comment 5•14 years ago
|
||
Comment on attachment 469018 [details] [diff] [review] Part 2: mark browser status bar with 'layer' attribute What about the sidebar?
Attachment #469018 -
Flags: review?(dao) → review+
Assignee | ||
Comment 6•14 years ago
|
||
With those patches, on Mac I get BasicLayerManager (0x1ebe34f0) BasicContainerLayer (0x1ebe3b70) [visible=< (x=0, y=0, w=994, h=996); >] [opaqueContent] // ROOT LAYER BasicThebesLayer (0x1ebe3c70) [visible=< (x=0, y=0, w=994, h=63); >] [opaqueContent] [valid=< (x=0, y=0, w=994, h=3); (x=0, y=3, w=994, h=60); >] // CHROME BasicContainerLayer (0x2090f860) [clip=(x=0, y=63, w=994, h=915)] [visible=< (x=0, y=63, w=994, h=915); >] [opaqueContent] // CONTENT DOCUMENT BasicContainerLayer (0x20932470) [visible=< (x=979, y=63, w=15, h=915); >] [opaqueContent] // SCROLLBAR BasicThebesLayer (0x208d4e30) [visible=< (x=979, y=63, w=15, h=915); >] [opaqueContent] [valid=< (x=979, y=63, w=15, h=915); >] // SCROLLBAR CONTENTS BasicThebesLayer (0x2090e000) [transform=[ 1 0; 0 1; 0 63; ]] [visible=< (x=0, y=0, w=979, h=915); >] [opaqueContent] [valid=< (x=0, y=0, w=979, h=915); >] // WEB PAGE BasicContainerLayer (0x1ebe4670) [clip=(x=0, y=0, w=994, h=996)] [visible=< (x=0, y=978, w=994, h=18); >] // STATUSBAR BasicThebesLayer (0x1ebe4840) [visible=< (x=0, y=978, w=994, h=18); >] [opaqueContent] [valid=< (x=0, y=978, w=994, h=18); >] // STATUSBAR CONTENTS Note in particular that the CHROME layer is now only 63 pixels high. Also, no sibling layers intersect. However, we'd still double-buffer (if layer-based double-buffering was enabled on Mac) since the STATUSBAR layer is not marked opaque. I need to look into that tomorrow. Another remaining issue is GTK2. nsNativeThemeGTK2 says it supports NS_THEME_STATUSBAR but doesn't paint anything for it. I don't know how statusbars are painted on GTK2 or even if they're opaque.
Assignee | ||
Comment 7•14 years ago
|
||
Good question. I haven't looked into that yet. We should probably do something similar for sidebars, but the main priority right now is to fix basic browsing so 130078 can land.
Comment on attachment 469017 [details] [diff] [review] Part 1: Add support for XUL 'layer' attribute Yeah, I was going to ask "what about the sidebar?" too, but I don't see why this fix wouldn't work for sidebars as well.
Attachment #469017 -
Flags: superreview?(vladimir) → superreview+
Updated•14 years ago
|
Attachment #469020 -
Flags: review?(jmathies) → review+
Attachment #469019 -
Flags: review?(joshmoz) → review+
Updated•14 years ago
|
Attachment #469017 -
Flags: review?(tnikkel) → review+
Assignee | ||
Comment 10•14 years ago
|
||
Ah right, I'm glad I'd filed it. I should have marked it blocking 130078 though.
Keywords: checkin-needed
Whiteboard: [needs landing]
Comment 11•14 years ago
|
||
Bug 579263 is marked blocking 130078 :).
Comment 12•14 years ago
|
||
Attachment #469329 -
Flags: review?(dao)
Comment 13•14 years ago
|
||
Comment on attachment 469329 [details] [diff] [review] part 5: draw window background into browser-bottombox to make it opaque (GTK) A side-effect of this is that it will change the origin of any tiled theme backgrounds for the bottombox from the origin of the window to the origin of the bottombox. I think that's OK. It's what we do with menubars and tabbars and I haven't heard of problems due to their tile-origin not matching up the tab panel (which uses the window origin IIUC). It looks like this wouldn't help on other platforms: nsNativeThemeWin::ThemeSupportsWidget does not support NS_THEME_WINDOW. nsNativeThemeCocoa::DrawWidgetBackground does not draw anything for NS_THEME_WINDOW even though nsNativeThemeCocoa::ThemeSupportsWidget says support is provided. Perhaps dialog would be OK on Mac? (It would also be fine with GTK.)
Assignee | ||
Comment 14•14 years ago
|
||
This fixes the last problem here. nsDisplayWrapList::IsOpaque was always returning PR_FALSE, and even if we fix that, nsDisplayList::IsOpaque was returning false anyway, because nsDisplayList::ComputeVisibility wasn't considering that aVisibleRegion might contain stuff that doesn't intersect the contents of the list. With this patch, and 130078, on simple pages we end up with a set of layers that are all marked opaque and where no sibling layers overlap, so backbuffer avoidance will be enabled.
Attachment #469335 -
Flags: review?(tnikkel)
Assignee | ||
Comment 15•14 years ago
|
||
Try this
Attachment #469335 -
Attachment is obsolete: true
Attachment #469344 -
Flags: review?(tnikkel)
Attachment #469335 -
Flags: review?(tnikkel)
Assignee | ||
Comment 17•14 years ago
|
||
Fixed nsDisplayZoom::ComputeVisibility.
Attachment #469344 -
Attachment is obsolete: true
Attachment #469345 -
Attachment is obsolete: true
Attachment #469351 -
Flags: review?(tnikkel)
Attachment #469344 -
Flags: review?(tnikkel)
Attachment #469345 -
Flags: review?(tnikkel)
Comment 18•14 years ago
|
||
The assertion "bad aListVisibleBounds: 'r.GetBounds() == aListVisibleBounds'" in part 6 gets hits a bunch of times on crashtests and reftests on try server.
Comment 19•14 years ago
|
||
(In reply to comment #13) > Comment on attachment 469329 [details] [diff] [review] > part 5: draw window background into browser-bottombox to make it opaque (GTK) > > A side-effect of this is that it will change the origin of any tiled theme > backgrounds for the bottombox from the origin of the window to the origin of > the bottombox. I think that's OK. Doesn't sound right to me. > It's what we do with menubars and tabbars I'm not sure what this means. We certainly don't use -moz-appearance: window for the menu bar or tab bar.
Comment 20•14 years ago
|
||
(In reply to comment #19) > > It's what we do with menubars and tabbars Sorry, I accidentally wrote tabbar when I meant toolbar, here. > I'm not sure what this means. We certainly don't use -moz-appearance: window > for the menu bar or tab bar. "window" backgrounds are painted with only gtk_style_apply_default_background(). That is also used for the -moz-appearance menubar and toolbar, though they may also paint other things, shadow borders or whatever. What I meant was that the tab bar uses the underlying window background with background origin at the origin of the window, while the backgrounds painted with menubar and toolbar have the background origin at the origin of the widget. I might be able to modify -moz-appearance statusbar to add a background (even though GtkFrame, used for statusbar, is usually transparent) only when the background is not tiled, and so statusbar would be opaque only when the background is not tiled. To me, however, trying to align tiling in the tab bar with tiling in the status bar didn't seem worth the trouble, given how far apart they are, and that the bottombox looks like a separate panel.
Comment 21•14 years ago
|
||
Imagine changing the height of the window by dragging the resize bar at the bottom of the window. Before this patch, the tiles in the bottombox would appear to stay fixed while the frames, buttons, etc. moved. After this patch, tiles in the bottombox would appear to move with the bottombox elements.
Comment 22•14 years ago
|
||
(In reply to comment #20) > "window" backgrounds are painted with only > gtk_style_apply_default_background(). Could this paint fancy gradients that make only sense with the top of the window as the origin? > That is also used for the -moz-appearance menubar and toolbar, though they may > also paint other things, shadow borders or whatever. Exactly, they don't just repeat the window appearance, they may in fact completely cover it as I understand it. Also, does the :not([lwthemefooter="true"]) part mean that [lwthemefooter="true"] is going to cause perf problems? This seems flawed.
Comment 23•14 years ago
|
||
(In reply to comment #22) > (In reply to comment #20) > > "window" backgrounds are painted with only > > gtk_style_apply_default_background(). > > Could this paint fancy gradients that make only sense with the top of the > window as the origin? Yes, good point. That is feasible. Though the possibilities are limited. No scaling is available, and the image is always tiled; it would have to be done with a large pixmap, large enough that it would always be larger than any window, which would be unfortunately inefficient. > > That is also used for the -moz-appearance menubar and toolbar, though they may > > also paint other things, shadow borders or whatever. > > Exactly, they don't just repeat the window appearance, they may in fact > completely cover it as I understand it. May or may not, yes. > Also, does the :not([lwthemefooter="true"]) part mean that > [lwthemefooter="true"] is going to cause perf problems? This seems flawed. Yes, it wouldn't perform as well.
Comment 24•14 years ago
|
||
(In reply to comment #23) > > > "window" backgrounds are painted with only > > > gtk_style_apply_default_background(). > > > > Could this paint fancy gradients that make only sense with the top of the > > window as the origin? > > Yes, good point. That is feasible. Though the possibilities are limited. No > scaling is available, and the image is always tiled; it would have to be done > with a large pixmap, large enough that it would always be larger than any > window, which would be unfortunately inefficient. It could be high and narrow, which doesn't seem too odd to me. > > Also, does the :not([lwthemefooter="true"]) part mean that > > [lwthemefooter="true"] is going to cause perf problems? This seems flawed. > > Yes, it wouldn't perform as well. Is there a plan for addressing this? Seems like we should care about this, despite talos not measuring it.
Comment 25•14 years ago
|
||
(In reply to comment #24) > > > Also, does the :not([lwthemefooter="true"]) part mean that > > > [lwthemefooter="true"] is going to cause perf problems? This seems flawed. > > > > Yes, it wouldn't perform as well. > > Is there a plan for addressing this? Could this be addressed by having LightweightThemeConsumer.jsm also set a background-color on .browser-bottombox, not only a background-image? As far as I understand it, all we need is .browser-bottombox to be (detectably) opaque.
Comment 26•14 years ago
|
||
What I really meant is, is there a plan to better handle non-opaque areas surrounding the content area? Seems like an add-on could easily trigger the slow path as well... Letting front-end code working around it everywhere doesn't scale.
Comment 27•14 years ago
|
||
s/Letting front-end code working around it/Letting front-end code work around it/
Assignee | ||
Comment 28•14 years ago
|
||
(In reply to comment #26) > What I really meant is, is there a plan to better handle non-opaque areas > surrounding the content area? There are optimizations we could do, but they'll add complexity and introduce their own performance tradeoffs. -- Maximum memory saved is a buffer the size of the content area, per window. -- Maximum performance saving is few % in Tp4, but practically none if you have hardware acceleration --- D2D/D3D/GL -- Using tiling or multiple buffers per ThebesLayer would introduce their own drawing, compositing and memory costs to offset any gains So it's not clear that those optimizations are worth having, if we can make some easy tweaks so that those optimizations only matter for a relatively small set of browser configurations. My advice for personas is to paint the persona image in the browser-bottombox with background-attachment:fixed so it appears at the right offset. (Though I don't know much about personas, so this could be wrong.) If you do that, and the persona image is opaque, we'll know the browser-bottombox is opaque. Karl made a good suggestion to deal with the GTK issues which I think would be worth doing. Let's define a new -moz-appearance value, say window-color, which is the same as 'window' if it's an opaque color, otherwise paints nothing. Then we can just set window-color on browser-bottombox. For most themes, we will then have an opaque background there. A theme that paints a large tiled image is already imposing a memory and performance hit so having us eat a little bit more is acceptable IMHO (and it really is a little bit --- the "slow path" is not a dive off a cliff here).
Assignee | ||
Comment 29•14 years ago
|
||
-- Changed nsDisplayList::ComputeVisibilityForRoot to compute aListVisibleBounds exactly the way the assertion is going to check it. This is necessary because various callers of ComputeVisibilityForRoot pass in visible regions that are much bigger than the union of the bounds of the display items ... for example drawWindow can do this, and there's nothing wrong with drawWindow doing that. -- nsDisplayTransform::ComputeVisibility needed to be fixed to call RecomputeVisibility on its contained nsDisplayWrapList, so the mVisibleRect of that item is set correctly. -- nsDisplaySVGEffects::ComputeVisibility needed to be fixed call ComputeVisibilityForSublist, passing an appropriate aListVisibleBounds. With those fixes, we get through reftests on Mac without asserting.
Attachment #469780 -
Flags: review?(tnikkel)
Updated•14 years ago
|
Attachment #469351 -
Attachment is obsolete: true
Attachment #469351 -
Flags: review?(tnikkel)
Comment 30•14 years ago
|
||
Comment on attachment 469329 [details] [diff] [review] part 5: draw window background into browser-bottombox to make it opaque (GTK) >+#browser-bottombox:not([lwthemefooter="true"]) { >+ /* opaque for layers optimization */ >+ -moz-appearance: window; I suggest using background-color: -moz-Dialog instead. Please file follow-up bugs on the side bar and :not([lwthemefooter="true"]).
Attachment #469329 -
Flags: review?(dao) → review+
Comment 31•14 years ago
|
||
Not sure about the -moz- before window-color. Kind of playing along here, I don't grasp why some attributes have this but not others.
Attachment #469795 -
Flags: review?
Updated•14 years ago
|
Attachment #469795 -
Flags: review? → review?(roc)
Comment 32•14 years ago
|
||
This version only draws untiled backgrounds. Tiled backgrounds will not be helped/affected by this. This probably could be extended to tiled backgrounds, with more complexity, by the native theme code measuring the offset from the frame to the root frame, but roc pointed out there would also need to be additional code to invalidate the frame should it ever be scrolled. It's still a front-end work-around.
Attachment #469329 -
Attachment is obsolete: true
Attachment #469798 -
Flags: review?
Updated•14 years ago
|
Attachment #469798 -
Flags: review? → review?(dao)
Comment 33•14 years ago
|
||
(In reply to comment #30) > >+#browser-bottombox:not([lwthemefooter="true"]) { > >+ /* opaque for layers optimization */ > >+ -moz-appearance: window; > > I suggest using background-color: -moz-Dialog instead. Sorry, saw this as I was attaching the new patches. -moz-dialog will never be tiled, but if we are happy not having tiled backgrounds for the bottom-box, then that's a much simpler solution. (Some of the side-bar uses background-color, fwiw.) Thoughts? > Please file follow-up bugs on the side bar and :not([lwthemefooter="true"]). OK.
Comment 34•14 years ago
|
||
Comment on attachment 469780 [details] [diff] [review] Part 6 v5 + // nsDisplayItem::ComputeVisiblity should only be called from ComputeVisibility The comments for ComputeVisibilityForSublist and ComputeVisibilityForRoot in nsDisplayList.h seem to be mixed up.
Attachment #469780 -
Flags: review?(tnikkel) → review+
Comment 35•14 years ago
|
||
(In reply to comment #33) > -moz-dialog will never be tiled, but if we are happy not having tiled > backgrounds for the bottom-box, then that's a much simpler solution. I think we are...
Assignee | ||
Comment 36•14 years ago
|
||
Let's go with the original part 5 then.
Assignee | ||
Comment 37•14 years ago
|
||
Comments fixed. Should be ready to check in.
Attachment #469780 -
Attachment is obsolete: true
Updated•14 years ago
|
Attachment #469795 -
Attachment is obsolete: true
Attachment #469795 -
Flags: review?(roc)
Updated•14 years ago
|
Attachment #469798 -
Attachment is obsolete: true
Attachment #469798 -
Flags: review?(dao)
Comment 38•14 years ago
|
||
Updated•14 years ago
|
Keywords: dev-doc-needed
Updated•14 years ago
|
Keywords: checkin-needed
Whiteboard: [needs landing]
Comment 39•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/6f2158e6ac2d http://hg.mozilla.org/mozilla-central/rev/60a5f8010d91 http://hg.mozilla.org/mozilla-central/rev/8710a458ac69 http://hg.mozilla.org/mozilla-central/rev/3c4fcfc05f4b http://hg.mozilla.org/mozilla-central/rev/28bf7628f0f1 http://hg.mozilla.org/mozilla-central/rev/4722b491cd0d
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Comment 40•14 years ago
|
||
(In reply to comment #30) > Please file follow-up bugs on the side bar and :not([lwthemefooter="true"]). Bug 591855 and bug 591557.
Comment 41•14 years ago
|
||
Er, i mean bug 591855 and bug 591853.
Comment 42•14 years ago
|
||
I don't think we want to document the layer attribute. It's just meant for a backend optimization and would likely be broken for general use.
Comment 43•14 years ago
|
||
(In reply to comment #42) > I don't think we want to document the layer attribute. It's just meant for a > backend optimization and would likely be broken for general use. I agree; I don't think this ought to be documented. However, it seems to me we should document the benefits of marking things as opaque using -moz-Dialog when building themes. Yes?
Comment 44•14 years ago
|
||
Between comment 42, the lack of response to comment 43, and the fact that the status bar is gone anyway, I'm removing the doc needed keyword here.
Keywords: dev-doc-needed
Comment 45•12 years ago
|
||
The "layer" attribute isn't documented on MDN AFAICT (see comment 0 and comment 1).
Keywords: dev-doc-needed
Comment 46•12 years ago
|
||
That may be intentional. ie (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #1) > This is a feature that we do not want to use much at all.
Comment 47•12 years ago
|
||
Not having it documented means people will copy it from existing code without understanding that it shouldn't be used :)
You need to log in
before you can comment on or make changes to this bug.
Description
•