726479 - Change startup notification code to use GTK APIs

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect, P3)

Description

[Karl Tomlinson (:karlt)]

SetUserTimeAndStartupIDForActivatedWindow in gtk2/nsWindow.cpp contains code
for setting the focus timestamp and stopping the startup notification
animation, after use of the remote protocol.

The code is currently not built by default, possibly because of the dependency
on libstartup-notification-1.so.0 that this would add.

Since GTK+-2.12, GTK+ offers the necessary apis.
https://bugzilla.gnome.org/show_bug.cgi?id=347375
Switching to the GTK APIs will mean this code can be enabled by default
without any extra dependency.

Although running Mozilla builds is only supported against GTK+-2.12 or newer,
they are built against GTK+-2.10.  That means Mozilla builds currently can't be linked against the gtk_window_set_startup_id symbol.  Until bug 713802 is fixed, setting the startup-id property may be simpler than finding gtk_window_set_startup_id at runtime.

Comment 1

[u575260]

ftw mozila 2017!
https://bugzilla.mozilla.org/show_bug.cgi?id=635134

Comment 2

[iofelben]

Attachment: Bug 726479 - Use GTK's support for startup notifications

Review commit: https://reviewboard.mozilla.org/r/112572/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/112572/

Comment 3

[Karl Tomlinson (:karlt)]

Comment on attachment 8837452 [details]
Bug 726479 - Use GTK's support for startup notifications

https://reviewboard.mozilla.org/r/112572/#review115316

That's for looking at this.  It will be nice to remove all this boilerplate.

However, this patch is changing behavior, and I assume that is not intentional.

::: widget/gtk/nsWindow.cpp
(Diff revision 1)
> -        if (sn_launchee_context_get_id_has_timestamp(ctx)) {
> -            gdk_x11_window_set_user_time(gdkWindow,
> -                sn_launchee_context_get_timestamp(ctx));
> -        }
> -
> -        sn_launchee_context_setup_window(ctx, gdk_x11_window_get_xid(gdkWindow));

gdk_notify_startup_complete_with_id() won't set the window user-time or startup id for us, but merely indicates that the application has started.  (It doesn't know the window.)

The user-time is required to make the app work with focus-stealing prevention when the window is shown.  I'm guessing the window manager may wish to use the startup id property.

gtk_window_set_startup_id() will set these appropriately.  (It is safe to assume this function is available now.)  Call that before gdk_notify_startup_complete_with_id() to keep the order the same.

Comment 4

[iofelben]

Looks like we don't need the gdk_notify_startup_complete_with_id() because gtk_window_set_startup_id() calls it, even on GTK2: https://github.com/GNOME/gtk/blob/gtk-2-24/gtk/gtkwindow.c#L1565

Comment 5

[iofelben]

Comment on attachment 8837452 [details]
Bug 726479 - Use GTK's support for startup notifications

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112572/diff/1-2/

Comment 6

[Karl Tomlinson (:karlt)]

Comment on attachment 8837452 [details]
Bug 726479 - Use GTK's support for startup notifications

https://reviewboard.mozilla.org/r/112572/#review118070

(In reply to iofelben from comment #4)
> Looks like we don't need the gdk_notify_startup_complete_with_id() because
> gtk_window_set_startup_id() calls it, even on GTK2:
> https://github.com/GNOME/gtk/blob/gtk-2-24/gtk/gtkwindow.c#L1565

But only when !disable_startup_notification and there is
http://searchfox.org/mozilla-central/rev/31b6089ce26fa76459642765115605d50a6c67b4/toolkit/xre/nsAppRunner.cpp#3869

Perhaps that can be removed now?

Comment 7

[Karl Tomlinson (:karlt)]

Comment on attachment 8837452 [details]
Bug 726479 - Use GTK's support for startup notifications

https://reviewboard.mozilla.org/r/112572/#review118070

But if only doing that, then showing a GTK dialog (e.g. save as, or print) even long after startup would gdk_notify_startup_complete(), which would broadcast a startup message using the id in the environment again.

I suspect the SetDesktopStartupID() call from nsAppRunner.cpp can be removed and GDK will use the id in the environment for the first window shown (whether it be a Gecko window or not).

Removing gtk_window_set_auto_startup_notification(false) will have the advantage of broadcasting the startup message even when the profilemanager is shown.

Comment 8

[Karl Tomlinson (:karlt)]

Comment on attachment 8837452 [details]
Bug 726479 - Use GTK's support for startup notifications

https://reviewboard.mozilla.org/r/112572/#review118094

::: widget/gtk/nsWindow.cpp:1309
(Diff revision 2)
>  // This will become obsolete when new GTK APIs are widely supported,
>  // as described here: http://bugzilla.gnome.org/show_bug.cgi?id=347375

Can you remove this comment, please?

Comment 9

[Michael Monreal [:micmon]]

I ran into some weird behavior after creating a custom application launcher for Firefox Nightly which included "StartupNotify=true" (which seems to be the default on modern Linux desktop apps). If I read this bug correctly, "StartupNotify=true" is currently not supported on official builds from Mozilla?

Comment 10

[Karl Tomlinson (:karlt)]

Yes, official builds still don't have --enable-startup-notification AFAIK.

Comment 11

[Robert Mader [:rmader]]

GTK2 support has been dropped from FF for a while now, so I gave this a try and it still applies almost cleanly and seems to work quite nicely. No loading spinner when launching the Wayland backend, immediately shows up in the alt-tab swither - great! iofelben, mind rebasing this so we can finally land it? If not, I can do so instead.

Comment 12

[Martin Stránský [:stransky] (ni? me)]

(In reply to robert.mader from comment #11)

GTK2 support has been dropped from FF for a while now, so I gave this a try and it still applies almost cleanly and seems to work quite nicely. No loading spinner when launching the Wayland backend, immediately shows up in the alt-tab swither - great! iofelben, mind rebasing this so we can finally land it? If not, I can do so instead.

Karl, are you ok to land this?

Comment 13

[Karl Tomlinson (:karlt)]

(In reply to Martin Stránský [:stransky] from comment #12)

Karl, are you ok to land this?

The changes requested in comments 6 to 8 need to be made to https://hg.mozilla.org/mozreview/gecko/rev/c8f74d891198eaa04a4bb0706dafa8fac8c9530d before that can land.

Comment 14

[Robert Mader [:rmader]]

Attachment: Bug 726479 - Use GTK's support for startup notifications, r=karlt

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f9cf5980bce9
Use GTK's support for startup notifications, r=karlt

Comment 16

[Robert Mader [:rmader]]

Right...ah...I hope it was ok to checkin so late in the cycle?

Comment 17

[Martin Stránský [:stransky] (ni? me)]

(In reply to robert.mader from comment #16)

Right...ah...I hope it was ok to checkin so late in the cycle?

I think it's ok, it may be committed to 74.

Comment 18

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/f9cf5980bce9

Comment 20

[David Heidelberg [:okias]]

Firefox 73.0 patched with this works well.

If anyone from dists packaging interested, I recommend applying patch a top of 73! :)