Created attachment 157638 [details] [review]
[chrome] Fix fullscreen behaviour

The check in _windowsRestacked checks for
windows[i].x >= primary.x
and
windows[i].x <= primary.x + primary.width
(likewise for y).

This is wrong because a fullscreen window on the secondary screen is likely
to have windows[i].x == primary.x + primary.width which means that the checks
for _both_ screens would be valid, but the first one would win due to
the "break;" statement.

But here the window isn't really on the primary but on the secondary one.

Fix that by using < instead of <= for those checks.

Created attachment 157826 [details] [review]
chrome: Fix fullscreen for broken apps

The fullscreen check in chrome.js still fails to detect a case where an app
sets the window coordinates to negative values.

Use Math.abs to work around this nonsense.

(The app in question is npviewer.bin aka flash)

Review of attachment 157826 [details] [review]:

::: js/ui/chrome.js
@@ +210,3 @@
             let layer = windows[i].get_meta_window().get_layer();
+                    Math.abs(windows[i].y) >= primary.y && Math.abs(windows[i].y) < primary.y + primary.height) {
+                if (Math.abs(windows[i].x) >= primary.x && Math.abs(windows[i].x) < primary.x + primary.width &&

Wouldn't we want Math.max(coord, 0), not Math.abs?  Or is it really setting it to the negative of what's expected?

(In reply to comment #3)
> Review of attachment 157826 [details] [review]:
> 
> ::: js/ui/chrome.js
> @@ +210,3 @@
>              let layer = windows[i].get_meta_window().get_layer();
> +                    Math.abs(windows[i].y) >= primary.y &&
> Math.abs(windows[i].y) < primary.y + primary.height) {
> +                if (Math.abs(windows[i].x) >= primary.x &&
> Math.abs(windows[i].x) < primary.x + primary.width &&
> 
> Wouldn't we want Math.max(coord, 0), not Math.abs?  Or is it really setting it
> to the negative of what's expected?

As we window is fullscreen anyway it does not matter what values it uses as long as it is on the same screen, but yeah Math.max(coord, 0) sounds more correct here.

Created attachment 157829 [details] [review]
chrome: Fix fullscreen for broken apps

The fullscreen check in chrome.js still fails to detect a case where an app
sets the window coordinates to negative values.

We just assume 0 when an app sets a negative value to work around this
nonsense.

(The app in question is npviewer.bin aka flash)

Comment on attachment 157829 [details] [review]
chrome: Fix fullscreen for broken apps

So what happens if your primary monitor doesn't start at 0,0, and you fullscreen flash on it? Does it position itself at -1,-1 or at primary.x-1,primary.y-1?

And this is getting confusing enough that we need a comment explaining exactly what it's checking for. Which, at this point, I think is that some apps just set FULLSCREEN and don't change their requested size/position at all, and some resize themselves to screen-sized and position themselves at the monitor's 0,0 position, and some apparently position themselves slightly offscreen (flash).

(In reply to comment #6)
> (From update of attachment 157829 [details] [review])
> So what happens if your primary monitor doesn't start at 0,0, and you
> fullscreen flash on it? Does it position itself at -1,-1 or at
> primary.x-1,primary.y-1?

I does position the window on 0,-24 for whatever reason ...

> And this is getting confusing enough that we need a comment explaining exactly
> what it's checking for.

Yeah makes sense.

> Which, at this point, I think is that some apps just
> set FULLSCREEN and don't change their requested size/position at all, and some
> resize themselves to screen-sized and position themselves at the monitor's 0,0
> position, and some apparently position themselves slightly offscreen (flash).

Yeah this are the 3 cases we check for.

Comment on attachment 157829 [details] [review]
chrome: Fix fullscreen for broken apps

Document fullscreen checks and add a third case to catch apps like
flash who position there windows sightly offscreen to hide there decorations

*** Bug 613701 has been marked as a duplicate of this bug. ***

Note that flash is definitely fixed, but EDuke32 (http://eduke32.com/) still shows below the panel when displayed fullscreen. I've traced it to happening only if the game changes the current desktop/display resolution.

*** Bug 617165 has been marked as a duplicate of this bug. ***

*** Bug 609425 has been marked as a duplicate of this bug. ***

This was fixed, right?