85860 - Starting second instance does not bring new window to the foreground.

Status: VERIFIED FIXED

(Core Graveyard :: Cmd-line Features, defect, P3)

Description

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

Steps to reproduce:
* Start a browser - with or without a URL on the command-line
* Attempt to start another instance of a browser with a different URL on the 
command line.

Existing Behaviour:
* A new window is opened for the second URL, but it is not in the foreground.

Expected Behaviour:
* The new window is in the foreground.

Note: If you pass the same URL, or no URL at all, no new window is created as 
the first is reused - in this case the single window should be brought to the 
foreground.

I should have a patch for this soon.  See also bug 65974 sa it is semi-related.

Adding law to CC as he seems to be involved in all these bugs :)

Comment 1

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

Adding a patch and fixing description.

Comment 2

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

Attachment: Proposed patch

Comment 3

[Bill Law]

Mark,

I had been thinking about this problem while hacking on this code recently for
turbo mode and DDE stuff.  My idea had been for the WM_COPYDATA message to send
back the window handle and have the sender then bring that window to the foreground.

But your solution is simpler and seems to accomplish the same result.

I have one question: Is the ::SetForegroundWindow call in nsWindow.cpp still
necessary?  I would think that the call you added prior to sending the
WM_COPYDATA would bring the other process to the foreground and the already
existing focus/activation stuff there would ensure the new window came to the top.

Comment 4

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

It appears you are correct - the nsWindow.cpp patch is not needed.  When I 
tried this yesterday it seemed to not work, but today it does!

As part of my testing, I discovered a slight problem with my patch.  If the 
second instance of Mozilla does not open a new window it does not work quite as 
expected - the existing window is not brought to the foreground, and the "Alt-
Tab" window list starts showing the hidden message window as the foreground 
window.

There is no second window created when no command-line params are specified to 
the second instance.

In this case it seems we will need to call nsWindow::Show() on the existing 
window.  It was probably this behaviour that confused me yesterday and made me 
mistakenly think the nsWindow patch was necessary and solved a problem.

Comment 5

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

Attachment: Less instrusive patch that actually works! Avoids nsWindow.cpp completely.

Comment 6

[Bill Law]

Mark, we have a bit of a problem.  Please look at bug 85664.  If you apply that
patch then I think your code won't ever kick in (except when we're responding to
client DDE requests).

Because all requests that result from a WM_COPYDATA message will now result in
new windows, I think that the second hunk (the ::SetForegroundWindow call in
OpenBrowserWindow) is not necessary (once the fix for bug 85664 lands).

When that hunk would execute, we will not have done the ::SetForegroundWindow
prior to sending the WM_COPYDATA; so it won't have the desired effect in that
case.  As it happens, DDE clients typically will have sent a WWW_Activate
request previously, and *that* would have triggered a ::SetForegroundWindow.

Can you apply the patch for bug 85664, remove the second hunk of the patch for
this bug, and see if it still works?

Comment 7

[Viswanath Ramachandran]

nav triage: moving to 1.0. We can reconsider, if the patch is reworked in 
accordance with Law's comments. 

Comment 8

[Mike McCabe]

Starting mozilla itself doesn't bring it into the foreground in front of other
application windows.  Possibly related... this is a problem I stumble across often.

I use a desktop switcher, and sometimes the interaction between that and modal
dialogs that don't get put in the foreground will lock the application.

Comment 9

[Bill Law]

->mozilla0.9.8

Comment 10

[Bill Law]

Attachment: Newer and simpler patch.

This one does the job without fiddling with the message window.

Note the slight tweak to nsGlobalWindow to suppress an annoying assertion
dialog that was appearing when the window was activated too early.  The native
window activation happens regardless and I haven't changed the logic at all.

Comment 11

[Dan M]

how about instead of this
+            if ( win ) {
+                return (long)hwndForDOMWindow( win );
this
+            return win ? (long)hwndForDOMWindow(win) : 0;
r=danm

Comment 12

[Bill Law]

Thanks, Dan.  You are right.  I had thought I'd just fall through to the end of
the if/else's at which point the 0 would be returned.  But the default is to
return TRUE, not 0!  My patch resulted in the caller doing
::SetForegroundWindow(1), which failed harmlessly, but was still wrong.

Comment 13

[jag (Peter Annema)]

Comment on attachment 63424 [details] [diff] [review]
Newer and simpler patch.

sr=jag, assuming you've applied dan's suggestion locally.

Comment 14

[Bill Law]

fixed

Comment 15

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

Works for me!  Thanks.  ->VERIFIED.