Reduce duplication and unify responsibility for window sizing

NEW
Unassigned

Status

()

enhancement
P3
normal
Last year
10 months ago

People

(Reporter: Gijs, Unassigned)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 affected)

Details

(Whiteboard: [fxperf:p5][qf:p5])

As noted in bug 1362774:

There are clearly too many cooks (browser JS, browser CSS, xpfe, widget code, resist fingerprinting, the OS, session restore, XUL store, nsBrowserGlue for the initial blank window, ...) in the "how do I size the window" kitchen.

Specifically, in no particular order:

1) we store sizes and sizemodes in XULStore for all windows
2) sessionstore stores sizes and sizemodes in its own storage for browser windows and some other windows (unsure if everything; I know it does "something" for devtools as well)
3) browser.js picks an initial size for windows
4) resistfingerprinting wants a proverbial finger-in-the-pie for every window that gets opened, clamping window sizes to particular intervals to reduce fingerprintability.
5) websites can open windows at particular sizes, and influence how much chrome they have etc.
6) a combination of browser JS, browser CSS and widget code influence chrome size, which in turn influences sizing for cases (3) and (4) where we have to extrapolate from the desired content size to something that will allow space for the chrome.
7) tabsintitlebar code (in browser-tabsintitlebar.js) influences those things as well
8) first blank window code in nsBrowserGlue.js and resistfingerprinting code in browser.js  manually intervene with some of this separately, too.


It would be really nice if we could reduce down this list of stakeholders. Maybe only sessionrestore or only nsXULWindow or whatever could have the final say over window sizing, based on having input from everywhere else, and it could do it in a more straightforward way, rather than creating and resizing at different points in the lifetime of the window, from different parts of the codebase. The fact that everything currently just messes with the bits they care about directly makes for a haphazard and buggy implementation that makes it hard to make any changes.

This is mostly a maintenance issue, though if we can reduce the amount of resizing, layout and measuring we do as part of all this, it's possible we could reduce the number of reflows and thus improve (perceived) perf a bit.
(In reply to :Gijs (he/him) from comment #0)
> As noted in bug 1362774:
> 
> There are clearly too many cooks (browser JS, browser CSS, xpfe, widget
> code, resist fingerprinting, the OS, session restore, XUL store,
> nsBrowserGlue for the initial blank window, ...) in the "how do I size the
> window" kitchen.

Thanks for filing this!

> 1) we store sizes and sizemodes in XULStore for all windows
> 2) sessionstore stores sizes and sizemodes in its own storage for browser
> windows and some other windows (unsure if everything; I know it does
> "something" for devtools as well)

Yes, and I'm not sure how that can become different. I'd be fine with, for example, passing size(mode) hints to a delegator that'll become in charge of handling everything window-property related.
Devtools integration is basically sessionstore code invoking hooks to devtools, through a devtools <--> browser glue proxy, and devtools does its own thing at that point.

> 4) resistfingerprinting wants a proverbial finger-in-the-pie for every
> window that gets opened, clamping window sizes to particular intervals to
> reduce fingerprintability.

If possible, I'd like resistfingerprinting yanked out of sessionstore. I think it was a mistake to put it there, instead of letting it adjust size(mode)s data during runtime only.
However, I think the implementation that was picked is entirely correlated to this very issue!
(In reply to Mike de Boer [:mikedeboer] from comment #1)
> > 1) we store sizes and sizemodes in XULStore for all windows
> > 2) sessionstore stores sizes and sizemodes in its own storage for browser
> > windows and some other windows (unsure if everything; I know it does
> > "something" for devtools as well)
> 
> Yes, and I'm not sure how that can become different. I'd be fine with, for
> example, passing size(mode) hints to a delegator that'll become in charge of
> handling everything window-property related.
> Devtools integration is basically sessionstore code invoking hooks to
> devtools, through a devtools <--> browser glue proxy, and devtools does its
> own thing at that point.

I think the history here is that we had the toolkit/xpfe/xpcom "just ask the xul persist storage for a size" stuff, which could be overridden by the window creation code (which was more or less in the same folders / toolkit space anyway), but we wanted to be able to restore multiple different windows at different sizes and so then the browser session restore code was born, and it just overwrites sizes (and sizemodes, of course) where/when necessary.

With the browser/toolkit separation less and less relevant, it seems the "simplest" solution would be to create a single thing responsible for window sizes, that could deal with remembering more than 1 windowsize. In practice, perhaps the easiest would be if sessionstore grew the capacity to remember the last window size for a given toplevel document URL like browser.xul or pageinfo.xul or whatever (and we took that out of xulstore). We could even move the sessionstore code into toolkit. The only potential downside I can think of here is that we currently load some sessionstore stuff pretty late, and we obviously need to know the size for at least the first window pretty soon.

Mike, thoughts on this idea? Is there a more lightweight option that I'm missing, maybe?
Flags: needinfo?(mdeboer)
Priority: -- → P3
I wholeheartedly agree that window sizing is very messy and I never wanted to touch it because of how complex it is. I'm just marking fxperf:p5 because it's not clear to me that it would lead to direct performance improvements, or if it's just filed for improving maintainability (although one could argue that making easier changes here would lead to making it easier for get other performance wins)

If, however, this becomes a necessity for bug 1362774, then we should reclassify it as p2.
Whiteboard: [fxperf] → [fxperf:p5][qf:p5:f64]
(In reply to :Gijs (he/him) from comment #2)
> Mike, thoughts on this idea? Is there a more lightweight option that I'm
> missing, maybe?

This sounds like a perfectly fine idea. I would even consider having this as a separate toolkit JSM to choose an appropriate storage mechanism to indeed circumvent tightening the connection between startup and sessionstore for startup window sizes.
In fact, a future direction where we chunk pieces of sessionstore data to be hauled in segmented by priority would be quite reasonable, instead of the current 'throw all the things in one giant JSON blob' strategy.
Flags: needinfo?(mdeboer)
(This separate module may end up living at toolkit/components/sessionstore/ of course!)
Whiteboard: [fxperf:p5][qf:p5:f64] → [fxperf:p5][qf:p5]
You need to log in before you can comment on or make changes to this bug.