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

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

toolkit

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

Mass moving M18 bugs to M19

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

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.

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

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

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

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

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.

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).

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

not quite done yet but it's a start

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

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

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"?

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

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?

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

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

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

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)

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

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

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

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

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...

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).

It's all Mozilla-bin

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?

alge#mozillazine test compiled for me -- thanks

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

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.

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 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.

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

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

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

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?

> + persistString.Find(gLiteralScreenY) > kNotFound; That's a _really_ bizarre use of kNotFound.... Just use != instead of >

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.

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() ?

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[].

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

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

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.

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

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 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.

*** 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.

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.

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)

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)?

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

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

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

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

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

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?

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.

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

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

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.

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.

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

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).

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"

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.

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.

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 on attachment 116067 [details] [diff] [review] Set window class & role, round 3 We need a gtk2 impl of this, too.

*** 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

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.

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?

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.

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

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.

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

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.

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.

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

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=

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.

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

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.

*** 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

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.

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.

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.

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

I think not.