223492 - Support startup notification on linux/unix desktops

Status: VERIFIED FIXED

(Firefox :: General, enhancement)

Description

[Keith Lea]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.5) Gecko/20031007 Firebird/0.7
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.5) Gecko/20031007 Firebird/0.7

Startup notification means indicating to the user that a program is starting up,
which is normally implemented by giving the user a busy cursor while it's loading.

The specification is at
http://freedesktop.org/Standards/startup-notification-spec. Many linux/unix
applications support this, and Firebird takes a few seconds to load, so it would
be useful.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Philip White]

Good idea.

Comment 2

[Murray Cumming]

In new versions of GNOME, the lack of startup-notification support also means
that  firefox will not have focus when launched from the panel.

http://bugzilla.gnome.org/show_bug.cgi?id=167847
has more about this, plus a (small) example of how to do it.

Comment 3

[Christopher Aillon (sabbatical, not receiving bugmail)]

Taking.

Comment 4

[Jose M Rodriguez]

Hi,

A last here, MoFo products have this working for the first direct instance,
but fails for the next.  This is way most distros disable this in the
.desktop file.

At last, for gtk2 builds, that allready include this.

The main problem is that xremote-client don't use this.

So, the first step maybe add support for startup-notification to xremote-client,
at last, for gtk2 builds.

The sources are under widget/src/xremoteclient, and the most simple approach
maybe add support for gnome startup-notify lib.

This may also add dependencies on libSM and libICE

Comment 5

[Hervé Cauwelier]

This bug is annoying with Firefox and Thunderbird under Ubuntu Gnome 2.11: you
want to send a URL by mail and the TB compose window is unfocused, wathever you
do. I'm afraid the beginner will try and try again because he wouldn't see
anything happen.

No, the bug is not in metacity. :-)

Comment 6

[Elijah Newren]

Jose: Right, the first instance works--but that's only because gtk+ does it
automatically for apps which don't forward open-window requests to previous
instances.  libSM and libICE shouldn't factor into this at all, though it could
add a dependency on libsn (sn = startup notification).

The bug that Murray pointed out only explains how to set apps up to be launched
with startup-notification, which doesn't help if the app does weird things that
requires extra support when running with startup-notification (like forwarding
open-window requests to a previous instance) and the app doesn't have that extra
support.  A better place to look is
http://mail.gnome.org/archives/nautilus-list/2005-February/msg00045.html where
it was discussed how to add this support to nautilus (some follow ups were part
of a different thread, I think, in March; just search for my name in the Archives)

I spent a little while trying to figure this out for Mozilla & Firefox, but had
a really hard time trying to track the startup process in order to find both
where the open-new-window request is sent and where it was received and how to
add extra information to the request.  I just spent another couple hours...could
someone help me out and verify if I'm finding the path right and suggest how to
pass the data appropriately?

It looks like the open-new-window request is a string sent as an X message from
XRemoteClient::DoSendCommand() which ends up in XRemoteService::ParseCommand(),
which through OpenURL() calls OpenChromeWindow(), which calls
<some-watcher-class>::OpenWindow() [is some-watcher-class == nsWindowWatcher? 
I'm just exhaustive grepping and then randomly picking here], which seems to go
to <who-knows-what-class>::CreateChromeWindow() or
<who-knows-what-class>::CreateChromeWindow2() [is who-knows-what-class ==
nsWindowCreator?], which could call any of who knows how many functions but I'm
guessing it's nsAppShellService::CreateTopLevelWindow() going to
JustCreateTopWindow() going to nsWebShell::Initialize() which calls various
things but I'm thinking the mWindow->Create call is the important one and I
believe that calls widget/src/gtk2/nsWindow.cpp:nsWindow::Create() which goes to
NativeCreate() which is where I can probably do all the startup notification
magic (is it okay to gtk_widget_realize() on the window in this function in
order to allow calls to sn_launchee_context_setup_window() and
gdk_x11_window_set_user_time()?).  I'm still not sure where the
"gtk_window_set_auto_startup_notification(FALSE)" call should go in a case like
this, though.  Maybe it isn't even needed.  But what's the best way of passing
the needed data (a startup id string and a 32-bit unsigned integer timestamp)
here?  Extend the nsWidgetInitData struct?

Comment 7

[Adam Guthrie]

*** Bug 301319 has been marked as a duplicate of this bug. ***

Comment 8

[Sasafrass]

How is this resolved? I still have this problem.

Comment 9

[Elijah Newren]

Sasafrass: Note that the status of this bug is "New", not "Resolved".

Comment 10

[Sasafrass]

Sorry about that, I thought I was posting at my own bug report that is
supposedly resolved. I don't know how that happened.

(In reply to comment #9)
> Sasafrass: Note that the status of this bug is "New", not "Resolved".

Comment 11

[Adam Guthrie]

*** Bug 311947 has been marked as a duplicate of this bug. ***

Comment 12

[Adam Guthrie]

*** Bug 319389 has been marked as a duplicate of this bug. ***

Comment 13

[William Cattey]

I just had this bug happen to me.
I was trying to look at a URL from inside Evolution.
I kept thinking the web browser wasn't starting,
but in fact it was that the new window was being stacked
under Evolution owing to the MetaCity window manager being
too catholic in how it stacks windows.

I understand this isn't a data loss bug, so it's not
all THAT severe, but I think it should be classified more
serious than "enhancement".  Lots of people are going to
thing Firefox and Evolution are broken because of this.

If it's not a terribly difficult behavor to correct,
I think it should be done.  (Hasn't GNOME and Firefox got
a bad enough rep about subtle issues of usability?)

-Bill Cattey
Linux Platform Coordinator
Massachusetts Institute of Technology

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Can anyone tell me when gdk_x11_set_user_time was added to GTK?

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

And shipped, I mean.

Comment 16

[Elijah Newren]

Robert: stable release or unstable release?  gdk_x11_window_set_user_time() first appeared in a stable version of gtk+ in gtk+-2.6.0 which was released 2004-12-16.  It existed in the unstable branch of gtk+ since 2004-05-05.  (It also existed as _gdk_x11_window_set_user_time() in gtk+ >= 2.4.1 since April 2004; it was unfortunately a little bit too late for gtk+-2.4.0 and thus missed the 2.4.x series).

If gtk+ >= 2.6.0 is too late a requirement for firefox, I can write a small local copy for you; I had to do that for several Gnome apps in Gnome 2.8.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'll just probe with dladdr to see if the symbol is there; I guess this bug just won't get fixed for people running with GTK < 2.6.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Here's the patch. It captures DESKTOP_STARTUP_ID and forwards it over XRemote. The way it does that isn't pretty ... I add " DESKTOP_STARTUP_ID=..." to the application name in argv[0]. I did it this way so that remotes to and from downlevel Firefox builds don't get confused.

The DESKTOP_STARTUP_ID timestamp is applied to the first real toplevel window that gets created next. This seems to work fine. In theory there could be races when multiple windows are being created that give the wrong window focus, but I think this is very low probability and not serious.

This patch also handles the non-remote case. I can now add StartupNotify=true to my Firefox.desktop file and it works great, the cursor spins and stops when the window comes up.

Since I'm mostly treading on bsmedberg code, I'm asking him for review. Elijah, it'd be great if you could do a sanity check too.

Comment 19

[Benjamin Smedberg]

Comment on attachment 221412 [details] [diff] [review]
fix

I won't be able to look at this for a couple weeks (after XTech).

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

No rush.

Comment 21

[Adam Guthrie]

When I tried to test this patch it didn't compile initially because it was using an undefined macro in XRemoteClient.ccp (NS_NAMED_LITERAL_CSTRING). I had to include nsStringAPI.h and add a few things to the Makefile.in to make this work.

This does not fix the "Firefox does not receive focus when links are opened from other windows" problem. (Even if the other app. supports startup notification as well; e.g. GNOME Terminal.)

I was under the impression that it would fix this (that's why I duped all these other bugs to it). This issue was mentioned in the GNOME 2.10 release notes: http://gnome.org/start/2.10/notes/rnknownissues.html (no idea why it's in French). If startup notification is not supposed to fix this, that's fine -- I can just file another bug -- but it would be great if it did!

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm working on a new patch that will actually fix it all :-).

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Updated patch. This patch improves on the last version in several ways. It uses libstartup-notification to do more things right. It signals startup-complete when the new window is shown, not just created. It can bring an existing window to the  front if we just remotely loaded a new tab into a window. It can forward DESKTOP_STARTUP_ID with -remote commands as well as full command lines. If a remote command/command line does not contain a DESKTOP_STARTUP_ID (it should, but many GNOME apps need to be modified to set DESKTOP_STARTUP_ID when they launch Firefox via gnome-vfs), we bring the new window to the front anyway.

This patch also makes the startup-notification part disabled by default; you have to --enable-startup-notification to get it, because the resulting build will not run unless libstartup-notification is present.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #23)
> If a remote command/command line does not contain a DESKTOP_STARTUP_ID (it
> should, but many GNOME apps need to be modified to set DESKTOP_STARTUP_ID when
> they launch Firefox via gnome-vfs), we bring the new window to the front
> anyway.

This is the part that fixes what Adam mentioned in comment #21.

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Comment 26

[Adam Guthrie]

Comment on attachment 222455 [details] [diff] [review]
fix

Requesting review on behalf of roc.

Comment 27

[Benjamin Smedberg]

Comment on attachment 222455 [details] [diff] [review]
fix

I can grant moa=bsmedberg for the build-config changes, but I can't review the GTK2 stuff; I don't know GTK.

Comment 28

[Brian Ryner (not reading)]

Comment on attachment 222455 [details] [diff] [review]
fix

I don't think I'll be able to get to this, sorry.

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 222455 [details] [diff] [review]
fix

Stuart, you're still listed as a GTK2 module owner, so let's see if you really are :-)

Comment 30

[Adam Guthrie]

So if I made a build with this patch in it and ran it for a week or so and didn't find anything wrong, could we just land it? :)

Comment 31

[Elijah Newren]

I printed this patch out and looked over it some weeks ago, but never got around to checking a few more things that I wanted to before commenting.  Luckily, I took some notes, so I'll try to add them here and see if they are useful.  Note that I'm not very familiar with the mozilla codebase or structure (and I didn't have time to figure out why my cvs builds of mozilla have failed in the recent past so I was unable to do real testing of the patch), so I'm just trying to comment from the gtk+ & EWMH/startup-notification standpoint of things.

I only caught a couple small things that looked problematic; overall it looks pretty good to me.

>+typedef void (* SetUserTimeFunc)(GdkWindow* aWindow, guint32 aTimestamp);
>+
>+static void
>+SetUserTimeAndStartupIDForActivatedWindow(GtkWidget* aWindow)

There's a proposed gtk+ function for handling basically this entire function; see http://bugzilla.gnome.org/show_bug.cgi?id=347375.  Of course, it hasn't yet been accepted to gtk+ HEAD, but it might be worth a FIXME comment for future people to come along and change.  Then again, there's some work being done on creating APIs (GUniqueApp/GtkUnique) for making this whole patch plus the general single-instance mechanism a lot easier to do, but that's even further from being finished.  This is basically just an FYI, I don't see any code changes needed because of this for now.


>+    if (desktopStartupID.IsEmpty()) {
>+        PRUint32 timestamp = GTKToolkit->GetFocusTimestamp();
>+        if (timestamp) {
>+            gdk_window_focus(aWindow->window, timestamp);
>+            GTKToolkit->SetFocusTimestamp(0);
>+        }
>+        return;
>+    }

This block of SetUserTimeAndStartupIDForActivatedWindow() is a little confusing.  It doesn't set the user time or the startup id for the activated window.  Instead, it sends an "activate me, please" message to the window manager.  I understand it's just a workaround when no startup id is available (because either firefox wasn't launched with startup-notification or wasn't built with MOZ_ENABLE_STARTUP_NOTIFICATION), but it'd be nice to have it documented as such.  It'd also be good to verify in testing that this block of code isn't being executed (i.e. that startup id is non-empty), since it is just a workaround.

Also note that this workaround is a bit of a race condition, since the timestamp used was just an approximation.  You can't really avoid a race entirely when no startup id is available, but you could get a better approximation to avoid the problems with races a bit better.  A more accurate timestamp approximation could be obtained in the second firefox instance and then a fake startup id could be created containing just that better approximation, with handling added in a couple places to deal with fake startup ids.  I'm not sure if it's worth the effort given that (a) the timestamp from the property notify event (nsGtkRemoteService::HandlePropertyChange()) should be a pretty decent approximation and (b) we probably want to eventually replace this code a few years down the road with GUniqueApp/GtkUnique (or whatever the final API is named when it's finished) -- which should automatically fix this anyway.


> NS_IMETHODIMP
> nsWindow::SetFocus(PRBool aRaise)
> {
<snip>
>+    if (toplevelWidget && aRaise) {
>+        SetUserTimeAndStartupIDForActivatedWindow(toplevelWidget);
>+    }

Here's one place where lack of understanding of the mozilla code base is hurting me.  Is SetFocus() called in response to javascript in web pages?  If so, this introduces a huge security bug.  See http://bugzilla.gnome.org/show_bug.cgi?id=164675#c10.  If that's the case, these three lines should just be removed; the raising/GetAttention calls will cause the user to be notified that the app wants focus.


>@@ -3054,34 +3127,39 @@ void
> nsWindow::NativeShow (PRBool  aAction)
<snip>
>         if (mIsTopLevel) {
>             moz_drawingarea_set_visibility(mDrawingarea, aAction);
>             gtk_widget_show(GTK_WIDGET(mContainer));
>             gtk_widget_show(mShell);
>+
>+            // Set up usertime/startupID metadata for the created window.
>+            if (mWindowType != eWindowType_invisible) {
>+                SetUserTimeAndStartupIDForActivatedWindow(mShell);
>+            }

This is backwards.  The timestamp and startup id should be set before showing the window.  A little explanation:

The current code:

1. Calls gtk_widget_show(mShell).  This causes the mShell window to be shown, which as a side-effect causes gtk+ to try to figure out what timestamp to use for the window.  Since you haven't called SetUserTimeAndStartupIDForActivatedWindow() yet, gtk+ is unaware of the messages mozilla is sending around due to the user interaction with the _other_ instance of mozilla that was launched.  So, gtk+ uses the last time the user interacted with _this_ instance, which is far too old.  metacity (or kwin) notices the new window mapped with a really old timestamp, and leaves the focus on some other window.

2. Calls SetUserTimeAndStartupIDForActivatedWindow(mShell).  This sets the user_time (via gdk_x11_window_set_user_time) and startup_id (via sn_launchee_context_setup_window) on mShell.  The former is useless since the window was already mapped and metacity determine whether to switch focus to a new window only when it is mapped.  The second sets a new startup id on the window, which more recent versions of metacity (or kwin) will treat as the window being launched for a second time.  So metacity goes through the steps needed to get the window re-launched and the focus will finally get transferred at this point, but it is a bit wasteful.  :-)

The correct order would be:

1. Call gtk_widget_realize(mShell).  This creates the relevant GdkWindow structures (and thus X window structures) for mShell.  You can basically think of it as making sure that mShell->window is not NULL.  This is necessary in order to have something to set the user time and startup id on.

2. Call SetUserTimeAndStartupIDForActivatedWindow(mShell), which in turn would set the user time and startup id on the X window.

3. Call gtk_widget_show(mShell).  This maps the window, and metacity/kwin notices the correct timestamp and startup id on the window and thus transfers focus to the window upon mapping it.  No second relaunch of the window either.  :)


> XRemoteClient::DoSendCommand(Window aWindow, const char *aCommand,
>+                             const char* aDesktopStartupID,
>                              char **aResponse, PRBool *aDestroyed)
<snip>
>+    len += sizeof(desktopStartupPrefix) - 1 + strlen(aDesktopStartupID);

This...

> XRemoteClient::DoSendCommandLine(Window aWindow, PRInt32 argc, char **argv,
>+                                 const char* aDesktopStartupID,
>                                  char **aResponse, PRBool *aDestroyed)
<snip>
>+      len += strlen(desktopStartupPrefix) + strlen(aDesktopStartupID);

is oddly inconsistent with this.  Why sizeof() in one case and strlen() in the other?  Not that it really matters, I was just curious...  :-)


>@@ -76,28 +76,30 @@ public:
>    * @param aCommand This is the command that is passed to the server.
>    * Please see the additional information located at:
>    * http://www.mozilla.org/unix/remote.html
>    *
>    * @param aResponse If there is a response, it will be here.  This
>    * includes error messages.  The string is allocated using stdlib
>    * string functions, so free it with free().
>    *
>    * @retun true if succeeded, false if no running instance was found.
>    */
>   virtual nsresult SendCommand(const char *aProgram, const char *aUsername,
>                                const char *aProfile, const char *aCommand,
>+                               const char* aDesktopStartupID,
>                                char **aResponse, PRBool *aSucceeded) = 0;

Shouldn't the documentation include a description of aDesktopStartupID?  Yeah, I know, I'm just being nit-picky...  :)


>+#if defined(HAVE_DESKTOP_STARTUP_ID) && defined(MOZ_TOOLKIT_GTK2)
>+      nsGTKToolkit* toolkit = GetGTKToolkit();
>+      if (toolkit) {
>+        nsCAutoString currentDesktopStartupID;
>+        toolkit->GetDesktopStartupID(&currentDesktopStartupID);
>+        if (!currentDesktopStartupID.IsEmpty()) {
>+          nsCAutoString desktopStartupEnv;
>+          desktopStartupEnv.AssignLiteral("DESKTOP_STARTUP_ID=");
>+          desktopStartupEnv.Append(currentDesktopStartupID);
>+          // Leak it with extreme prejudice!
>+          PR_SetEnv(ToNewCString(desktopStartupEnv));
>+        }
>+      }
>+#endif
>+

I don't understand what this is for or why it's being done.  Does firefox, when the first instance is being launched, do a whole bunch of startup stuff and then fork a child process to take its place or something?

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #31)
> I printed this patch out and looked over it some weeks ago, but never got
> around to checking a few more things that I wanted to before commenting. 
> Luckily, I took some notes, so I'll try to add them here and see if they are
> useful.

Thanks, this is very useful.

> There's a proposed gtk+ function for handling basically this entire function;
> see http://bugzilla.gnome.org/show_bug.cgi?id=347375.  Of course, it hasn't
> yet been accepted to gtk+ HEAD, but it might be worth a FIXME comment for
> future people to come along and change.

Sure.

> This block of SetUserTimeAndStartupIDForActivatedWindow() is a little
> confusing.

I'll add a comment about the workaround, but I don't plan to make it more precise.

> Here's one place where lack of understanding of the mozilla code base is
> hurting me.  Is SetFocus() called in response to javascript in web pages?  If
> so, this introduces a huge security bug.  See
> http://bugzilla.gnome.org/show_bug.cgi?id=164675#c10.  If that's the case,
> these three lines should just be removed; the raising/GetAttention calls will
> cause the user to be notified that the app wants focus.

This is so that remote Firefox requests to load a page in a new tab of an existing window will actually be able to raise the window in spite of Metacity's focus-stealing prevention. This is what users expect, not GetAttention-style blinking or whatever. I think it should be harmless because setting SetUserTimeAndStartupIDForActivatedWindow can't actually trigger a raise, can it? It will only help an raise request by the current code succeed, and that shouldn't happen inappropriately because we don't rely on Metacity's focus-stealing prevention for security. (Besides, SetUserTimeAndStartupIDForActivatedWindow only works once after the last remote request, so an attacker would find it hard to leverage.)

> This is backwards.  The timestamp and startup id should be set before showing
> the window.

OK thanks.

> > XRemoteClient::DoSendCommandLine(Window aWindow, PRInt32 argc, char **argv,
> >+                                 const char* aDesktopStartupID,
> >                                  char **aResponse, PRBool *aDestroyed)
> <snip>
> >+      len += strlen(desktopStartupPrefix) + strlen(aDesktopStartupID);
> 
> is oddly inconsistent with this.  Why sizeof() in one case and strlen() in
> the other?  Not that it really matters, I was just curious...  :-)

Good catch, that was just an optimization that I didn't do consistently.

> >+#if defined(HAVE_DESKTOP_STARTUP_ID) && defined(MOZ_TOOLKIT_GTK2)
> >+      nsGTKToolkit* toolkit = GetGTKToolkit();
> >+      if (toolkit) {
> >+        nsCAutoString currentDesktopStartupID;
> >+        toolkit->GetDesktopStartupID(&currentDesktopStartupID);
> >+        if (!currentDesktopStartupID.IsEmpty()) {
> >+          nsCAutoString desktopStartupEnv;
> >+          desktopStartupEnv.AssignLiteral("DESKTOP_STARTUP_ID=");
> >+          desktopStartupEnv.Append(currentDesktopStartupID);
> >+          // Leak it with extreme prejudice!
> >+          PR_SetEnv(ToNewCString(desktopStartupEnv));
> >+        }
> >+      }
> >+#endif
> >+
> 
> I don't understand what this is for or why it's being done.  Does firefox,
> when the first instance is being launched, do a whole bunch of startup stuff
> and then fork a child process to take its place or something?

Yes, sometimes it re-registers XPCOM components (or otherwise twiddle its configuration) and then restart itself from scratch.

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: updated fix

Chris, could you review this? It is probably something you want.

Comment 34

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 243267 [details] [diff] [review]
updated fix

Elijah, thanks for the comments.


>Index: toolkit/xre/nsAppRunner.cpp
>===================================================================
>RCS file: /home/rocallahan/mozilla-cvs-mirror/mozilla/toolkit/xre/nsAppRunner.cpp,v
>retrieving revision 1.145
>diff -u -t -p -1 -2 -r1.145 nsAppRunner.cpp
>--- toolkit/xre/nsAppRunner.cpp	27 Sep 2006 13:39:29 -0000	1.145
>+++ toolkit/xre/nsAppRunner.cpp	24 Oct 2006 09:58:55 -0000

>+#if defined(MOZ_WIDGET_GTK) || defined(MOZ_WIDGET_GTK2) || defined(MOZ_ENABLE_XREMOTE)
>+    // Stash DESKTOP_STARTUP_ID becaus gtk_init will clear it.
>+#define HAVE_DESKTOP_STARTUP_ID
>+    char* desktopStartupID = PR_GetEnv("DESKTOP_STARTUP_ID");
>+    if (desktopStartupID) {
>+      if (desktopStartupID[0] == 0) {
>+        desktopStartupID = nsnull;
>+      } else {
>+        desktopStartupID = strdup(desktopStartupID);
>+      }
>+    }

Hm, the above looks weird to me.  I missed the strdup() at first and was wondering how you weren't corrupting things when you called free() later.  Maybe clarify the comment.  Also, it looks somewhat better if you do something like:

if (desktopStartupID && desktopStartupID[0] != 0) {
  desktopStartupID = strdup(desktopStartupID);
} else {
  desktopStartupID = nsnull;
}

>@@ -2122,24 +2177,25 @@ XRE_main(int argc, char* argv[], const n
> 
> #if defined(MOZ_WIDGET_GTK2)
>     // g_set_application_name () is only defined in glib2.2 and higher.
>     PRLibrary *glib2 = nsnull;
>     _g_set_application_name_fn _g_set_application_name =
>       (_g_set_application_name_fn)PR_FindFunctionSymbolAndLibrary("g_set_application_name", &glib2);
>     if (_g_set_application_name) {
>       _g_set_application_name(gAppData->name);
>     }
>     if (glib2) {
>       PR_UnloadLibrary(glib2);
>     }
>+  gtk_window_set_auto_startup_notification(PR_FALSE);

gtk_window_set_auto_startup_notification wasn't added until GTK+2.2... hate to add another dlsym call, but I think we need one.  Though I really do think it might be worth bumping the minimum gtk2 requirement to 2.2, as we use quite a few things throughout the tree which are >= 2.2, and 2.2 is going back to e.g. RH Linux 9 and RHEL 3.0.  Maybe worth filing a bug for that.


>@@ -2522,21 +2585,40 @@ XRE_main(int argc, char* argv[], const n
>       // Ensure that these environment variables are set:
>       SaveFileToEnvIfUnset("XRE_PROFILE_PATH", profD);
>       SaveFileToEnvIfUnset("XRE_PROFILE_LOCAL_PATH", profLD);
> 
> #ifdef XP_MACOSX
>       if (gBinaryPath) {
>         static char kEnvVar[MAXPATHLEN];
>         sprintf(kEnvVar, "XRE_BINARY_PATH=%s", gBinaryPath);
>         PR_SetEnv(kEnvVar);
>       }
> #endif
> 
>+#if defined(HAVE_DESKTOP_STARTUP_ID) && defined(MOZ_TOOLKIT_GTK2)
>+      nsGTKToolkit* toolkit = GetGTKToolkit();
>+      if (toolkit) {
>+        nsCAutoString currentDesktopStartupID;
>+        toolkit->GetDesktopStartupID(&currentDesktopStartupID);
>+        if (!currentDesktopStartupID.IsEmpty()) {
>+          nsCAutoString desktopStartupEnv;
>+          desktopStartupEnv.AssignLiteral("DESKTOP_STARTUP_ID=");
>+          desktopStartupEnv.Append(currentDesktopStartupID);
>+          // Leak it with extreme prejudice!
>+          PR_SetEnv(ToNewCString(desktopStartupEnv));

Hooray for leaking with putenv.  Sigh...

>+        }
>+      }
>+#endif
>+
>       rv = LaunchChild(nativeApp, appInitiatedRestart);
>       return rv == NS_ERROR_LAUNCHED_CHILD_PROCESS ? 0 : 1;
>     }
>+
>+#ifdef HAVE_DESKTOP_STARTUP_ID
>+    free(desktopStartupID);
>+#endif

You're leaking if |needsRestart| which seems like it might be okay, but probably good to free it anyway.  Could be factored by moving the return out.

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: updated to comments

Picking an sr out of the air ... Mats?

Comment 36

[Mats Palmgren (inactive)]

Comment on attachment 244776 [details] [diff] [review]
updated to comments

Sorry for the late review :-(
I found a few nits while reading the patch, the rest looks good,
but I haven't actually compiled or tested it.  r=mats

> Index: widget/src/xremoteclient/mozilla-xremote-client.cpp
> +#include <prenv.h>

Doesn't seem necessary for the changes you're doing.


> Index: toolkit/components/remote/nsGTKRemoteService.cpp
> +#ifdef MOZ_WIDGET_GTK2
> +#include "nsGTKToolkit.h"
> +#endif
> ...
> +static nsGTKToolkit* GetGTKToolkit()

Wrap this function in "#ifdef MOZ_WIDGET_GTK2" too?


> Index: toolkit/components/remote/nsGTKRemoteService.h
> +  static const char* HandleCommand(char* aCommand, nsIDOMWindow* aWindow,
> +                                  PRUint32 aTimestamp);

Indent, two occurrences.


> Index: toolkit/xre/nsAppRunner.cpp
> +    // Stash ... memory becaus gtk_init will clear it.

s/becaus/because

Comment 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 244776 [details] [diff] [review]
updated to comments

I'll take that as an sr if that's OK :-)

Comment 38

[Mats Palmgren (inactive)]

Sure, if you say so ;-)
I'm a bit uncertain over my rights to r/sr any bug though...
According to http://www.mozilla.org/owners.html and
http://www.mozilla.org/hacking/reviewers.html I don't exist so I'm trying
to not overstep what I'm allowed to do...

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Huh, I was pretty sure you were an sr and had sr'ed my bugs in the past... You certainly *should* be!

Comment 40

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 244776 [details] [diff] [review]
updated to comments

Hunting for a super-reviewer...

Comment 41

[tor]

Comment on attachment 244776 [details] [diff] [review]
updated to comments

A few nits:

If we were dealing with a more friendly source control system, I'd say rename nsToolkit.cpp to nsGTKToolkit.cpp.

I assume you'll be removing nsToolkit.h when checking in?

GetMainWidget() and GetGTKToolkit() are duplicated in toolkit/components/remote/nsGTKRemoteService.cpp and toolkit/xre/nsAppRunner.cpp.  Could they be moved into static methods of nsGTKToolkit or some other place where the definitions could be shared?

Comment 42

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't think I can link statically to stuff in widget.

Comment 43

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Checked in.

Comment 44

[Andrew Schultz]

FYI,  I fixed xpfe bustage from this with
   rv = client.SendCommand(program, username, profile, remote,
-                          &response, &success);
+                          nsnull, &response, &success);

Comment 45

[Adam Guthrie]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a3pre) Gecko/20070217 Minefield/3.0a3pre

Opening links from both GNOME Terminal and Thunderbird now bring Firefox into focus; e.g. make it the front-most window and unminimize it if it's minimized.

Thanks for fixing this, roc. Also thanks to all the reviewers.

->VERIFIED

Comment 47

[Colin Walters]

Attachment: startup notification fixes backported to 2.0

I found this bug to be too annoying to stand waiting for Firefox 3 to get it, so I've backported the patch to the Firefox 2 codebase (I believe this is MOZILLA_1_8 branch?).

I did development from the Fedora firefox package (firefox-2.0.0.4 + some patches).  If desired I can take a look at making sure the patch applies cleanly and works on the 2.0 CVS.  But for now I'm just placing this patch here so other interested parties (perhaps 2.0 distributors) could consider it.

Comment 48

[Colin Walters]

Comment on attachment 271137 [details] [diff] [review]
startup notification fixes backported to 2.0

Requesting review to see if this could make it in to the 2.0 stream.

For reviewers - obviously this is just a backport of Robert's work, I didn't do anything original here.  

Almost all the fixes were trivial, with the exception of GetMainWidget.  There was no GetDocShell method on nsPIDOMWindow, but from looking at other places in the code I was able to replace that by GetExtantDocument and GetContainer.

Comment 49

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 271137 [details] [diff] [review]
startup notification fixes backported to 2.0

>--- toolkit/components/remote/nsGTKRemoteService.cpp	2006-01-05 22:19:20.000000000 -0500
>+++ toolkit/components/remote/nsGTKRemoteService.cpp	2007-07-05 17:34:41.000000000 -0400

>@@ -155,20 +163,46 @@
>   return PL_DHASH_NEXT;
> }
> 
>+static nsIWidget* GetMainWidget(nsIDOMWindow* aWindow)
>+{
>+  // get the native window for this instance
>+  nsCOMPtr<nsPIDOMWindow> window(do_QueryInterface(aWindow));
>+  NS_ENSURE_TRUE(window, nsnull);
>+  nsCOMPtr<nsIDocument> doc(do_QueryInterface(window->GetExtantDocument()));
>+  NS_ENSURE_TRUE(doc, nsnull);
>+  nsCOMPtr<nsISupports> container = doc->GetContainer();
>+  nsCOMPtr<nsIBaseWindow> baseWindow(do_QueryInterface(container));
>+  NS_ENSURE_TRUE(baseWindow, nsnull);
>+ 
>+   nsCOMPtr<nsIWidget> mainWidget;
>+   baseWindow->GetMainWidget(getter_AddRefs(mainWidget));
>+  return mainWidget;

The spacing above is a little wonky.

>+}
>+
>+static nsGTKToolkit* GetGTKToolkit()
>+{
>+  nsCOMPtr<nsIAppShellService> svc = do_GetService(NS_APPSHELLSERVICE_CONTRACTID);
>+  if (!svc)
>+    return nsnull;
>+  nsCOMPtr<nsIDOMWindowInternal> window;
>+  svc->GetHiddenDOMWindow(getter_AddRefs(window));
>+  if (!window)
>+    return nsnull;
>+  nsIWidget* widget = GetMainWidget(window);
>+  if (!widget)
>+    return nsnull;
>+  nsIToolkit* toolkit = widget->GetToolkit();
>+  if (!toolkit)
>+    return nsnull;
>+ return NS_STATIC_CAST(nsGTKToolkit*, toolkit);

And here again for the return.

Looks fine otherwise, and I agree this would make Firefox 2.0.0.x on Linux a much better experience.  r=caillon.

Comment 50

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Looks OK but Colin, I original did this on the 1.8 branch, it shipped with SUSE's Firefox. Do you want to have a look at the SUSE source RPM and check what i did with GetMainWidget? I don't remember myself...

Comment 51

[Colin Walters]

I wanted to comment on this bug; we integrated this patch for Fedora 8, and a bit of a tempest in a teapot has resulted.

The primary issue seems to be that for people using multiple workspaces on Linux, popular window managers such as Metacity and Compiz do not behave as these people would like.  In a single workspace case, this patch creates the expected effect that clicking on a link will show that link.  I see no issue there, and most people seem to not disagree.

Metacity: https://bugzilla.redhat.com/show_bug.cgi?id=307581
Compiz: https://bugzilla.redhat.com/show_bug.cgi?id=339711
Upstream Metacity: http://bugzilla.gnome.org/show_bug.cgi?id=482354

Executive summary here is that I think the Firefox patch is basically correct (though Bill pointed out some potential issues I'm going to investigate), but Firefox may be blamed for the change when the conclusion seems to be this is a window manager issue, so I wanted to comment here.

Comment 52

[Walter Meinl]

For the sake of building with gcc-4.2 (on amd64) you'll probably should add

#ifdef MOZ_ENABLE_STARTUP_NOTIFICATION
libsn/sn.h
libsn/sn-common.h
libsn/sn-launchee.h
libsn/sn-launcher.h
libsn/sn-monitor.h
libsn/sn-util.h
#endif
to config/system-headers
see bug391937 

Comment 53

[Robert O'Callahan (:roc) (email my personal email if necessary)]

If you give that to me in patch format I can review it and land it :-)

Comment 54

[Walter Meinl]

Attachment: system-headers patch for the backport

here it is

Comment 55

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 287338 [details] [diff] [review]
system-headers patch for the backport

thanks

Comment 56

[Walter Meinl]

Comment on attachment 287338 [details] [diff] [review]
system-headers patch for the backport

This is only for the branch, the trunk has it already, sorry for the confusion, I reset the 1.9 approval flag and ask for approval for 1.8.10

Comment 57

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 287338 [details] [diff] [review]
system-headers patch for the backport

Oh. We haven't landed the startup notification patch on the Firefox 2 branch, so this patch is not needed there. Maybe your distro needs it.

Comment 58

[Walter Meinl]

(In reply to comment #57)
> (From update of attachment 287338 [details] [diff] [review])
> Oh. We haven't landed the startup notification patch on the Firefox 2 branch,
> so this patch is not needed there. Maybe your distro needs it.
> 
Ok, but now its all together in case you'll land the branch patch.