891194 - BROKEN_WM_Z_ORDER should not be defined on Mac

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

The code in the BROKEN_WM_Z_ORDER branch has different semantics (in terms of window ordering) than when it's not defined, as social discovered in bug 874566.  It looks like bug 462222 is currently fixing this - once it is done we can just drop this workaround from the module.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Via bug we, the current state seems to be that BROKEN_WM_Z_ORDER should now only be considered true on Linux, but Mac works fine.  So in the short term, it might make sense to change the #ifdef in RecentWindow.jsm to reflect this.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

(In reply to Mark Hammond (:markh) from comment #1)
> Via bug we

Was that supposed to be a bug #? :) If your findings are right, we should  re-summarize bug 462222 and adjust BROKEN_WM_Z_ORDER accordingly in a bunch of places.

Comment 3

[Mark Hammond [:markh] [:mhammond]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #2)
> (In reply to Mark Hammond (:markh) from comment #1)
> > Via bug we
> 
> Was that supposed to be a bug #? :)

It was :)  Bug 874566.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

re-summarizing this to be about the general problem, now that bug 450576 is WFM.

Comment 5

[Katie Thomas]

Attachment: name.patch

This is my first patch--all feedback will be greatly appreciated!

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8345931 [details] [diff] [review]
name.patch

Hey Katie,

Those review flags should be used to request review - setting them to "?" and entering in your mentor's email address sends the review request. I've taken the liberty of doing that for you, and requesting review from your mentor.

Thanks for the patch!

-Mike

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 8345931 [details] [diff] [review]
name.patch

Thanks!

Comment 8

[Dão Gottwald [:dao]]

https://hg.mozilla.org/integration/fx-team/rev/9f7da731bf06

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 8345931 [details] [diff] [review]
name.patch

Backed out (https://hg.mozilla.org/integration/fx-team/rev/d3c5304d8011) because this seems to have caused browser_586068-browser_state_interrupted.js to fail on Linux, probably due to a trailing space in the ifdef:

>--- a/browser/components/sessionstore/src/SessionStore.jsm
>+++ b/browser/components/sessionstore/src/SessionStore.jsm
>@@ -81,19 +81,21 @@ const TAB_EVENTS = [
> // Browser events observed.
> const BROWSER_EVENTS = [
>   "load", "SwapDocShells", "UserTypedValueChanged"
> ];
> 
> // The number of milliseconds in a day
> const MS_PER_DAY = 1000.0 * 60.0 * 60.0 * 24.0;
> 
>-#ifndef XP_WIN
>+#ifdef XP_UNIX 
>+#ifndef XP_MACOSX
> #define BROKEN_WM_Z_ORDER
> #endif
>+#endif

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Dão Gottwald [:dao] from comment #9)
> because this seems to have caused
> browser_586068-browser_state_interrupted.js to fail on Linux, probably due
> to a trailing space in the ifdef:

Confirming that this fixes the timeout.

Comment 11

[Dão Gottwald [:dao]]

Relanded with the trailing spaces removed:

https://hg.mozilla.org/integration/fx-team/rev/adb909c5d6c3

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/adb909c5d6c3

Comment 13

[Ben Bucksch (:BenB)]

This may have caused regression bug 924456, see bug 924456 comment 38.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

More "exposed" rather than caused, FWIW.