1881597 - Audit nsCocoaWindow early exits on native window existence

Status: NEW

(Core :: Widget: Cocoa, enhancement, P3)

Description

[Brad Werth [:bradwerth] | Out until March 3]

Bug 1879816 revealed that nsCocoaWindow functions tend to equate the existence of the mWindow (the native window object) as an indicator of whether or not the function should be a no-op. Many of these functions have side effects, and it may be important for those side effects to occur even when the native window has been destroyed. Perform an audit of this class and reason how they would and should behave when mWindow is nil.

Comment 1

[Brad Werth [:bradwerth] | Out until March 3]

Attachment: Bug 1881597: Clarify the existence checks for the native window object in nsCocoaWindow.

The changes in this patch are all motivated by the question "what if
mWindow was nil?" The resulting changes fall into the following
categories:

1. Removing if(!mWindow) checks almost everywhere. Most of these
   checks are optimizations to short-circuit later changes to mWindow.
   While well-intentioned, even if there aren't important side effects now,
   this is a foot-gun for later changes that may add important side
   effects. And it's also unlikely we're doing enough calls on an
   nsCocoaWindow whose native window has already been destroyed that this
   will be a performance hit. Since it's safe to invoke selectors on nil
   ids, just take the check out and let the selectors return their default
   values. This has the net effect of simplifying some functions that were
   explicitly using a default value which is what we get if we call the
   selector anyway.

2. Ensuring more side effects get called. This is a corrollary of the
   above. Show() will now unregister popup windows from their parents.
   DispatchOcclusionState() will notify widget listeners if the native
   window is gone (that makes it transparent). DispatchSizeModeEvent() will
   notify widget listeners if the native window is gone (that makes it
   "normal").

3. Add some checks where needed. The change in ProcessTransitions()
   ensures that we don't wait for a delegate method that will never be
   called. The assert added to CreateNativeWindow() ensures we aren't
   overwriting mWindow without destroying it.

4. Centralizing some side effects. SetMenuBar() had a side effect when
   mWindow was nil; that got moved into DestroyNativeWindow().
   DestroyNativeWindow() also added a call to Show() since it was clear
   that the side effects in Show() required the existence of mWindow.

The net effect of all these changes is that there is less nsCocoaWindow
code, and listeners will be informed about changes to nsCocoaWindows
whose native windows have been destroyed.

Comment 2

[Brad Werth [:bradwerth] | Out until March 3]

Try run looks good https://treeherder.mozilla.org/jobs?repo=try&revision=844fc51d9b8cb465109a8810ba182686ab5b489d&selectedTaskRun=JmleCxXiTpytO2cOVr1EcA.0.