389423 - Clicking on Dock icon doesn't restore last minimized window in Minefield

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Marcia Knous [:marcia]]

Seen using Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a7pre) Gecko/200707240404 Minefield/3.0a7pre.

STR:
1. Minimize an open window.
2. Click on the Minefield dock icon.

Expected: The open window will launch.
Reality: Clicking does nothing, the open page does not maximize.

Comment 1

[Steven Michaud [:smichaud] (Retired)]

My guess is this is some kind of Apple Event silliness, which it might
be possible to deal with in toolkit/xre/MacApplicationDelegate.mm.

I'll take a look when I have the chance (probably later this week or
next week).

Comment 2

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix that doesn't use/change nsNativeAppSupportMac.cpp

Turns out this bug is caused by problems in both
MacApplicationDelegate.mm and nsNativeAppSupport.cpp.

Here's a patch that changes only MacApplicationDelegate.mm (it
replaces the methods from nsNativeAppSupport.cpp that need to be
changed).

This works just fine, but it violates the nsINativeAppSupport
paradigm.

So (hopefully later today) I'll also post a second, alternative patch
that changes both MacApplicationDelegate.mm and nsNativeAppSupport.cpp.

The applicationShouldHandleReopen: delegate method (in
MacApplicationDelegate) is called whenever the user clicks the Dock
icon.  The current code correctly handles the case where the browser
has no open windows (not even minimized ones) -- it calls
nsNativeAppSupportMac::ReOpen(), which opens a new browser window.

But (as Marcia reported in comment #0) it doesn't correctly handle the
browser has open windows but all of them are minimized.  There are two
reasons for this:

1) The current applicationShouldHandleReopen: method does nothing if
   hasVisibleWindows is TRUE.  But on OS X a minimized window counts
   as a visible window.

2) The code in nsNativeAppSupportMac::ReOpen() that handles minimized
   windows only works on Carbon apps -- not on Cocoa apps.

Comment 3

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix that creates and uses nsNativeAppSupportCocoa.mm

Here's my alternate patch, which I promised earlier, that uses
nsINativeAppSupport.  I made a few changes to
nsNativeAppSupportMac.cpp (to make it support Cocoa apps instead of
Carbon ones) and changed its name to nsNativeAppSupportCocoa.mm.

Because support for Mac widgets has been dropped (in favor of Cocoa
widgets), we no longer need a nsNativeAppSupportMac.cpp on the trunk.

Warning:  This is not a proper patch, and won't apply to the trunk.
In order to make it more readable (and to show how few changes I made
to nsNativeAppSupportMac.cpp), I changed the "original"
nsNativeAppSupportMac.cpp file's name to nsNativeAppSupportCocoa.mm
before generating the patch.

Side note:  For good measure I also changed the code in
nsNativeAppSupportCocoa::Start() to check for at least OS X 10.4.  I
haven't yet had a chance to test this.  Hopefully I'll be able to do
that sometime this afternoon.

Comment 4

[Steven Michaud [:smichaud] (Retired)]

Benjamin, which of these two kinds of patches do you prefer?

Whichever one you pick, I'll ask someone to review it.

Comment 5

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 274633 [details] [diff] [review]
Fix that doesn't use/change nsNativeAppSupportMac.cpp

I talked to Benjamin and Michael Wu on #developers, and the
nsINativeAppSupport patch won by a nose.

But if someone has strong preferences the other way, let me know.

Comment 6

[Steven Michaud [:smichaud] (Retired)]

> Side note:  For good measure I also changed the code in
> nsNativeAppSupportCocoa::Start() to check for at least OS X 10.4.  I
> haven't yet had a chance to test this.  Hopefully I'll be able to do
> that sometime this afternoon.

I just tested this in OS X 10.3 ... and it sort-of worked.

No alert message was displayed, but the browser did quit immediately.
Today's Minefield nightly also quits immediately, but only after
displaying an ld error about an undefined reference to
_GetAliasSizeFromPtr.  So my build's behavior is an improvement.

Better still would be to display an alert box and then die.

I don't know the ancient Mac techniques used in the code for
nsNativeAppSupportCocoa::Start() that I borrowed from
nsNativeAppSupportMac.cpp.  But I could manage to display some kind of
Cocoa alert dialog ... which should be fine for our purposes.

I'll work on that (probably tomorrow).

Comment 7

[Steven Michaud [:smichaud] (Retired)]

On second thought maybe I shouldn't display an alert box.

It would be a cop-out to display it always in English ... but
localizing it would be a _lot_ of work (most of which would have to be
done by other people).

Maybe it's best to change my patch to explicitly make the browser die
silently on OS X versions < 10.4.

If someone wants to make nsNativeAppSupportCocoa::Start() display an
alert box on OS X versions less than 10.4, that can be the subject of
another bug.

Comment 8

[Samuel Sidler (old account; do not CC)]

Steven, 10.3 is no longer supported on the trunk. You probably don't need to worry about it for this patch.

Comment 9

[Steven Michaud [:smichaud] (Retired)]

Attachment: Revised nsINativeAppSupport patch (quit with console error on less than 10.4)

Here's a revision of my nsINativeAppSupport patch that gets rid of the
ancient (and no longer workable) dialog cruft from
nsNativeAppSupportCocoa::Start() and replaces it with code that logs
to the console (in English) and quits on OS X versions less than 10.4.

This doesn't fully resolve the problem of how to let the user know
that the browser won't run on these OS X versions ... but that should
be saved for another bug.

My current code is a decent interim solution ... and is definitely an
improvement on displaying a linker error :-)

Comment 10

[Josh Aas]

Comment on attachment 274781 [details] [diff] [review]
Revised nsINativeAppSupport patch (quit with console error on less than 10.4)

Please search for and replace all the tabs in nsNativeAppSupportCocoa.mm with two spaces. Looks like there are more in that file. This can be done on checkin.

+            *a_nativeWindow = (NSWindow *)mruWidget->GetNativeData(NS_NATIVE_WINDOW);

Please no space between the NSWindow and its pointer '*'.

NS_ADDREF( *aResult );

Please remove the spaces around the *aResult.

Comment 11

[Steven Michaud [:smichaud] (Retired)]

Attachment: nsINativeAppSupport patch rev2 (follow Josh's suggestions)

Comment 12

[Benjamin Smedberg]

Comment on attachment 276642 [details] [diff] [review]
nsINativeAppSupport patch rev2 (follow Josh's suggestions)

Approval is not required in toolkit/

Comment 13

[Steven Michaud [:smichaud] (Retired)]

OK, thanks.

I'll land this tomorrow.

Comment 14

[Steven Michaud [:smichaud] (Retired)]

Landed on trunk.

Comment 15

[Asa Dotzler [:asa]]

Will Thunderbird need a similar fix? 

Comment 16

[Steven Michaud [:smichaud] (Retired)]

> Will Thunderbird need a similar fix?

I don't know.

Does Thunderbird currently use Cocoa widgets and XRE_main() (in
toolkit/xre/nsAppRunner.cpp)?  If so, current trunk versions should
already have this fix.

(I assume you're talking about Thunderbird on the trunk.  Branch
versions will almost certainly not need this fix.)

Comment 17

[Asa Dotzler [:asa]]

Yes, I'm talking about the trunk. I've cc'd mscott. 

Comment 18

[Scott MacGregor]

yeah that patch would be used by Thunderbird too Asa. Did the fix not work for you?

Comment 19

[Asa Dotzler [:asa]]

Sorry, I haven't gotten today's build yet for trunk Tbird. I'll assume for now that all is cool here. Just wanting to make sure though. Thanks guys.