Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
XUL
P4
normal
RESOLVED FIXED
18 years ago
3 years ago

People

(Reporter: Brad Behm, Assigned: jag (Peter Annema))

Tracking

Trunk
mozilla1.9alpha1
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 8 obsolete attachments)

(Reporter)

Description

18 years ago
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

Updated

18 years ago
Assignee: cbegle → waterson
Status: UNCONFIRMED → NEW
Component: Browser-General → XP Miscellany
Ever confirmed: true

Comment 1

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

Comment 2

18 years ago
toolkit
Assignee: waterson → trudelle
Component: XP Miscellany → XP Toolkit/Widgets
QA Contact: asadotzler → jrgm

Comment 3

18 years ago
reassigning to danm as p4 for m18, cc pavlov, put on helpwanted radar
Assignee: trudelle → danm
Keywords: helpwanted
Priority: P3 → P4
Target Milestone: --- → M18

Comment 4

18 years ago
Mass moving M18 bugs to M19
Target Milestone: M18 → M19

Comment 5

17 years ago
mass-moving all bugs to m21 that are not dogfood+ or nsbeta2+ or nsbeta2-
Target Milestone: M19 → M21

Comment 6

17 years ago
Created attachment 10979 [details] [diff] [review]
patch that sets the window class on toplevel windows and dialogs

Comment 7

17 years ago
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.

Updated

17 years ago
Keywords: patch

Updated

17 years ago
Target Milestone: M21 → Future

Comment 8

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

Comment 9

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

Comment 10

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

Comment 11

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

Comment 12

17 years ago
  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.
Target Milestone: Future → mozilla1.0

Comment 13

17 years ago
  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).
Assignee: danm → arik

Comment 14

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

Comment 15

17 years ago
Created attachment 31052 [details] [diff] [review]
this merges rob ginda and my patches and starts to add support to not persist js popup windows, not done yet

Comment 16

17 years ago
not quite done yet but it's a start
Status: NEW → ASSIGNED
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

17 years ago
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

16 years ago
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

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

Comment 21

16 years ago
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

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

Comment 23

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

Comment 24

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

Comment 25

16 years ago
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)
Target Milestone: mozilla1.0 → mozilla1.0.1

Comment 26

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

Comment 27

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

Comment 28

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

Comment 29

16 years ago
Sheesh. Jag can you review the patches on this bug and decide whether they 
do the right thing.
Assignee: arik → jaggernaut
Severity: trivial → normal
Status: ASSIGNED → NEW
Target Milestone: mozilla1.0.1 → ---

Comment 30

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

Comment 31

15 years ago
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).
Target Milestone: --- → mozilla1.1alpha

Comment 32

15 years ago
It's all Mozilla-bin

Updated

15 years ago
URL: N/A

Updated

15 years ago
Blocks: 157830

Comment 33

15 years ago
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

15 years ago
Created attachment 94137 [details] [diff] [review]
current patch, nits addressed

alge#mozillazine test compiled for me -- thanks

Updated

15 years ago
Attachment #10979 - Attachment is obsolete: true
Attachment #31052 - Attachment is obsolete: true

Comment 35

15 years ago
Created attachment 94138 [details] [diff] [review]
current patch, really

whoops, i talked some mods over w/ alge instead of passing patches and we had a
miscommunication.
Attachment #94137 - Attachment is obsolete: true

Comment 36

15 years ago
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.
Assignee: jaggernaut → timeless
Target Milestone: mozilla1.1alpha → mozilla1.1final

Comment 37

15 years ago
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

15 years ago
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.
Attachment #94138 - Flags: review+
Attachment #94138 - Flags: needs-work+

Comment 39

15 years ago
Created attachment 94238 [details] [diff] [review]
patch per reviews

did i mention i don't want to maintain this code?
Attachment #94138 - Attachment is obsolete: true

Comment 40

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

r=akkana
Attachment #94238 - Flags: review+

Comment 41

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

sr=alecf
Attachment #94238 - Flags: superreview+
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

15 years ago
> +                        persistString.Find(gLiteralScreenY) > kNotFound;

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

Comment 44

15 years ago
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.
Attachment #94238 - Flags: needs-work+

Comment 45

15 years ago
Created attachment 94406 [details] [diff] [review]
how's this?

Comment 46

15 years ago
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

15 years ago
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.
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

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

Comment 50

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

Comment 51

15 years ago
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

15 years ago
Created attachment 94861 [details] [diff] [review]
like before + handle alloc failures

as for blizzard's huh question, someone would have to ask arik.
Attachment #94238 - Attachment is obsolete: true
Attachment #94406 - Attachment is obsolete: true

Comment 53

15 years ago
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.
Status: NEW → ASSIGNED
No longer blocks: 157830

Comment 54

15 years ago
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

15 years ago
*** Bug 162365 has been marked as a duplicate of this bug. ***
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

15 years ago
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

15 years ago
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
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

15 years ago
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.
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

15 years ago
The original patch used Mozilla.<windowtype>, has this changed?
I have no idea, I just suggested this based on what jwz wrote.

Comment 64

15 years ago
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

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

Comment 66

15 years ago
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

15 years ago
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

15 years ago
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

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

Comment 70

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

Comment 71

15 years ago
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

15 years ago
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

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

Comment 74

15 years ago
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

15 years ago
Created attachment 115380 [details] [diff] [review]
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

15 years ago
Created attachment 116056 [details] [diff] [review]
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.
Attachment #115380 - Attachment is obsolete: true

Comment 77

15 years ago
Created attachment 116067 [details] [diff] [review]
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.
Attachment #116056 - Attachment is obsolete: true
Attachment #116067 - Flags: review?(blizzard)

Updated

14 years ago
Blocks: 55486

Comment 78

13 years ago
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)

Updated

13 years ago
Assignee: timeless → kherron+mozilla
Status: ASSIGNED → NEW

Updated

13 years ago
Attachment #116067 - Flags: superreview?(blizzard)
Attachment #116067 - Flags: review?(caillon)
Attachment #116067 - Flags: review?(blizzard)
Comment on attachment 116067 [details] [diff] [review]
Set window class & role, round 3

We need a gtk2 impl of this, too.
Attachment #116067 - Flags: superreview?(blizzard) → superreview-
*** Bug 194299 has been marked as a duplicate of this bug. ***
*** Bug 196815 has been marked as a duplicate of this bug. ***
*** Bug 221711 has been marked as a duplicate of this bug. ***
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
Blocks: 92033
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.
Attachment #116067 - Flags: review?(caillon) → review-

Updated

13 years ago
Assignee: kherron+mozilla → jag

Comment 85

13 years ago
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

13 years ago
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

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

Comment 88

12 years ago
Created attachment 183281 [details] [diff] [review]
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

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

Comment 90

12 years ago
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

12 years ago
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.

Updated

12 years ago
Attachment #183281 - Attachment description: Combined earlier patches for 1.7.7 → Combined earlier patches for 1.7.7/8
Attachment #183281 - Flags: review?

Comment 92

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

Updated

12 years ago
Attachment #183281 - Flags: review? → review?(jag)
(Assignee)

Comment 93

12 years ago
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=
Attachment #183281 - Flags: review?(jag) → review?(bryner)

Comment 94

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

Comment 95

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

Comment 96

12 years ago
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

12 years ago
*** Bug 55486 has been marked as a duplicate of this bug. ***
Comment on attachment 183281 [details] [diff] [review]
Combined earlier patches for 1.7.7/8

not going to have time for this, sorry
Attachment #183281 - Flags: review?(bryner)

Comment 99

12 years ago
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

12 years ago
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 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.
Attachment #183281 - Flags: superreview+
Attachment #183281 - Flags: review+
Actually the version I'm checkin in fixes some leaks on allocation failure in SetWindowClass.
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 ... mconnor@mozilla.com would be a good person to try.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Depends on: 335068

Comment 104

11 years ago
Correcting milestone... Btw, is this going to make it to the branch?
Keywords: helpwanted
Target Milestone: mozilla1.1final → mozilla1.9alpha
I think not.
Depends on: 496653
You need to log in before you can comment on or make changes to this bug.