300485 - Launching SeaMonkey when it's already running, with "open in new tab" preference, should open a new tab rather than window.

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Jason Bassford]

When preferences are set to open links in a new tab, rather than a new window,
SeaMonkey is already running, and you launch it again, you should get a new tab
in the existing window rather than a new browser window.

This is in line with the "single window with multiple tabs" experience people
would expect from using tabbed browsing.

If somebody really wants a new window, they can use File -> New -> Navigator
Window, or Ctrl-N.

Implementing this should also fix the problem with some external links, called
from other applications, still opening in a new window rather than a tab.

Comment 1

[Andrew Schultz]

oops.  forgot this was already filed.

*** This bug has been marked as a duplicate of 299983 ***

Comment 2

[Jason Bassford]

This bug isn't about opening a link, per se.  It's about what happens when you
launch another instance of Mozilla.  It's not a URL that's being opened, but the
application itself.

I'm not sure if fixing bug 299983 will fix this one.

Comment 3

[Sven Grull]

This is also fixed after "updating" the singlewindow extension as described in
bug 300465. 
The question in general is whether this bug will covered by the fix
for bug 121969 or whether this bug is a new rfe.

Comment 4

[[e-mail]]

Attachment: Proposed patch (Windows)

You can't do clever stuff in app startup, not even NS_NewURI :-(

Comment 5

[jag (Peter Annema)]

Comment on attachment 189715 [details] [diff] [review]
Proposed patch (Windows)

>Index: nsNativeAppSupportWin.cpp
>===================================================================

>@@ -1772,8 +1779,7 @@
>       printf( "Launching browser on url [%s]...\n", arg.get() );
> #endif
>       if (NS_SUCCEEDED(nativeApp->EnsureProfile(args)))
>-        (void)OpenBrowserWindow( arg.get(), newWindow );
>-      return;
>+        return OpenBrowserWindow( arg.get(), newWindow, aResult );
>     }
> 
> 
>@@ -1785,8 +1791,7 @@
>       printf( "Launching chrome url [%s]...\n", arg.get() );
> #endif
>       if (NS_SUCCEEDED(nativeApp->EnsureProfile(args)))
>-        (void)OpenWindow( arg.get(), "" );
>-      return;
>+        return OpenWindow( arg.get(), "", aResult );
>     }
> 
>     // try for the "-profilemanager" argument, in which case we want the

You've changed the logic here from always returning to only returning on
success.

Just wanted to write this down so I won't forget, I'll look at the rest of the
patch later.

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 189715 [details] [diff] [review]
Proposed patch (Windows)

+	     (void)nsNativeAppSupportWin::HandleRequest( (LPBYTE)cds->lpData,
PR_FALSE, getter_AddRefs( win ) );

why are you passing PR_FALSE here? mmm... ok, so you are passing it so that a
new window isn't forced anymore?

(some docs on these arguments would be really nice)

(and a -p diff would've also been nice)

+	     nsCOMPtr<nsIDOMWindow> win;
+	     HandleRequest( LPBYTE( url.get() ), new_window, getter_AddRefs(
win ) );

maybe HandleRequest should allow passing nsnull if the caller isn't interested
in the window...

     // If a window was opened, then we're done.
     // Note that we keep on trying in the unlikely event of an error.
     if (rv == NS_ERROR_NOT_AVAILABLE || rv == NS_ERROR_ABORT || windowOpened)
{
-      return;
+      return rv;

shouldn't that be return NS_OK here?

@@ -2154,7 +2158,7 @@
   if (wwatch && sarg) {
     nsCOMPtr<nsIDOMWindow> newWindow;

newWindow can be removed

+	 nsCOMPtr<nsIDOMChromeWindow> chromeWin( do_QueryInterface( navWin ) );
+	 if ( !chromeWin ) {

should that maybe be an assertion too?

+	 io->NewURI( nsDependentCString( args ), nsnull, nsnull,
getter_AddRefs( uri ) );

I doubt that args is UTF-8, but oh well...


Was that change jag pointed out intentional?

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

In OpenBrowserWindow, if newWindow==PR_TRUE, shouldn't you pass OPEN_NEWWINDOW
to nsIBrowserDOMWindow? hm... I guess neither NEWTAB nor NEWWINDOW are really
great, since it should really follow the user prefs.

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

(sorry about the delay, the patch looked scarier than it was...)

Comment 9

[[e-mail]]

(In reply to comment #5)
> (From update of attachment 189715 [details] [diff] [review])
>>       if (NS_SUCCEEDED(nativeApp->EnsureProfile(args)))
>>-        (void)OpenBrowserWindow( arg.get(), newWindow );
>>-      return;
>>+        return OpenBrowserWindow( arg.get(), newWindow, aResult );
Fixed locally to
rv = nativeApp->EnsureProfile(args);
if (NS_SUCCEEDED(rv))
  rv = OpenBrowserWindow( arg.get(), newWindow, aResult );
return rv;
Similarly for the second change.

(In reply to comment #6)
>(some docs on these arguments would be really nice)
OK, I've tweaked the comments for HandleRequest and OpenBrowserWindow locally.

>(and a -p diff would've also been nice)
Sorry, this is my tree without a .cvsrc, so I have to remember to type it :-/

>maybe HandleRequest should allow passing nsnull if the caller isn't interested
>in the window...
I tried that and decided it was too much effort...

>>     // If a window was opened, then we're done.
>>     // Note that we keep on trying in the unlikely event of an error.
>>     if (rv == NS_ERROR_NOT_AVAILABLE || rv == NS_ERROR_ABORT || windowOpened)
>>     {
>>-      return;
>>+      return rv;
>shouldn't that be return NS_OK here?
Fixed locally.

>>   if (wwatch && sarg) {
>>     nsCOMPtr<nsIDOMWindow> newWindow;
>newWindow can be removed
Yeah, that was left over from my null result attempt.

>>+	 io->NewURI( nsDependentCString( args ), nsnull, nsnull, getter_AddRefs( uri
) );
>I doubt that args is UTF-8, but oh well...
I checked and there are three callers. The first passes "about:blank"; the
second calls nsICmdLineService::GetURLToLoad which thoughtfully doesn't specify
the charset but I expect needs to be fixed to be UTF-8; the third caller used
NS_LossyConvertUTF16toASCII which I changed to NS_ConvertUTF16toUTF8.

(In reply to comment #7)
>In OpenBrowserWindow, if newWindow==PR_TRUE, shouldn't you pass OPEN_NEWWINDOW
>to nsIBrowserDOMWindow? hm... I guess neither NEWTAB nor NEWWINDOW are really
>great, since it should really follow the user prefs.
That would assume that I can find an nsIBrowserDOMWindow...

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

> That would assume that I can find an nsIBrowserDOMWindow...

oops, I forgot that there was an if (newWindow) break; above

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 189715 [details] [diff] [review]
Proposed patch (Windows)

r=biesi with the changes from comment 9; can you attach a patch with them at
some point? (after the sr if you want)

Comment 12

[[e-mail]]

Attachment: Updated for comments

Comment 13

[[e-mail]]

Comment on attachment 190315 [details] [diff] [review]
Updated for comments

suite-only patch.

Comment 14

[[e-mail]]

Fix checked in :-)

Comment 15

[Jason Bassford]

Excellent work!  This also fixed bug 300465 and bug 121969.