29856 - *nix only : Window Class the same for all mozilla windows

Status: RESOLVED FIXED

(Core :: XUL, defect, P4)

Description

[Brad Behm]

Under Linux (and presumable other *nixes), certain window managers (ie. Sawmill)
allow a user to select window preferences based on the 'window class'.

I use this feature to allow Mozilla to start full screen.

Unfortunately, all mozilla windows use the same window class, so JavaScript
alerts, error popups, JavaScript popups, the preferences dialog, and the
bookmark editor (to name a few) come up fullscreen, too.

This could be easily fixed by having child windows use a different window class.

Thanks

Comment 1

[Asa Dotzler [:asa]]

-> XP Miscellany, over to waterson for a look.

Comment 2

[Chris Waterson]

toolkit

Comment 3

[Peter Trudelle]

reassigning to danm as p4 for m18, cc pavlov, put on helpwanted radar

Comment 4

[Peter Trudelle]

Mass moving M18 bugs to M19

Comment 5

[Peter Trudelle]

mass-moving all bugs to m21 that are not dogfood+ or nsbeta2+ or nsbeta2-

Comment 6

[Andrew Taylor]

Attachment: patch that sets the window class on toplevel windows and dialogs

Comment 7

[Andrew Taylor]

The patch I submitted sets the class on toplevel windows and dialogs.  For all
dialogs, the class is dialog, Mozilla and for toplevels (browser, mailnews) it
is toplevel, Mozilla.

Ideally, to be like 4.x, name part should be Navigator, Mail, etc. and dialogs
should have a symbolic name like openURLDialog_popup or whatever, but I couldn't
figure out how to access that info from nsWindow without a major change.

But for now, your dialogs won't come up fullscreen, but there is still no way to
distinguish a browser window from a mail window or a irc window.

Comment 8

[R.K.Aa.]

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

Comment 9

[R.K.Aa.]

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

Comment 10

[R.K.Aa.]

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

Comment 11

[Dan M]

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

Comment 12

[Dan M]

  See this is what happens when you start ignoring your Bugzilla mail. An 
interesting patch has been languishing for half a year now. Seems like the patch 
would be a helpful partial solution, so I'm setting to a real milestone for 
consideration. Sorry, Andrew, for not noticing earlier.
  A more interesting version could perhaps synthesize a class from the XUL 
Window's "windowtype" attribute; that would distinguish between the browser and 
mail windows, for instance. It would also require an interface change. Something 
to think about, though.

Comment 13

[Dan M]

  Reassigning to Arik. Say thanks, Arik.
  Building on the above patch and my less above suggestion, we need a bit more 
granularity; different classes for different kinds of top-level windows. I 
suggest you key off of the |windowtype| attribute in the <window> DOM element 
defining the XUL window. To do this you'll have to wait until the XUL window has 
loaded (well after the gtk widget has been created), grab the attribute from the 
DOM element, and ship it off to the widget. Hopefully it's not too late by then.
  A good string to use would be the window type, taken from the windowtype 
attribute we place on most <window> XUL elements. In navigator.xul, for instance, 
it's "navigator:browser". Sometime after the XUL file has loaded (well after the 
gtk widget was created), grab the |windowtype| attribute from the window DOM 
element, and send it to SetWindowClass. There presumably you'll use the window 
type string to calculate some suitable class ID.
  Best to do all this from nsXULWindow, after the chrome has loaded. See 
nsXULWindow::LoadPositionAndSizeFromXUL method as a guide. It's called just after 
the chrome is loaded, and it pulls attributes out of the <window> DOM element.
  To get this string to the widget, I suggest you add a new method to the 
interface, nsIWidget::SetWindowClass, or some such. Give it a stubbed 
implementation in nsBaseWidget.cpp and a real implementation in the gtk version 
(gtk/nsWidget.cpp or gtk/nsWindow.cpp).

Comment 14

[arik devens]

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

Comment 15

[arik devens]

Attachment: this merges rob ginda and my patches and starts to add support to not persist js popup windows, not done yet

Comment 16

[arik devens]

not quite done yet but it's a start

Comment 17

[Brendan Eich [:brendan]]

Indentation is wacky:

     NS_IMETHOD CaptureMouse(PRBool aCapture) = 0;
 
+  NS_IMETHOD SetWindowClass (char *aClass) = 0;
+

Else after return is a non-sequitur -- don't do it, man!

+    return NS_OK;
+  } else
+    return NS_ERROR_FAILURE;

Can't you use NS_LITERAL_STRING here, as above for the GetAttribute?

+    docShellElement->SetAttribute(NS_ConvertASCIItoUCS2("persist"),
persistString);

The ?: expressions selecting PR_TRUE or PR_FALSE are unnecessary here:

+  if (aPersistPosition)
+    *aPersistPosition = persistString.Find("screenX") >= 0 ||
persistString.Find("screenY") >= 0 ? PR_TRUE : PR_FALSE;
+  if (aPersistSize)
+    *aPersistSize = persistString.Find("width") >= 0 ||
persistString.Find("height") >= 0 ? PR_TRUE : PR_FALSE;
+  if (aPersistSizeMode)
+    *aPersistSizeMode = persistString.Find("sizemode") >= 0 ? PR_TRUE :
PR_FALSE;

Just use the condition before the ? as the boolean out param value.

Similar deal here, just print PRBools as ints -- but more important: why are
these printfs not #ifdef DEBUG or (better) #ifdef DEBUG_arik?

+
+    printf ("aPersistPosition: %d\n", aPersistPosition == PR_FALSE ? 1 : 0);
+    printf ("aPersistSize: %d\n", aPersistSize == PR_FALSE ? 1 : 0);
+    printf ("aPersistSizeMode: %d\n", aPersistSizeMode == PR_FALSE ? 1 : 0);

They should not be compiled in normally.  Best if they're just removed, I think.

/be

Comment 18

[arik devens]

erk, i forgot to remove all sorts of debug and test code before i posted this
patch, will post a new one without it, thanks brendan

Comment 19

[Jesse Ruderman]

Even though this bug is Linux-only, will it be able to share code with bug 
57576, "Apps in the suite should use different/distinct icons"?

Comment 20

[Boris Zbarsky [:bzbarsky]]

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

Comment 21

[Braden]

Poke. It's been 2 months since Arik said he was going to remove the debug stuff
from the patch. Did other problems with this surface?

Comment 22

[Boris Zbarsky [:bzbarsky]]

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

Comment 23

[Boris Zbarsky [:bzbarsky]]

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

Comment 24

[R.K.Aa.]

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

Comment 25

[Asa Dotzler [:asa]]

Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 
(you can query for this string to delete spam or retrieve the list of bugs I've 
moved)

Comment 26

[R.K.Aa.]

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

Comment 27

[R.K.Aa.]

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

Comment 28

[timeless]

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

Comment 29

[John Morrison]

Sheesh. Jag can you review the patches on this bug and decide whether they 
do the right thing.

Comment 30

[Stuart Ballard]

Just found this bug; based on the date and content of the last comment it seems
like another "hey, what's up with this" is due...

Comment 31

[jag (Peter Annema)]

Address brendan's comments, fix the leak caused by use of ToNewCString() where
.get() could/should be used, attach a new patch and get a review.

-> 1.1alpha, though it's not high on my priority list (so finding another owner,
or at least someone to update the patch, might be a good idea if you're in a
hurry to get this fixed).

Comment 32

[hugo van woerkom]

It's all Mozilla-bin

Comment 33

[David G King]

I don't fully understand the inaction on this bug (before anyone asks, I don't
have access to *nix so can't help much). Anyway, it's marked P4, which explains
some of the inaction, but it effectively is a blocker to v1.1 (via Bug #157830),
so should the priority be changed to something higher?

Comment 34

[timeless]

Attachment: current patch, nits addressed

alge#mozillazine test compiled for me -- thanks

Comment 35

[timeless]

Attachment: current patch, really

whoops, i talked some mods over w/ alge instead of passing patches and we had a
miscommunication.

Comment 36

[timeless]

well, there's a patch, someone wrote it (arik devens), someone feels it might be
useful for 1.1 (transitivity), maybe someone else can scare up reviews for it.
jag just hopped on a plane and brendan went into hiding for some secret project.

Comment 37

[Dan Rosen]

Oh right, Arik Devens. He was the magical disappearing intern for XPToolkit last
summer. Just like I was the magical disappearing employee, I guess.

Comment 38

[Akkana Peck]

Comment on attachment 94138 [details] [diff] [review]
current patch, really

>Index: widget/src/xpwidgets/nsBaseWidget.h
>===================================================================
>+  NS_IMETHOD              SetWindowType(nsWindowType aWindowType);
>+  NS_IMETHOD              GetWindowClass(char *aClass);
>+  NS_IMETHOD              SetWindowClass(char *aClass);

SetWindowType seems to be out of place, and I don't see a definition for it. 
Also it makes the compile fail.  Looks like this might have been merge cruft,
and should probably be removed.

> Index: widget/src/gtk/nsWidget.h
> ===================================================================
> +  NS_IMETHOD SetWindowClass(char *aClass);

You also need a line like this:
+  NS_IMETHOD GetWindowClass(char *aClass);
or it doesn't compile.

>+  index = persistString.Find(gLiteralScreenX);
>+  if (!aPersistPosition && index > kNotFound) {
>+    persistString.Cut(index, gLiteralScreenX.Length());
>+    saveString = PR_TRUE;
>+  } else if (aPersistPosition && index == kNotFound) {
>+    persistString.Append(gLiteralSpace+gLiteralScreenX);
>+    saveString = PR_TRUE;
>+  }

If it were code I maintained, I would probably have abstracted this pattern
into a function or macro, since you have to repeat it so often (the code might
be more readable that way, too).  Stylistic issue, personal preference, I'll
leave that one up to the sr/module owner.

Otherwise it looks fine.  r=akkana if you fix the first two issues, your choice
on the last one.

Comment 39

[timeless]

Attachment: patch per reviews

did i mention i don't want to maintain this code?

Comment 40

[Akkana Peck]

Comment on attachment 94238 [details] [diff] [review]
patch per reviews

r=akkana

Comment 41

[Alec Flett]

Comment on attachment 94238 [details] [diff] [review]
patch per reviews

sr=alecf

Comment 42

[Christopher Blizzard (:blizzard)]

Two things:

+    NS_IMETHOD GetWindowClass(char *aClass) = 0;

Shouldn't that be a **aClass, not a simple char *?  Generally, we don't go
messing with pointers like that.

Second:

+      windowClass.Append(NS_LITERAL_STRING("-jsSpamPopupCrap"));

huh?

Comment 43

[Boris Zbarsky [:bzbarsky]]

> +                        persistString.Find(gLiteralScreenY) > kNotFound;

That's a _really_ bizarre use of kNotFound.... Just use != instead of >

Comment 44

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 94238 [details] [diff] [review]
patch per reviews

Oh and to what blizzard said about

> +    NS_IMETHOD GetWindowClass(char *aClass) = 0;

I want to add that that won't actually get anything, since the char* is passed
by value so modifying it locally does not propagate changes to the caller.

Comment 45

[timeless]

Attachment: how's this?

Comment 46

[David Bradley]

Is it kosher to pass the strdup'd string out like this? I'm too tired to
remember or go look. Will all users use free() ?

Comment 47

[Braden]

I'd say it's bad form in a C++ project to expect users to use free instead of
delete. But there's another problem there: strdup (and presumably whatever it
might be replaced with) allocates memory, so there ought to be a check for
allocation failure.

Comment 48

[Christian :Biesinger (don't email me, ping me on IRC)]

doing that is ok, I suppose... at least in case of "out  string foo" in an .idl
file, people are supposed to use nsMemory::Free to free the string later, which
is free() and not delete[].

Comment 49

[Braden]

In that case, strdup shouldn't be used at all. Use of free is an implementation
detail of nsMemory::Free.

Comment 50

[David Bradley]

I agree with Braden. Unfortunately it looks like this is commonly broken which
means nsMemory is useless now without some cleanup work.

Comment 51

[David Bradley]

That's very true, unfortunately this "pattern" is repeated frequently, either
with nsCRT::strdup, PL_strdup, or strdup.

So if you want to be correct, use nsMemory to allocate the buffer. If you want
to be consistent with much of Mozilla and add to the growing amount of code
making nsMemory useless then use strdup. At this point it probably doesn't make
much difference one way or another.

Comment 52

[timeless]

Attachment: like before + handle alloc failures

as for blizzard's huh question, someone would have to ask arik.

Comment 53

[timeless]

a few points.
1. There's no nsIWidget.idl (there was a while ago but it was killed, see patch
history)

this is a summary of what alecf told me
2. for cstrings we use cstdlib
3. for widestrings we use nsMemory which in turn uses cstdlib.
   an alternative nsIMemory impl isn't obligated to use cstdlib, but nsMemory
will use it. (and i think there's something about xpcom consumers using our
nsMemory via some interface...)

hopefully i can get alecf to review the latest patch.

Comment 54

[Alec Flett]

Comment on attachment 94861 [details] [diff] [review]
like before + handle alloc failures

boy who reviewed that earlier patch?! oh right. :)

anyhow, I should have just said "use nsACString&" instead of char** - its the
New Better Way.

Comment 55

[R.K.Aa.]

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

Comment 56

[[e-mail]]

It seems the patch now makes windows have a window class
string like "navigator:browser" or "mail:3pane". This is probably a
conforming window class name, but I would suggest to use something
like "NavigatorBrowser" because some configuration files dont seem to like
a colon there (e.g. IceWM's). Even if this is really ICEWM's bug, making the
string here simple and not use special characters will probably make
it easier to use in many current and future situations. 

Comment 57

[Jamie Zawinski]

xrdb is absolutely going to barf on the colon (some WMs, e.g., twm and mwm
descendants like SGI's 4Dwm) use xrdb to talk about windows, even if the app
itself doesn't use Xt/Xrm.

You could just replace the colon with a "." and get away with it.  But that
would be wrong, because dots are used to indicate widget hierarchy, not type
hierarchy.  The window name/class both talk about the top-level window, so there
should be no dots in them.  Also, don't use asterisks or other punctuation.  And
don't confuse "window name" and "window title": the names/classes are rather
static, and the title is based on the page.  The "class" should be the name of
the application, and the "name" should be the kind of window it is.

In NS3, all windows had the class "Netscape", and these names were used for the
various types of top-level window:

    Navigator
    Mail
    News
    Editor
    Composition
    Bookmark
    Download
    AddressBook
    Dialog

You ought to use the same names.  There's no compelling argument to use
something different.

Comment 58

[Robert Ginda]

The compelling argument not to use the same names is that the class is read
dynamically from the "windowtype" attribute of the <xul:window/> tag.  We *have*
to do it this way to allow XUL apps that havn't been written yet (or XUL apps
that have been written, but aren't shipped by default) to specify a window
class.  Hardcoded lists are just plain wrong in this situation.

The "windowtype" attribute already exists[1], is documented[2], and 
already used as a unique identifier for a specific class of window.  It would be
a shame to introduce a "windowclass" attribute and end up with *two* unique
identifiers per window.  We could "fix" the known windowtype attributes, but
we'd still have the potential for someone to introduce a "bogus" windowtype. 
Fixing windowtype is no small task, either.  Not only do you have to fix the XUL
files, but every callsite that used the previous type name.

Why not just replace [\D\W]* with "_" and be done with it?


1: http://lxr.mozilla.org/mozilla/search?string=windowtype%3D
 
http://lxr.mozilla.org/mozilla/source/xpfe/appshell/public/nsIWindowMediator.idl#64
2: http://www.xulplanet.com/tutorials/xultu/elemref/ref_window.html

Comment 59

[[e-mail]]

I second Robert Ginda's suggestion of sticking with the windowtype and
replacing everthing that is not in
[-a-zA-Z0-9] by a "_". (More on allowed ressource name syntax for xrdb and
special characters used is in the X man page)

Comment 60

[Jamie Zawinski]

Well, I sure don't agree that "doing it the XUL way" trumps "doing it the X
way", but I guess you've been barking up that tree for a long time now.

Comment 61

[Christian :Biesinger (don't email me, ping me on IRC)]

well, these two don't seem to be exclusive to me.
couldn't mozilla use always the same window class, and a different window name
(the windowtype attribute)?

Comment 62

[Robert Ginda]

The original patch used Mozilla.<windowtype>, has this changed?

Comment 63

[Christian :Biesinger (don't email me, ping me on IRC)]

I have no idea, I just suggested this based on what jwz wrote.

Comment 64

[Tim Phipps]

Re: comment 57
 >   Dialog
> You ought to use the same names.  There's no compelling argument to use
something different.

Yes there is, Dialog is a daft name, see bug 116233

Comment 65

[Jamie Zawinski]

Tim, you're confusing window title / icon title with window name / window class.
All four are completely different things.

Comment 66

[Tim Phipps]

Yes, you're right, it's just that I'd like to see bug 116233 being fixed at the
same time as this.

Comment 67

[Christian Reis]

Has part of this been checked in? The latest versions have
navigator:browser.Mozilla for browser windows here, and I've been trying to
track it down. Has it been changed by somebody else in another bug?

Comment 68

[Jamie Zawinski]

This is better in 1.2b, but as mentioned in comments #57 and #58, those colons
are going to be problematic.  Also I notice that it's doing:

    WM_CLASS(STRING) = "Mozilla", "navigator:browser"
    WM_CLASS(STRING) = "Mozilla", "bookmarks:manager"

That's backwards: you're setting res_name to Mozilla and res_class to
navigator:browser.  You ought to set them the other way around, since "class" is
for the more general and "name" the more specific.

Comment 69

[Jamie Zawinski]

It's gotten worse again in 1.3b.  See bug 194299.

Comment 70

[Boris Zbarsky [:bzbarsky]]

You mean it's gotten worse in the experimental gtk2 builds of 1.3b, right?

Comment 71

[Jamie Zawinski]

You're speaking some strange moon-man language! I got it out of 
http://ftp.mozilla.org/pub/mozilla/releases/mozilla1.3b/Red_Hat_8x_RPMS/gtk2/i386/
if that's what you're asking, since I don't know what "xft" means and there's no
README.  Version info is in that other bug.

Comment 72

[Boris Zbarsky [:bzbarsky]]

Yeah, that's the gtk2 build (look at the dir name).  Those are very much
"work-in-progress" right now (though getting almost up to parity with the
"standard" gtk1 builds.

Comment 73

[Jamie Zawinski]

Ok, if the gtk1 build is "standard", where's the RPM for it?  In the unexplained
"xft" directory?

Comment 74

[Boris Zbarsky [:bzbarsky]]

In the RedHat 7.x directory, for one.

But yes, it looks like the "xft" directory has the non-gtk2 RedHat 8 RPMS (the
ones in "gtk2" are using both gtk2 _and_ xft).

Comment 75

[Kenneth Herron]

Attachment: Set window class and window role

As noted in comment 68, the previous patches--one of which is currently in
mozilla--have the two WM_CLASS strings backwards. It would also be nice to set
the WM_WINDOW_ROLE property; some window managers apparently use this to help
remember window positions.

This patch addresses these issues, along with some of the code-review comments
about the previous patches. This patch makes the following changes:

* Remove GetWindowClass(). This didn't really serve any purpose.

* Pass the main program name ("Mozilla") as an argument to SetWindowClass()
  instead of hardcoding it into the gtk implementation.

* Convert disallowed characters in the xul window type to '_', as discussed.

* If the xul window type has a colon, split it into two strings & use the 
  second string for the WM_WINDOW_ROLE property. Otherwise, use the xul
  window type for both res_name and WM_WINDOW_ROLE.

* Remove the code that appends "-jsSpamPopupCrap" to the xul window type.

Currently, a browser window is identified as:

    WM_CLASS(STRING) = "Mozilla", "navigator:browser"

With this patch, a browser window is identified as:

    WM_WINDOW_ROLE(STRING) = "browser"
    WM_CLASS(STRING) = "navigator", "Mozilla"

Comment 76

[Kenneth Herron]

Attachment: Set window class & role, round 2

Here's another try at setting the window class & role. It has the following
differences from the previous patch:

1) Set a default wm_class when the window is initially created. This assures
the window will have a resonable wmclass even if the xul doesn't contain a
window type (e.g. dialog windows).

2) Uppercase the first letter of the wmclass name string.

3) Move the SetWindowClass() method out of nsWidget into nsWindow.

4) Removed some cruft, oops.

With changes (1) and (2), mozilla is much more compatible with ns3 as outlined
in comment 57.

Comment 77

[Kenneth Herron]

Attachment: Set window class & role, round 3

Clean up SetWindowClass() a bit more based on its new class. Sorry for the
spam; this should be the last update unless someone suggests a change.

Comment 78

[Peter Daum]

It looks like this problem had been fixed in the gtk1 builds for quite
a while. When you use the gtk2 toolkit, however, still all windows get
the same window class.This had already been mentioned in 02/2003.
While at that time it was certainly true that gtk2 was "work in
progress", I would assume that gtk2 soon will be the default build for
*nix.

Are there any plans to fix this window class issue for gtk2, too?
(I guess, the most logical solution would be to set the same window class
no matter which toolkit is used)

Comment 79

[Christopher Blizzard (:blizzard)]

Comment on attachment 116067 [details] [diff] [review]
Set window class & role, round 3

We need a gtk2 impl of this, too.

Comment 80

[Christopher Blizzard (:blizzard)]

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

Comment 81

[Christopher Blizzard (:blizzard)]

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

Comment 82

[Christopher Blizzard (:blizzard)]

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

Comment 83

[Christopher Blizzard (:blizzard)]

There's actually a pretty good patch in bug 194299 for this problem.  It should
probably just use gtk_window_set_wmclass instead of rolling its own X calls and
save some code.  But it's a good start.  There's also the "don't use this" in
the gtk documentation which gives me pause:

http://developer.gnome.org/doc/API/2.2/gtk/GtkWindow.html#gtk-window-set-wmclass

Comment 84

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 116067 [details] [diff] [review]
Set window class & role, round 3

ICCCM implies that we should be using "mozilla" "Mozilla" for all mozilla
windows.  Another good reason to switch to Firefox and co.

Comment 85

[Jamie Zawinski]

Firefox 1.0 and Mozilla 1.7.3 still do this:

    WM_CLASS(STRING) = "Gecko", "Firefox-bin"
    WM_CLASS(STRING) = "Gecko", "Mozilla-bin"

This bug is about to pass its five year anniversary.

If we're lucky, maybe it will be fixed before it's old enough to vote?

Comment 86

[Kenneth Herron]

In the 14 months between the time I wrote my last patch and the time it got
reviewed, I somehow lost interest in working this bug. But in case anyone else
wants to tackle it, I'd like to respond to the review comments:

(In reply to comment #79)
> (From update of attachment 116067 [details] [diff] [review] [edit])
> We need a gtk2 impl of this, too.

The gtk2 implementation was attached to bug 194299 at the time.


(In reply to comment #83)
> There's actually a pretty good patch in bug 194299 for this problem.  It should
> probably just use gtk_window_set_wmclass instead of rolling its own X calls and
> save some code.  But it's a good start.  There's also the "don't use this" in
> the gtk documentation which gives me pause:
> 
> http://developer.gnome.org/doc/API/2.2/gtk/GtkWindow.html#gtk-window-set-wmclass

As I recall, gtk_window_set_wmclass() can't be used to change the class after a
window is realized, and gtk2 at least prints something to stderr if you try.
nsWindow::SetWindowClass() was being called on realized windows, so it had to be
implemented with direct X calls. There's a comment to that effect in the patch.

Regarding their "don't use this" warning, the function isn't deprecated or
anything of the sort. Gtk's initialization code sets the default window class
strings from argv[0], and in their their narrow interpretation of the ICCCM
there's no reason to change it. 


(In reply to comment #84)
> (From update of attachment 116067 [details] [diff] [review] [edit])
> ICCCM implies that we should be using "mozilla" "Mozilla" for all mozilla
> windows.  Another good reason to switch to Firefox and co.

If you're aiming for ICCCM compliance, then you should WONTFIX this bug because
it's specifically asking for non-compliant behavior. The ICCCM would allow using
a class of "Mozilla" or "Gecko" for all apps and instance names of "mozilla",
"thunderbird", etc. if you wanted to do that, but that's not what this bug is
requesting.

Comment 87

[Elmar Ludwig]

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

Comment 88

[Alex]

Attachment: Combined earlier patches for 1.7.7/8

I have taken the two patches (gtk1 and gtk2) referred to by Kenneth Herron and
tidied them up for Mozilla 1.7.7.  I have been running them for the last few
days without incident.	Once again I can tell a message composition window from
a navigator window etc.

Maybe someone official would like to actually import them, and this bug could
become the very first one I've voted for that actually gets fixed.  I won't be
holding my breath, though.

Comment 89

[mmc]

Alex, try asking for reviews on your patch if you want this bug to move forward.

Comment 90

[Alex]

If anyone cares, the patches applied pretty cleanly to 1.7.8 as well.  No idea
about 1.8x branch.

I have no idea how to ask for a review of these patches.  What else are you
expected to do other than post actual patches in the official thread for the bug?

Since they weren't mine, my C is rusty and I've never programmed X Windows
libraries in my life, I wouldn't be in much of a position to modify them if that
was required.  As far as I can see, the original patches from Kenneth Herron
have some comments about "review" and "superreview" and they didn't seem to make
any difference.  If I sound jaded and cynical, it's because I am.

My understanding of some threads I read recently, was that the only way any
"bugs which make Mozilla better rather than fixing something that makes it
crash" would be worked on was if they had more than 50 votes or more than 50
CCs.  This bug is still way short on votes, but if everyone still interested
could get one other person to CC, maybe there would be a chance.

Comment 91

[Arthur]

Thanks a lot for your work.

"I have no idea how to ask for a review of these patches.  What else are you
expected to do other than post actual patches in the official thread for the bug?"

Click on the "Edit"-Link next to your patch. There you can set a '?' after
'review'. Like this the appropriate reviewers/drivers should get notified. Else
they would have to police all those ten thousands of open bugs.

Comment 92

[Alex]

OK, done that.  Let's see what happens.  Thanks for the info.

Comment 93

[jag (Peter Annema)]

Comment on attachment 183281 [details] [diff] [review]
Combined earlier patches for 1.7.7/8

Just looking at some of the values this patch will create based on lxr on
windowtype=:

    WM_WINDOW_ROLE(STRING) = "browser"
    WM_CLASS(STRING) = "navigator", "Mozilla"

    WM_WINDOW_ROLE(STRING) = "text"
    WM_CLASS(STRING) = "composer", "Mozilla"

    WM_WINDOW_ROLE(STRING) = "html"
    WM_CLASS(STRING) = "composer", "Mozilla"

    WM_WINDOW_ROLE(STRING) = "page-info
    WM_CLASS(STRING) = "Browser "Mozilla"

    WM_WINDOW_ROLE(STRING) = "profileSelection"
    WM_CLASS(STRING) = "mozilla", "Mozilla"

    WM_WINDOW_ROLE(STRING) = "venkman_bp-props"
    WM_CLASS(STRING) = "mozapp", "Mozilla"

    WM_WINDOW_ROLE(STRING) = "chatzilla_config_add"
    WM_CLASS(STRING) = "irc", "Mozilla"

Is this ok?

I looked at the patch briefly and it looks like it's doing what it claims to be
doing, but I don't know gtk all that well. Asking bryner to r=

Comment 94

[Alex]

I don't know gtk either, I just remember enough about C to be able to adapt the
patches posted by someone else to a more recent version.

The Class and Resource are set fine for all the windows I have used.  That's the
limit of my knowledge.  I don't even know what a Role is.

The resources I see are capitalised (e.g. Navigator) with respect to your list,
but for all I know that's an artefact of my window manager.

Comment 95

[jag (Peter Annema)]

You're right, the first letter should be uppercased, so Navigator, Composer,
Browser, Mozilla, Mozapp, Irc.

Comment 96

[bugzilla]

This was fixed for a while, but Firefox-1.x has this problem again, presumably
reintroduced when the window class and instance were renamed to "firefox-bin".

It would be wonderful to see this fixed again.

Comment 97

[Kenneth Herron]

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

Comment 98

[Brian Ryner (not reading)]

Comment on attachment 183281 [details] [diff] [review]
Combined earlier patches for 1.7.7/8

not going to have time for this, sorry

Comment 99

[Stewart]

Looks like bug #283342 is related to this one.
There's a few patches over there already, though... so I'm not sure if I should mark the other bug as a duplicate of this one, or what.

Comment 100

[Alex]

To me, the bugs look related but not the same.  I have no idea if the patches on this bug fix the problems listed on the other bug.  Nor do I have any idea if the patches on the other bug work reliably.

I can say that the patches on this bug have worked for me for nearly a year now and allow me to identify major windows (browser vs Mail window etc) in my window manager.  I have never paid attention to popups (which this other bug talks about).  All that has proved impossible, is getting the patches QA'd by someone with the power to commit them.  Dumping another (albeit related) bug on this one isn't likely to help anyone, it seems to me.

I now understand why the original developer of the patches just gave up.

Comment 101

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

Comment on attachment 183281 [details] [diff] [review]
Combined earlier patches for 1.7.7/8

r+sr on the gtk2 changes. I will check this in, GTK2 only.

Comment 102

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

Actually the version I'm checkin in fixes some leaks on allocation failure in SetWindowClass.

Comment 103

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

Checked in.

sandreas41, I believe your patch in bug 283342 that adds windowtype attributes to a bunch of windows is still useful in conjunction with this patch. Please request review for it ... [e-mail] would be a good person to try.

Comment 104

[Magnus Melin [:mkmelin]]

Correcting milestone... Btw, is this going to make it to the branch?

Comment 105

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

I think not.