776325 - WM_CLASS not set properly on xulrunner applications and apps where the app name is not the same as the executable name

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Mike Hommey [:glandium]]

Bug 496653 allowed the --class option to work, but at the same time, didn't set a default using the branding, like it was before, but uses the gdk program name, which is, by default, the executable name.

For backwards compatibility, Iceweasel's executable is "firefox-bin", so Iceweasel's WM_CLASS is now "Navigator", "Firefox-bin" instead of "Navigator", "Iceweasel".

Comment 1

[Mike Hommey [:glandium]]

As a matter of fact, it also sets the class to "Xulrunner-bin" for all xulrunner applications.

Comment 2

[Karl Tomlinson (:karlt)]

Bug 737791 pointed out that WM_CLASS is ISO Latin-1 and shouldn't be localized, but brandShortName is localized.

Is there still reason to use bin/firefox-bin instead of bin/firefox?

I had assumed that xulrunner applications were started with a stub executable that would be named differently for each app.

Comment 3

[Mike Hommey [:glandium]]

(In reply to Karl Tomlinson (:karlt) from comment #2)
> Bug 737791 pointed out that WM_CLASS is ISO Latin-1 and shouldn't be
> localized, but brandShortName is localized.

We could use the name declared in application.ini data. Sadly, the gtk api is not very helpful: if you gdk_set_program_class before calling gtk_parse_args, gtk_parse_args overrides whatever you set, and if you call it after, you break handling of the --class argument. I guess the only way is to prepend { "--class", aAppData->name } to argv.

> Is there still reason to use bin/firefox-bin instead of bin/firefox?

Backwards compatibility. Some desktop managers like to keep "firefox-bin" in the launchers users create.

> I had assumed that xulrunner applications were started with a stub
> executable that would be named differently for each app.

You can launch xulrunner apps with "xulrunner /path/to/application.ini". I know that's what conkeror does.

Comment 4

[Karl Tomlinson (:karlt)]

(In reply to Mike Hommey [:glandium] from comment #3)
> We could use the name declared in application.ini data. Sadly, the gtk api
> is not very helpful: if you gdk_set_program_class before calling
> gtk_parse_args, gtk_parse_args overrides whatever you set, and if you call
> it after, you break handling of the --class argument.

g_set_prgname is effective at setting the default for WM_CLASS even when used before gtk_parse_args.  It is probably more appropriate too, as it sets defaults for both the name and class fields.

Comment 5

[Mike Hommey [:glandium]]

(In reply to Karl Tomlinson (:karlt) from comment #4)
> (In reply to Mike Hommey [:glandium] from comment #3)
> > We could use the name declared in application.ini data. Sadly, the gtk api
> > is not very helpful: if you gdk_set_program_class before calling
> > gtk_parse_args, gtk_parse_args overrides whatever you set, and if you call
> > it after, you break handling of the --class argument.
> 
> g_set_prgname is effective at setting the default for WM_CLASS even when
> used before gtk_parse_args.  It is probably more appropriate too, as it sets
> defaults for both the name and class fields.

But I'm not sure what over side effects g_set_prgname has. GTK seems to rely on its value is several places, and who knows what else does.

Comment 6

[Mike Hommey [:glandium]]

The most notable use of g_get_prgname is in gtk_recent_manager_add_item.

Comment 7

[Emmanuele Bassi (:ebassi)]

(In reply to Mike Hommey [:glandium] from comment #5)
> > g_set_prgname is effective at setting the default for WM_CLASS even when
> > used before gtk_parse_args.  It is probably more appropriate too, as it sets
> > defaults for both the name and class fields.
> 
> But I'm not sure what over side effects g_set_prgname has. GTK seems to rely
> on its value is several places, and who knows what else does.

g_set_prgname() is definitely what we in the gtk+ team recommend people using in order to describe the binary name, just like g_set_application_name() is what we suggest for the translatable, user visible name.

in general, g_set_prgname() should also match the name of the desktop file, so that window managers that do application tracking can match the WM_CLASS to the desktop file:

https://live.gnome.org/GnomeShell/ApplicationBased

(In reply to Mike Hommey [:glandium] from comment #6)
> The most notable use of g_get_prgname is in gtk_recent_manager_add_item.

you can override that by using gtk_recent_manager_add_full() and set the GtkRecentData.app_name yourself, if you want to maintain backward compatibility with the old name. I am not sure it's worth it, though: the name is used to do filtering on the list when displaying it (inside the file selection dialog, mostly) but there aren't many consumers of the GtkRecent API that do filtering on application name.

Comment 8

[Mike Hommey [:glandium]]

Attachment: Set gtk program name to the one defined in application.ini

Comment 9

[Karl Tomlinson (:karlt)]

Comment on attachment 644847 [details] [diff] [review]
Set gtk program name to the one defined in application.ini

This is the same as what the remote command handling uses to get the command name, so this looks like the right approach.

Comment 10

[Karl Tomlinson (:karlt)]

Marco, the proposed patch will override the prgname set in webapprt.cpp.
Where is the .desktop file created for the webapp?
Is the owa- prefix necessary?

Comment 11

[Mike Hommey [:glandium]]

Mmmmm now that i think of it, it will be a problem, because it's supposedly forbidden to call this function twice. And more than webapprt, third party embedders might well be using g_set_prgname :-/

Comment 13

[Marco Castelluccio [:marco]]

(In reply to Karl Tomlinson (:karlt) from comment #10)
> Where is the .desktop file created for the webapp?
> Is the owa- prefix necessary?

The desktop entry file is created under XDG_DATA_HOME/applications. We can get rid of the prefix if we need to (I added it just because it was an xdg-utils advice).

Comment 14

[Karl Tomlinson (:karlt)]

(In reply to Mike Hommey [:glandium] from comment #11)
> Mmmmm now that i think of it, it will be a problem, because it's supposedly
> forbidden to call this function twice.

The comment "Note that for thread-safety reasons this function can only be called once" was added in 
http://git.gnome.org/browse/glib/commit?id=10520a9228e11c6b9083eb96948d7ed5e40bda4b

I can see that there could be a problem if a thread has just called g_get_prgname while g_set_prgname is being called, but that situation doesn't apply here.

The set-once restriction is enforced for g_set_application_name, but not g_set_prgname, perhaps for backward compat.

> And more than webapprt, third party
> embedders might well be using g_set_prgname :-/

Anyone using g_set_prgname and gtk runs the risk of it being called twice because gtk will call g_set_prgname iff the --name parameter is provided.

Adding g_set_prgname to nsAppRunner is essentially requiring xulrunner apps to set name appropriately in nsXREAppData, but this is already necessary for remote commands.

I don't know what non-xulrunner embedding APIs we support now, but I assume they don't use nsAppRunner.

Comment 15

[Karl Tomlinson (:karlt)]

(In reply to Marco Castelluccio from comment #13)
> We can get rid of the prefix if we need to (I added it just because it was an
> xdg-utils advice).

Another option is to set nsXREAppData::name to owa-<profile>.

Comment 16

[Mike Hommey [:glandium]]

Attachment: Set nsXREAppData::name instead of using g_set_prgname is Linux webapprt

Comment 17

[Mike Hommey [:glandium]]

Comment on attachment 650456 [details] [diff] [review]
Set nsXREAppData::name instead of using g_set_prgname is Linux webapprt

Marco, can you double check this works as intended?

Comment 18

[Karl Tomlinson (:karlt)]

Comment on attachment 650456 [details] [diff] [review]
Set nsXREAppData::name instead of using g_set_prgname is Linux webapprt

>     }
> 
>+
>     nsCOMPtr<nsIFile> directory;

An extra blank line has slipped in here, accidentally, I assume.

Comment 19

[Marco Castelluccio [:marco]]

If I run the webapprt from the shell with the patch applied, the DE doesn't show the correct icon.

Comment 20

[Mike Hommey [:glandium]]

(In reply to Marco Castelluccio from comment #19)
> If I run the webapprt from the shell with the patch applied, the DE doesn't
> show the correct icon.

Did you apply both patches? The second relies on the first one.

Comment 21

[Marco Castelluccio [:marco]]

Comment on attachment 650456 [details] [diff] [review]
Set nsXREAppData::name instead of using g_set_prgname is Linux webapprt

(In reply to Mike Hommey [:glandium] from comment #20)
> Did you apply both patches? The second relies on the first one.

Oh, sorry, it works correctly.

Comment 22

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f1adad380895
https://hg.mozilla.org/integration/mozilla-inbound/rev/b627fbbdf0e4

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f1adad380895
https://hg.mozilla.org/mozilla-central/rev/b627fbbdf0e4

Comment 24

[Mike Hommey [:glandium]]

Comment on attachment 644847 [details] [diff] [review]
Set gtk program name to the one defined in application.ini

[Approval Request Comment]
Regression caused by bug 496653
User impact if declined: xulrunner applications run with xulrunner or firefox -app share the same window class as firefox, and may mixed up with firefox, or with each other by desktop managers.
Testing completed (on m-c, etc.): the resulting builds have been verified to have the right behavior for webapprt, firefox, and xulrunner apps
Risk to taking this patch (and alternatives if risky): Low
String or UUID changes made by this patch: None

Comment 25

[Mike Hommey [:glandium]]

Comment on attachment 650456 [details] [diff] [review]
Set nsXREAppData::name instead of using g_set_prgname is Linux webapprt

Webapprt part of the fix. See previous comment for actual approval request comment.

Comment 26

[Mike Hommey [:glandium]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a78609c4e41d
https://hg.mozilla.org/releases/mozilla-aurora/rev/4b8725e713c4

Beta needs a different patch for webapprt, but we actually don't enable webapprt on beta, so I just pushed the nsAppRunner part:

https://hg.mozilla.org/releases/mozilla-beta/rev/59e1040773a1