1155668 - Session restore should restore windows to the proper workspace on Ubuntu

Status: NEW

(Firefox :: Session Restore, defect)

Description

[(not currently active) Ted Mielczarek]

I'm intentionally filing a bug separate from bug 372650 because that one is crufty and I have a very specific set of fixes in mind for this.

Ubuntu uses Compiz as the default window manager. Compiz implements workspaces as "viewports" per:
http://standards.freedesktop.org/wm-spec/wm-spec-latest.html#largedesks

So, for example, on my Ubuntu system with a single 1920x1200 monitor attached and a 2x2 set of workspaces it's configured like:
$ xprop -root -notype _NET_NUMBER_OF_DESKTOPS
_NET_NUMBER_OF_DESKTOPS = 1

$ xprop -root -notype _NET_DESKTOP_GEOMETRY
_NET_DESKTOP_GEOMETRY = 3840, 2400

$ xwininfo -root | grep geometry
  -geometry 1920x1200+0+0

So I have a single desktop with geometry of 3840x2400, but the work area is only 1920x1200 (minus launcher etc). The root window is in fact only 1920x1200. You can also find out what the current viewport is, so with a terminal on the top right of my 4 workspaces, I get:

$ xprop -root -notype _NET_DESKTOP_VIEWPORT
_NET_DESKTOP_VIEWPORT = 1920, 0

When you switch workspaces, Compiz moves all your windows relative to the root window, so if you switch to a workspace to the right of the current one, any windows on the workspace you're switching away from get moved to negative X coordinates. If you switch to a workspace to the left of the current one they will get moved to X coordinates larger than the desktop width (and similarly for up/down). A simple example, this is a Firefox window on my top right workspace when I switch to the top-left and back:
-134445184[7ffff6b684a0]: GetScreenBounds 2139,62 | 1391x854
-134445184[7ffff6b684a0]: GetScreenBounds 219,62 | 1391x854

...and if I move it to the top-left workspace and then switch to the top-right:
-134445184[7ffff6b684a0]: GetScreenBounds -1701,62 | 1391x854

So there are two things that need to get fixed to make this work properly:
1) The SessionStore code doesn't currently listen for events for window moves. Since window positions change every time you switch desktops, this means that the position it has for every window is likely to be coordinates within the root window, since it updates window state when tab state changes and you're likely to have the window visible when that's happening. We do already have a chrome event for window moves, we probably just need to hook up an event listener for that:
https://hg.mozilla.org/mozilla-central/annotate/51e3cb11a258/xpfe/appshell/nsWebShellWindow.cpp#l262

2) SessionStore refuses to restore windows outside of the screen bounds, which is not unreasonable:
https://hg.mozilla.org/mozilla-central/annotate/51e3cb11a258/browser/components/sessionstore/SessionStore.jsm#l2829

We'll probably have to add an API to nsIScreenManager or nsIScreen to give it more information so that it knows that restored windows will be accessible.

I'll file separate bugs on implementing the bits necessary for these two points.

Comment 1

[(not currently active) Ted Mielczarek]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e93ba0d111e3

Comment 2

[(not currently active) Ted Mielczarek]

Attachment: MozReview Request: bz://1155668/ted

/r/7459 - bug 1155668 - Session restore should restore windows to other workspaces under Compiz. r=ttaubert

Pull down this commit:

hg pull -r dcb594d64fa67dc79b83cebaa0a6872129725ea7 https://reviewboard-hg.mozilla.org/gecko/

Comment 3

[(not currently active) Ted Mielczarek]

This patch is fairly straightforward given all the dependencies. The browser-chrome test is a mixed bag--it only works under Compiz but it should detect and exit gracefully in other window managers. Just to be sure I added a key to the manifest conditions with the window manager name (bug 1157414) so we don't even try to run this elsewhere.

Unfortunately this means we won't run this test in automation as-is. I'll file a RelEng bug to see if we could feasibly run b-c tests on EC2 GPU instances where we could actually run Compiz.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

https://reviewboard.mozilla.org/r/7459/#review6253

::: browser/components/sessionstore/test/browser_window_restore_compiz_workspace.js:53
(Diff revision 1)
> +  let promiseRestored = promiseTopic("sessionstore-windows-restored", 1000);

Can you please pass a higher timeout? We've had problems with low timeouts on slow machines. They're arbitrary and I'd really like to get rid of them soon. Maybe something like 10s should be sufficient to keep it from failing in the near future.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8596128 [details]
MozReview Request: bz://1155668/ted

https://reviewboard.mozilla.org/r/7457/#review6255

Ship It!

Comment 6

[(not currently active) Ted Mielczarek]

I kind of wanted that topic callback to not have a timeout and just use the default Mochitest timeout, but I didn't want to change the existing code.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Yeah totally understand that, that's fine. Just wanted to prevent issues for now.

Comment 9

[(not currently active) Ted Mielczarek]

Attachment: MozReview Request: bug 1155668 - Session restore should restore windows to other workspaces under Compiz. r=ttaubert

Comment 10

[Paul Molodowitch]

Hi - I would love for firefox windows to restore to the right workspace on Ubuntu. Does this fix still apply? Seems like it was 99% of the way there and then got forgotten...