The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 15

Status

()

Core
Widget: Gtk
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

({regression})

unspecified
mozilla17
All
Linux
regression
Points:
---

Firefox Tracking Flags

(firefox14 wontfix, firefox15 fixed, firefox16 fixed, firefox17 fixed)

Details

(Whiteboard: [qa-])

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
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".
(Assignee)

Comment 1

5 years ago
As a matter of fact, it also sets the class to "Xulrunner-bin" for all xulrunner applications.
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.
(Assignee)

Comment 3

5 years ago
(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.
(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.
(Assignee)

Comment 5

5 years ago
(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.
(Assignee)

Comment 6

5 years ago
The most notable use of g_get_prgname is in gtk_recent_manager_add_item.
(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.
(Assignee)

Updated

5 years ago
Assignee: nobody → mh+mozilla
(Assignee)

Comment 8

5 years ago
Created attachment 644847 [details] [diff] [review]
Set gtk program name to the one defined in application.ini
Attachment #644847 - Flags: review?(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.
Attachment #644847 - Flags: review?(karlt) → review+
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?
(Assignee)

Comment 11

5 years ago
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 :-/

Updated

5 years ago
Keywords: regression

Updated

5 years ago
Duplicate of this bug: 741176
(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).
(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.
(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>.
(Assignee)

Comment 16

5 years ago
Created attachment 650456 [details] [diff] [review]
Set nsXREAppData::name instead of using g_set_prgname is Linux webapprt
Attachment #650456 - Flags: review?(karlt)
(Assignee)

Updated

5 years ago
status-firefox14: --- → wontfix
status-firefox15: --- → affected
status-firefox16: --- → affected
status-firefox17: --- → affected
Summary: WM_CLASS not set from branding anymore → WM_CLASS not set properly on xulrunner applications and apps where the app name is not the same as the executable name
(Assignee)

Comment 17

5 years ago
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?
Attachment #650456 - Flags: feedback?(mar.castelluccio)
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.
Attachment #650456 - Flags: review?(karlt) → review+
If I run the webapprt from the shell with the patch applied, the DE doesn't show the correct icon.
(Assignee)

Comment 20

5 years ago
(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 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.
Attachment #650456 - Flags: feedback?(mar.castelluccio) → feedback+
(Assignee)

Comment 22

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1adad380895
https://hg.mozilla.org/integration/mozilla-inbound/rev/b627fbbdf0e4
Target Milestone: --- → mozilla17
https://hg.mozilla.org/mozilla-central/rev/f1adad380895
https://hg.mozilla.org/mozilla-central/rev/b627fbbdf0e4
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 24

5 years ago
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
Attachment #644847 - Flags: approval-mozilla-beta?
Attachment #644847 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 25

5 years ago
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.
Attachment #650456 - Flags: approval-mozilla-beta?
Attachment #650456 - Flags: approval-mozilla-aurora?
Attachment #644847 - Flags: approval-mozilla-beta?
Attachment #644847 - Flags: approval-mozilla-beta+
Attachment #644847 - Flags: approval-mozilla-aurora?
Attachment #644847 - Flags: approval-mozilla-aurora+
Attachment #650456 - Flags: approval-mozilla-beta?
Attachment #650456 - Flags: approval-mozilla-beta+
Attachment #650456 - Flags: approval-mozilla-aurora?
Attachment #650456 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 26

5 years ago
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
(Assignee)

Updated

5 years ago
status-firefox15: affected → fixed
status-firefox16: affected → fixed
status-firefox17: affected → fixed
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.