Closed Bug 590468 Opened 9 years ago Closed 9 years ago

Reduce size of chrome document layer due to status bar

Categories

(Core :: Layout, defect)

defect
Not set

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+
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.
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)
This creates a layer for the statusbar contents, addressing the performance issues described in this bug.
Attachment #469018 - Flags: review?
Attachment #469018 - Flags: review? → review?(dao)
We know this, because we paint them.
Attachment #469019 - Flags: review?(joshmoz)
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 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+
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.
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+
Attachment #469020 - Flags: review?(jmathies) → review+
Attachment #469019 - Flags: review?(joshmoz) → review+
Duplicate of this bug: 579263
Attachment #469017 - Flags: review?(tnikkel) → review+
Ah right, I'm glad I'd filed it. I should have marked it blocking 130078 though.
Keywords: checkin-needed
Whiteboard: [needs landing]
Bug 579263 is marked blocking 130078 :).
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.)
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)
Attached patch Part 6 v2 (obsolete) — Splinter Review
Try this
Attachment #469335 - Attachment is obsolete: true
Attachment #469344 - Flags: review?(tnikkel)
Attachment #469335 - Flags: review?(tnikkel)
Attached patch Part 6 v3 (obsolete) — Splinter Review
no, this
Attachment #469345 - Flags: review?(tnikkel)
Attached patch Part 6 v4 (obsolete) — Splinter Review
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)
The assertion "bad aListVisibleBounds: 'r.GetBounds() == aListVisibleBounds'" in part 6 gets hits a bunch of times on crashtests and reftests on try server.
(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.
(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.
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.
(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.
(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.
(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.
(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.
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.
s/Letting front-end code working around it/Letting front-end code work around it/
(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).
Attached patch Part 6 v5 (obsolete) — Splinter Review
-- 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)
Attachment #469351 - Attachment is obsolete: true
Attachment #469351 - Flags: review?(tnikkel)
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+
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?
Attachment #469795 - Flags: review? → review?(roc)
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?
Attachment #469798 - Flags: review? → review?(dao)
(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 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+
(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...
Let's go with the original part 5 then.
Attached patch Part 6 v6Splinter Review
Comments fixed. Should be ready to check in.
Attachment #469780 - Attachment is obsolete: true
Attachment #469795 - Attachment is obsolete: true
Attachment #469795 - Flags: review?(roc)
Attachment #469798 - Attachment is obsolete: true
Attachment #469798 - Flags: review?(dao)
Depends on: 591557
Keywords: checkin-needed
Whiteboard: [needs landing]
Depends on: 591853
(In reply to comment #30)
> Please file follow-up bugs on the side bar and :not([lwthemefooter="true"]).

Bug 591855 and bug 591557.
Depends on: 591998
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.
(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?
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
The "layer" attribute isn't documented on MDN AFAICT (see comment 0 and comment 1).
Keywords: dev-doc-needed
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.
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.