335068 - Firefox and Thunderbird windows grouped under "Mozilla" rather than in separate groups

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Tuukka Tolvanen (sp3000)]

In gnome window list 2.12.3, firefox and thunderbird windows are now all grouped under "Mozilla" rather than separately.

STR:
1. start Firefox and Thunderbird
2. open new firefox windows until window grouping occurs

Expected: Firefox windows grouped as e.g. "Firefox", not including Thunderbird windows

Actual: fx & tbird windows grouped into one "Mozilla" group

regression 2006-04-08-04-trunk firefox & 05-trunk thunderbird -- 2006-04-10-04-trunk firefox & thunderbird

blaming bug 29856 from the sound of it and the regression window

Comment 1

[Tuukka Tolvanen (sp3000)]

yeah, backing out bug 29856 seems to fix this for me.

Comment 2

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

Attachment: fix

This should fix it.

Comment 3

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

Tuukka, can you verify that this patch fixes the regression for you?

Comment 4

[Tuukka Tolvanen (sp3000)]

It doesn't...

If I change "Mozilla" in http://lxr.mozilla.org/mozilla/source/xpfe/appshell/src/nsXULWindow.cpp#1345
that's the window group name I get.

Comment 5

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

do you feel like whipping up a patch to replace that with the name taken from nsXULAppInfo?
(see http://lxr.mozilla.org/mozilla/source/chrome/src/nsChromeRegistry.cpp#2080 )

Comment 6

[Tuukka Tolvanen (sp3000)]

with appshell in tier 9 and toolkit incl. xulapp in tier 50 the code from nsChromeRegistry won't quite work out as such...

Comment 7

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

Hmm. There's got to be a way to do this. We might have to move nsIXULAppInfo somewhere else.

Comment 8

[Tuukka Tolvanen (sp3000)]

would getting brandShortName from brand.properties w/ nsIStringBundle be ok? that wfm for fx and tb anyhow... bsmedberg mentioned he'd been thinking about moving nsIXULAppInfo up into tier 9 somewhere

Comment 9

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

Yes, brandShortName sounds like a great solution. If you can't get the brandShortName then just use "Mozilla".

Comment 10

[Tuukka Tolvanen (sp3000)]

Attachment: patch1: brandShortName -> res_class

here we go ...mind you, as this causes the apps to group separately, the label shown in the window list is no longer picked up from WM_CLASS.res_class, but from WM_CLIENT_LEADER's WM_NAME, which makes it "Firefox" even when brandShortName is something else like "Minefield". That's consistent with the pre-regression behaviour though.

One question is what to do when windowType turns out blank -- this patch leaves it as is such that no action is taken, but that may leave res_class inconsistent with other windows of the app.

Comment 11

[Tuukka Tolvanen (sp3000)]

> from WM_CLIENT_LEADER's WM_NAME, which makes it "Firefox" even when

...as far as I can tell by poking with xprop, anyway; didn't see that string elsewhere.

Comment 12

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

I think you should set the appName even if windowType is empty.

Also I'd like the whole body (apart from "return NS_OK;") to be inside #ifdef MOZ_WIDGET_GTK2, so non-GTK2 platforms don't eat the cost of this (admittedly small).

Thanks!

Comment 13

[Tuukka Tolvanen (sp3000)]

Attachment: patch2: with gtk2 ifdefs, allow empty windowType

Comment 14

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

Is an empty string for res_name and role OK?

Comment 15

[Tuukka Tolvanen (sp3000)]

what would you like them to fall back on, besides "Popup" and "Toplevel"?

Comment 16

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

How about setting res_name to "Mozilla" and just not calling gdk_window_set_role if role is empty?

I really don't know this X WM stuff.

Comment 17

[Tuukka Tolvanen (sp3000)]

Attachment: patch3

Sets res_class to brandShortName rather than the "Mozilla" placeholder already with the gtk_window_set_wmclass calls before realize, so if the xul windowtype turns out missing, nothing needs to be done later. Role is only set if there is a xul windowtype. Adds the early gtk_window_set_wmclass for eWindowType_dialog which was omitted from the patch from bug 29856 (why?) -- otherwise dialogs without xul windowtype are left with the good old "Gecko", "Appname-bin". Narrows SetWindowClass back to one arg since the other one needs to be figured out before SetWindowClass anyway.

Comment 18

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

Comment on attachment 228652 [details] [diff] [review]
patch3

Thanks!

Comment 19

[Tuukka Tolvanen (sp3000)]

roc, checkin?

Comment 20

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

I can't check anything in for a while, my laptop died and I'm travelling.

Comment 21

[:Gavin Sharp [email: [e-mail]]]

mozilla/widget/src/xpwidgets/nsBaseWidget.h 	1.82
mozilla/widget/src/xpwidgets/nsBaseWidget.cpp 	1.146
mozilla/xpfe/appshell/src/nsXULWindow.cpp 	1.170
mozilla/widget/src/gtk2/nsWindow.h 	1.63
mozilla/widget/src/gtk2/nsWindow.cpp 	1.176
mozilla/widget/public/nsIWidget.h 	3.150