Closed Bug 392813 Opened 17 years ago Closed 16 years ago

nsCocoaWindows are hidden/shown multiple times (and we re-execute the same code)

Categories

(Core :: Widget: Cocoa, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: hwaara, Assigned: hwaara)

Details

Attachments

(1 file, 1 obsolete file)

I've noticed that nsCocoaWindow::Show() is called multiple times on the same window. 

For example, just bringing a window to front a few times, will call ::Show(PR_TRUE) every time, and we re-setup and re-initialize a bunch of stuff as if the window was fresh and wasn't already showing.

(I dare not say that this would be the cause of bug 354768, but my gut feeling is that "something like this" is exactly what would be the real cause of it.)

Unless this is by design, I think it is bug-prone and we should ensure to only show/hide once and when needed, otherwise we may end up in a weird inconsistent state.
Attached patch Fix (obsolete) — Splinter Review
Proposed patch. Bail early if we're already showing, or already hidden.

I've done some basic testing with this and things seem to still work right (I'd be worried if they didn't, considering there's no reason we should be running this code twice).
Attachment #277323 - Flags: review?(joshmoz)
CC'ing you FYI, Steven, since it might (or might not be) interesting when debugging bug 354768.
I should note that this patch might need some testing. Since this means we won't run Show(aShouldShow) if aShouldShow == mVisible, we should revisit for example the case when we in ::StandardCreate() create a window with mVisible set to true.

Also, Steven if you would have the time, it would be interesting if you could apply it on top of your other patch and hear your comments.
Attached patch Fix v1.1Splinter Review
This patch also makes us set mVisible correctly, in the case where a nsCocoaWindow is created from a native window. 

We already have the bug that if a hidden native window uses nsCocoaWindow, we'll end up in an inconsistent state.

I've been doing some general testing with this in both Camino and Firefox to make sure things still work. Using this patch, I've only found one irregularity during startup in Firefox:

*** Registering components in: nsAlertsServiceModule
WARNING: Cannot create startup observer : service,@mozilla.org/alerts-service;1: file /Users/hakan/Programmering/mozilla/mozilla/embedding/components/appstartup/src/nsAppStartupNotifier.cpp, line 113

I haven't verified that it's because of my change. (Has anyone seen this already on trunk?) I'll verify & look further into it before requesting review.
Attachment #277323 - Attachment is obsolete: true
Attachment #277323 - Flags: review?(joshmoz)
As a side-note: the mVisible variable on nsCocoaWindow is useless, AFAICS. We could replace the 2-3 places it's ever used by just doing [mWindow isVisible]. 
> As a side-note: the mVisible variable on nsCocoaWindow is useless,
> AFAICS. We could replace the 2-3 places it's ever used by just doing
> [mWindow isVisible].

I'm not so sure about this.

Currently we set mVisible to TRUE when an (ordinary) NSWindow is
"ordered out" (by nsCocoaWindow::Show()) and to FALSE when it's
"ordered front".

I'm not sure that isVisible: called on an NSWindow that's been
"ordered out" always returns FALSE.

And mVisible is also used to indicate whether "sheets" are visible or
not -- and I have no idea how isVisible: works on them.
> Currently we set mVisible to TRUE when an (ordinary) NSWindow is
> "ordered out" (by nsCocoaWindow::Show()) and to FALSE when it's
> "ordered front".
>
> I'm not sure that isVisible: called on an NSWindow that's been
> "ordered out" always returns FALSE.

Sorry.  We set mVisible TRUE when an (ordinary) NSWindow is _"ordered
front"_ and FALSE when it's _"ordered out"_. ...
(From comment #6)

> I'm not sure that isVisible: called on an NSWindow that's been
> "ordered out" always returns FALSE.

I've now done some tests that indicate I was wrong about this -- they
show that isVisible always returns FALSE for NSWindows that have just
been "ordered out" by nsCocoaWindow::Show() (and that isVisible always
returns TRUE for NSWindows that have just been "ordered front" by
nsCocoaWindow::Show()).

I wasn't able to figure out how to test "sheets", though.

I also confirmed Håkan's initial report that nsCocoaWindow::Show()
often gets called several times in a row with the same value for
bState -- i.e. if often gets called several times in a row to "show"
the same window or to "hide" the same window.

Håkan, I hope to try your patch later this week.
checked in this fix as part of bug 417124
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: