It would be good if other apps were able to use the tray icon code Thunderbird is already using. I suppose this would be relevant for the people implementing support for it on KDE/Gnome, so I'm marking depends, hopefully that's the right thing to do in this case. Also, getting an assignee for this would be good. I'm getting first uni classes about Java and C next week, so I'm very doubtful I would be the right person for this ;-). If someone is interested, please feel free to take this bug.

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

Matthew, I hope you don't mind if I take this bug ;-)

This is a patch for mozilla/toolkit It already works fine and you'll find a readme.txt file in the doc/ directory but it's only a work in progress. It's here only because I need your comments. So please spend a few cycles on it. Thanks !

Daniel, I think you should post a spec on wiki.mozilla.org and let people comment on it. In particular, I'm interested how the <systray> element you mentioned (not implemented yet?) works when there are several instances of a window with it. Also is this going to be a generic solution for creating system tray icons or just minimize-to-tray functionality?

(In reply to comment #4) > Daniel, I think you should post a spec on wiki.mozilla.org and let people > comment on it. In particular, I'm interested how the <systray> element you > mentioned (not implemented yet?) works when there are several instances of a > window with it. Nickolay, I dropped the systrayicon element after discussions with bsmedberg and a few others.

Thanks for the information. Still I'm sure it would be appreciated if you posted the spec (something like what's in the readme.txt file) to wiki.mo and a notice to dev-platform. As I understand, this is not supposed to be a generic solution for systray icons then, it's a pity. It's not obvious how does this work with multiple instances of a window without reading the patch. XUL is pretty inconsistent about attribute naming, should they be camelCase? (I first read "clickstorestore" as click-store-store and couldn't make sense of it until I read the description.)

Bug 208923 may be of interest.

Currently there is a window attribute called "minimizeintaskbar" and the default value is "disabled". This means that the default behavior is different than default FF behavior, since it no special attribute is set, the window starts minimizing to the system tray instead of the taskbar. I'm not sure this is the most logical behavior. It might be better to preserve default FF behavior unless the developers explicitly adds a new attribute.

(In reply to comment #8) > Currently there is a window attribute called "minimizeintaskbar" and the > default value is "disabled". This means that the default behavior is different > than default FF behavior, since it no special attribute is set, the window > starts minimizing to the system tray instead of the taskbar. I'm not sure this > is the most logical behavior. It might be better to preserve default FF > behavior unless the developers explicitly adds a new attribute Excellent point. Yes, you're right.

here are some links to add a systray icon in linux desktop : http://www.freedesktop.org/wiki/Standards_2fsystemtray_2dspec http://developer.kde.org/documentation/library/kdeqt/kde3arch/protocols-docking.html

Since we are in the links, here is one that gives an example (used in Jabberzilla and Mango -ex mozchat) that works on windows and gnome : http://jabberstudio.org/cgi-bin/viewcvs.cgi/jabberzilla/trunk/src/components/base/src/systray/

Daniel, this is what I'm thinking of in terms of splitting the tray stuff and the hiding stuff. This should facilitate things like comment #6 and showing the icon even when the window is visible. Please look at this for a bit and see if it makes sense? Also, could you elaborate on comment #5 a bit? Is the xbl binding going away, or something? (As-is, XBL should still be workable with the changed interfaces)

I _heartily_ endorse the splitting of display-icon-in-tray and minimize-by-hiding/unhide-by-clicking-on-icon. The latter is I think one additional primitive ("hide window") and the some script logic to combine it as the app deems appropriate. We could, and probably should, provide a utility function or two to cover the common cases like "on minimize: hide window(s?) and unhide systray icon; on systray N-click: show window(s) and hide systray icon".

Comment on attachment 221726 [details] Revved interface (split hiding and tray) Interface naming is perhaps obviously wrong here, but also: <trayicon> should use event handlers for dispatching the "callback" behaviour, so that people don't have to learn a new programming model, and things like multiple listeners and such work correctly. And then we don't have to reinvent DOM events, which is always nice.

Here's my take. I grabbed Mook and glazou's code as necessary, as well as some Jabberzilla code (for generating the Win32 icon from imgIContainer -- thanks prefiks!) and did a complete refactoring so the JS-level stuff is in an XBL. I also modified all callbacks to use DOM events, as shaver suggested. Caveats: a) I found generating the patch more challenging than the implementation itself. Would be great if someone could try it on a clean tree. b) I didn't spend much time on the licenses and that stuff, since I don't know what is meant to be there considering the Frankenstein-like nature of the code. c) I tested on a 1.8.0.x branch build by building the code into our extension using shaver's "export XPI_NAME" trick, so I don't know whether it works on a normal trunk build. You'd definitely have to modify toolkit/components/Makefile.in to add the systray/ directory. To use, just place something like this in your browser.xul overlay: <window id="main-window"> <trayicon id="apTrayIcon" title="&ap.workbench.appname;" image="chrome://allpeers/skin/workbench/allpeers_16.png" contextmenu="apTrayIconMenu" closeToTray="true" minimizeToTray="true" alwaysShow="true" /> </window> The referenced menu is in a popupset. The last three attributes are all optional and can be mixed and matched. I'm pretty happy with the overall approach. The main annoyance is the nsWindowUtil class. I put everything in there that I felt should *not* actually be in the systray project. Ideally the window implementation would be extended to include methods for hide(), show(), minimize(), etc. (as part of nsIXULWindow?), and the events I added ("minimizing", "closing", etc.) should be native DOM events. I'd be interested in opinions on how to do this. You should be able to change the title (tooltip) of the icon dynamically. I want to do this for the image as well, and supported cycling/animated images, but I simply didn't have time to do this in this rev. I also didn't implement any of the fancy-schmatzy stuff in Mook's version (e.g. holding CTRL to minimize all windows). If people feel this is needed, the hooks should be there to add it. Final comment: the context menu is displayed too low on the screen. I didn't even try to figure out why; if anyone knows how the positioning is determined, please let me know and I'll fix it.

I should also mentioned that nsWin32Bitmap does not belong here either. It also needs to be refactored/completed to support cursors and bitmaps as well as icons. I think this should eventually be used for the Win32 implementation of nsWindow::SetCursor().

Comment on attachment 223433 [details] [diff] [review] XBL and DOM event-based system tray icon implementation >+ if (handled) { >+ GetCursorPos(&mousePos); >+ POINT screenPos; >+ screenPos.x = mousePos.x; >+ screenPos.y = mousePos.y; >+ ::ClientToScreen(hwnd, &screenPos); You've got those backwards, methinks. GetCursorPos will give you the mouse position in screen coords, and presumably you want the relative ones. Unfortunately, I don't know how you can get the relative position within the icon that was hit. I doubt it matters, though, just get the screenPos from GetCursorPos for displaying the popup correctly, and leave the relative ones as 0 in the event.

general comments only here : 1. I prefer lower-case names for attributes to be consistent with existing xul attributes 2. I miss the clickstorestore attribute that was in my patch and specifies the number of clicks on the icon to restore the window(s) 3. you should NOT use the contextmenu attribute for the icon's context since a window can have a context menu AND an icon menu... 4. I also miss from my patch the minimizeallonclose attribute, that seems to be true by default in your implem

Comment on attachment 223433 [details] [diff] [review] XBL and DOM event-based system tray icon implementation >Index: jar.mn >=================================================================== >RCS file: jar.mn >diff -N jar.mn >--- /dev/null 1 Jan 1970 00:00:00 -0000 >+++ jar.mn 26 May 2006 10:45:53 -0000 >@@ -0,0 +1,3 @@ >+toolkit.jar: >+* content/systray/TrayIcon.xbl (content/TrayIcon.xbl) >+* skin/classic/systray/Bindings.css (skin/Bindings.css) This seems very weird to me. I would VERY MUCH prefer the following: toolkit.jar: % content systray %content/systray/ xpcnativewrappers=yes *+ content/systray/TrayIcon.xbl (content/TrayIcon.xbl) *+ content/systray/Bindings.css (skin/Bindings.css) then you just have to add the following to a window <?xml-stylesheet href="chrome://systray/content/Bindings.css" type="text/css"?> to be able to use the trayicon element. >Index: content/TrayIcon.xbl >=================================================================== >RCS file: content/TrayIcon.xbl >diff -N content/TrayIcon.xbl >--- /dev/null 1 Jan 1970 00:00:00 -0000 >+++ content/TrayIcon.xbl 26 May 2006 10:31:35 -0000 >@@ -0,0 +1,198 @@ >+<?xml version="1.0"?> >+ >+<bindings id="systrayBindings" >+ xmlns="http://www.mozilla.org/xbl" >+ xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"> >+ >+ <binding id="trayicon" extends="xul:spacer"> >+ <implementation> >+ <constructor><![CDATA[ Please, just for readability in XBL, always start <binding> <implementation> <constructor> <destructor> <field> <property> <method> <handlers> and <handler> elements after a blank line. Please also always encaspuslate JS code in a CDATA section with standalone CDATA markers. >+ this.currentTitle = this.getAttribute("title"); >+ if (this.alwaysShow()) >+ this.showIcon(); >+ this.windowUtil.init(window); >+ this.windowUtil.watch(); >+ >+ var self = this; Never use tabs, use whitespaces please... This looks VERY badly indented to me. >+ >+ // add event handlers for window events >+ window.addEventListener("closing", function(event) { self.onClosing(event); }, false); >+ window.addEventListener("minimizing", function(event) { self.onMinimizing(event); }, false); >+ window.addEventListener("activating", function(event) { self.onActivating(event); }, false); >+ ]]> >+ </constructor> >+ <destructor><![CDATA[ >+ if (this.alwaysShow()) >+ this.hideIcon(); >+ if (this.getWindowCount() > 0) >+ { >+ // get another window to display the icon >+ var e = this.windowMediator.getEnumerator(null); >+ var win = e.getNext().QueryInterface(Components.interfaces.nsIDOMWindow); Calling getWindowCount() uses one window enumerator and you use a second one just above. You need only one. >+ var trayIcon = win.document.getElementById(this.id); >+ trayIcon.showIcon(); >+ } Wow. No you can't assume that all windows in an app will have a systray icon. So in the code above, trayIcon may very well be null... >+ ]]> >+ </destructor> You did not remove the event listeners you added to the window in the constructor... >+ <field name="trayService" readonly="true"> >+ Components.classes["@mozilla.org/system-tray-service;1"]. >+ getService(Components.interfaces.nsISystemTrayService); >+ </field> >+ <field name="windowUtil" readonly="true"> >+ Components.classes["@mozilla.org/window-util;1"]. >+ createInstance(Components.interfaces.nsIWindowUtil); >+ </field> >+ <field name="windowMediator" readonly="true"> >+ Components.classes["@mozilla.org/appshell/window-mediator;1"]. >+ getService(Components.interfaces.nsIWindowMediator); >+ </field> >+ <field name="hiddenCount">0</field> >+ <field name="isMinimizing">false</field> >+ <field name="currentTitle"></field> >+ <property >+ name="title" >+ onset="this.currentTitle=val; trayService.setTitle(this.id, this.currentTitle);" >+ onget="return this.currentTitle;" >+ /> You should probably also update the trayicon's title attribute. In that case, you probably don't need the currentTitle field. >+ <method name="showIcon"> >+ <body> >+ var imageSpec = this.getAttribute("image"); >+ var imageUri; >+ if (!imageSpec) >+ imageUri = null; >+ else >+ { >+ var ioService = Components.classes["@mozilla.org/network/io-service;1"]. >+ getService(Components.interfaces.nsIIOService); >+ imageUri = ioService.newURI(imageSpec, null, null); >+ } >+ this.trayService.showIcon(this.id, imageUri, window); >+ if (this.currentTitle) >+ this.trayService.setTitle(this.id, this.currentTitle); Do you need this last call if imageUri is null ? >+ </body> >+ </method> >+ <method name="hideIcon"> >+ <body> >+ this.trayService.hideIcon(this.id); >+ </body> >+ </method> >+ <method name="onClosing"> >+ <parameter name="aEvent" /> >+ <body><![CDATA[ >+ if (this.closeToTray() && this.getWindowCount() == 1) >+ { >+ this.minimizeWindow(window); >+ aEvent.preventDefault(); >+ } >+ ]]></body> There should be an attribute on the trayicon element to say all windows should be minimized when this one is closed. >+ </method> >+ <method name="onMinimizing"> >+ <parameter name="aEvent" /> >+ <body><![CDATA[ >+ if (this.minimizeToTray()) >+ { >+ this.minimizeWindow(window); >+ aEvent.preventDefault(); >+ } >+ ]]></body> There should be an attribute on the trayicon element to say all windows should be minimized when this one is minimized. >+ </method> >+ <method name="onActivating"> >+ <parameter name="aEvent" /> >+ <body><![CDATA[ >+ this.restoreWindow(); >+ ]]></body> If all windows were minimized together, then restore all of them... >+ </method> >+ <method name="minimizeWindow"> >+ <parameter name="win" /> >+ <body> >+ if (this.isMinimizing || this.windowUtil.isHidden()) >+ return; >+ >+ this.isMinimizing = true; >+ >+ this.windowUtil.minimize(); >+ this.windowUtil.hide(); >+ >+ this.hiddenCount++; >+ this.showIcon(); >+ >+ this.isMinimizing = false; >+ </body> >+ </method> >+ <method name="minimizeAll"> >+ <body> >+ // the current window first so we're sure the systray will use its icon >+ this.minimizeWindow(window); >+ >+ // then the others >+ var e = this.windowMediator.getEnumerator(null); >+ while (e.hasMoreElements()) { >+ var w = e.getNext(); >+ //if (w.id == window.id) >+ this.minimizeWindow(w); >+ } >+ </body> >+ </method> >+ <method name="restoreWindow"> >+ <body><![CDATA[ >+ if (!this.windowUtil.isHidden()) >+ return; >+ >+ this.windowUtil.show(); >+ this.windowUtil.restore(); >+ >+ this.hiddenCount--; >+ if (this.hiddenCount == 0 && !this.alwaysShow()) >+ this.hideIcon(); >+ ]]> >+ </body> >+ </method> >+ <method name="restoreAll"> >+ <body> >+ var e = this.windowMediator.getEnumerator(null); >+ while (e.hasMoreElements()) { >+ var win = e.getNext().QueryInterface(Components.interfaces.nsIDOMWindow); >+ var trayIcon = win.document.getElementById(this.id); >+ >+ if (trayIcon) >+ //if (win.id == window.id) >+ trayIcon.restoreWindow(); >+ } >+ </body> >+ </method> >+ <method name="alwaysShow"> >+ <body> >+ return (this.getAttribute("alwaysShow") == "true"); >+ </body> >+ </method> >+ <method name="minimizeToTray"> >+ <body> >+ return (this.getAttribute("minimizeToTray") == "true"); >+ </body> >+ </method> >+ <method name="closeToTray"> >+ <body> >+ return (this.getAttribute("closeToTray") == "true"); >+ </body> >+ </method> >+ <method name="getWindowCount"> >+ <body> >+ var e = this.windowMediator.getEnumerator(null); >+ var count = 0; >+ while (e.hasMoreElements()) { >+ var w = e.getNext(); >+ // if (w.id == window.id) >+ count++; >+ } >+ return count; >+ </body> >+ </method> >+ </implementation> >+ <handlers> >+ <handler event="click" phase="target" clickcount="2" button="0"> >+ this.restoreAll(); >+ event.preventDefault(); I want an attribute to control the clickcount here. >+ </handler> >+ </handlers> >+ </binding> >+</bindings> >Index: skin/Bindings.css >=================================================================== >RCS file: skin/Bindings.css >diff -N skin/Bindings.css >--- /dev/null 1 Jan 1970 00:00:00 -0000 >+++ skin/Bindings.css 26 May 2006 12:24:26 -0000 >@@ -0,0 +1,3 @@ >+trayicon { >+ -moz-binding: url("chrome://toolkit/content/systray/TrayIcon.xbl#trayicon"); >+} Urgh. Weird url. See my first comment above. Other comments: a lot of your js code inside the xbl should use tray/catch statements...

Neil had some additional comments: - alwaysShow, minimizeToTray and closeToTray should be properties, not method. - window minimize/restore should work using nsIDOMChromeWindow. - window hide/show should work using nsIBaseWindow.visibility, although it didn't work when I tried. May be necessary to QI the nsIXULWindow for nsIBaseWindow instead of the docshell. - the new DOM events (onminimizing, etc.) could/should be moved to the widgets module.

(In reply to comment #19) > (From update of attachment 223433 [details] [diff] [review] [edit]) > toolkit.jar: > % content systray %content/systray/ xpcnativewrappers=yes I thought that xpcnativewrappers are on by default now, so that parameter shouldn't be necessary (not that this code works with content a lot anyway). > *+ content/systray/TrayIcon.xbl (content/TrayIcon.xbl) > *+ content/systray/Bindings.css (skin/Bindings.css) > Initial Caps look unusual in file names. > Other comments: a lot of your js code inside the xbl should use tray/catch > statements... Nice typo ;)

Note: XBL is actually an XML language and should be stored in .xml files.

Here's an improved version of the patch that takes into account much of the feedback I've received. It uses nsIDOMChromeWindow.minimize() and restore(), and nsIBaseWindow.visibility instead of special methods in nsWindowUtil. Unfortunately nsXULWindow::GetVisibility() doesn't work (see http://lxr.mozilla.org/mozilla/source/xpfe/appshell/src/nsXULWindow.cpp#735) so I had to leave the isHidden() method. I'll file a bug (and hopefully patch) about this. I also left the watcher methods in the window util class for now. I might need help moving these into widgets. I incorporated many of Daniel's suggestions, but didn't add many (any?) new features. I admit that I'm concentrating on the bells and whistles that we need, right now, but it would probably be pretty easy for someone (Daniel?) to add a lot of that stuff. I did add support for changing system tray icons using the "image" property.

Oops, indentation was a bit screwy in the XBL file. This should fix that. Also, I should have mentioned that I applied the patch from the more logicial Mozilla root rather than /mozilla/toolkit/components/systray, as with the original patch.

For some reason the previous version got truncated so I'm trying again.

Sorry about the spam. As usual creating/uploading the patch has proven more difficult that actually developing the code. This version should be fine (or at least not truncated). I may have to start testing and then uploading, rather than the other way around. :-)

BTW: I'm solliciting input about a potential Mac version here: www.google.com/groups?threadm=nYqdndQ_z7yv0RTZnZ2dnUVZ_s6dnZ2d@mozilla.org. Any insights about Linux would also be appreciated. Also, if anyone wants to volunteer to be QA contact for this, that would be really useful.

Comment on attachment 224552 [details] [diff] [review] Complete XBL patch >Index: toolkit/components/systray/jar.mn >=================================================================== >RCS file: toolkit/components/systray/jar.mn >diff -N toolkit/components/systray/jar.mn >--- /dev/null 1 Jan 1970 00:00:00 -0000 >+++ toolkit/components/systray/jar.mn 5 Jun 2006 12:13:55 -0000 >@@ -0,0 +1,3 @@ >+toolkit.jar: >+* content/systray/trayicon.xml (content/trayicon.xml) >+* content/systray/bindings.css (content/bindings.css) >Index: toolkit/components/systray/content/bindings.css >=================================================================== >RCS file: toolkit/components/systray/content/bindings.css >diff -N toolkit/components/systray/content/bindings.css >--- /dev/null 1 Jan 1970 00:00:00 -0000 >+++ toolkit/components/systray/content/bindings.css 26 May 2006 12:24:26 -0000 >@@ -0,0 +1,3 @@ >+trayicon { >+ -moz-binding: url("chrome://toolkit/content/systray/TrayIcon.xbl#trayicon"); Please don't use tab but whitespace. Just because of the above, your patch CANNOT work. The jar says trayicon.xml when your -moz-binding property refers to TrayIcon.xbl... >+} >Index: toolkit/components/systray/content/trayicon.xml >=================================================================== >RCS file: toolkit/components/systray/content/trayicon.xml >diff -N toolkit/components/systray/content/trayicon.xml >--- /dev/null 1 Jan 1970 00:00:00 -0000 >+++ toolkit/components/systray/content/trayicon.xml 6 Jun 2006 13:07:17 -0000 >@@ -0,0 +1,246 @@ >+<?xml version="1.0"?> >+ >+<bindings id="systrayBindings" >+ xmlns="http://www.mozilla.org/xbl" >+ xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"> >+ >+ <binding id="trayicon" extends="xul:spacer"> >+ <implementation> >+ <constructor><![CDATA[ >+ if (this.alwaysShow) Please, just for readability in XBL, always start <binding> <implementation> <constructor> <destructor> <field> <property> <method> <handlers> and <handler> elements after a blank line. Please also always encaspuslate JS code in a CDATA section with standalone CDATA markers. >+ this.showIcon(); >+ this.windowUtil.init(window); >+ this.windowUtil.watch(); >+ >+ // add event handlers for window events >+ window.addEventListener("closing", this.handleClosingEvent, false); >+ window.addEventListener("minimizing", this.handleMinimizingEvent, false); >+ window.addEventListener("activating", this.handleActivatingEvent, false); >+ >+ if (!window.trayIcon) >+ window.trayIcon = new Array(); >+ window.trayIcon[this.id] = this; What happens if your trayicon element has no id attribute ? Sorry, but I don't like at all that you put the trayIcon array at the window level. I think it should be handled by the XBL itself and become a this.trayIcon . In that case, all your references below to a w.trayIcon where w is a window should probably become a w.getElementsByTagName('trayicon').item(0).trayIcon with all due sanity checks on the existances of a trayicon element... >+ ]]> >+ </constructor> >+ >+ <destructor><![CDATA[ >+ // remove event handlers for window events >+ window.removeEventListener("closing", this.handleClosingEvent, false); >+ window.removeEventListener("minimizing", this.handleMinimizingEvent, false); >+ window.removeEventListener("activating", this.handleActivatingEvent, false); >+ >+ if (this.alwaysShow) >+ this.hideIcon(); >+ if (this.getWindowCount() > 0) >+ { >+ // get another window to display the icon >+ var e = this.windowMediator.getEnumerator(null); >+ while (e.hasMoreElements()) >+ { >+ var win = e.getNext(); >+ var trayIcon = win.trayIcon ? win.trayIcon[this.id] : null; >+ if (trayIcon) >+ { >+ trayIcon.showIcon(); >+ break; >+ } >+ } >+ } The above is suboptimal. You enumerate twice. What about: var e = this.windowMediator.getEnumerator(null); var iconChanged = false; while (e && e.hasMoreElements()) { { var w = e.getNext(); var trayIcon = win.trayIcon ? win.trayIcon[this.id] : null; if (trayIcon) { trayIcon.showIcon(); iconChanged = true; break; } } if (!iconChanged) this.hideIcon(); >+ ]]> >+ </destructor> >+ >+ <field name="trayService" readonly="true"> >+ Components.classes["@mozilla.org/system-tray-service;1"]. >+ getService(Components.interfaces.nsISystemTrayService); >+ </field> >+ <field name="windowUtil" readonly="true"> >+ Components.classes["@mozilla.org/window-util;1"]. >+ createInstance(Components.interfaces.nsIWindowUtil); >+ </field> >+ <field name="windowMediator" readonly="true"> >+ Components.classes["@mozilla.org/appshell/window-mediator;1"]. >+ getService(Components.interfaces.nsIWindowMediator); >+ </field> >+ <field name="hiddenCount">0</field> >+ <field name="isMinimizing">false</field> >+ <field name="handleClosingEvent">({ self: this, handleEvent : function handleEvent(e) { this.self.onClosing(e); } })</field> >+ <field name="handleMinimizingEvent">({ self: this, handleEvent : function handleEvent(e) { this.self.onMinimizing(e); } })</field> >+ <field name="handleActivatingEvent">({ self: this, handleEvent : function handleEvent(e) { this.self.onActivating(e); } })</field> max 80 chars lines please. >+ >+ <property >+ name="title" >+ onset="this.setAttribute('title', val);" >+ onget="return this.getAttribute('title');" >+ /> >+ <property >+ name="image" >+ onset="this.setAttribute('image', val); this.showIcon();" >+ onget="return this.getAttribute('image');" >+ /> >+ <property >+ name="alwaysShow" >+ onget="return this.getAttribute('alwaysShow') == 'true';" >+ /> Don't you need a setter here too ? If the code removes the alwaysShow attribute, and both the icon and the window are visible, shouldn't you remove the icon from the systray ? >+ <handlers> >+ <handler event="click" phase="target" clickcount="2" button="0"> >+ this.restoreAll(); >+ event.preventDefault(); >+ </handler> >+ >+ <handler event="click" button="2"> >+ var popup = document.getElementById(this.contextMenu); >+ if (popup) >+ popup.showPopup(this, event.screenX, event.screenY, "context"); >+ event.stopPropagation(); >+ event.preventDefault(); >+ </handler> I would like the number of clicks to be controlled by an attribute on the <trayicon> element here. >+ </handlers> >+ </binding> >+</bindings> >Index: toolkit/components/systray/public/Makefile.in >=================================================================== > > [...] > >+DEPTH=../../../.. >+topsrcdir=@top_srcdir@ >+srcdir=@srcdir@ >+VPATH=@srcdir@ >+ >+include $(DEPTH)/config/autoconf.mk >+ >+MODULE = systray >+ >+XPIDLSRCS = nsISystemTrayService.idl \ There is a tab before = in the line above. >+ nsIWindowUtil.idl \ >+ nsIWin32Bitmap.idl \ >+ $(NULL) >+ You should probably already discriminate per platform. >+include $(topsrcdir)/config/rules.mk >Index: toolkit/components/systray/public/nsISystemTrayService.idl >=================================================================== >RCS file: toolkit/components/systray/public/nsISystemTrayService.idl >diff -N toolkit/components/systray/public/nsISystemTrayService.idl >--- /dev/null 1 Jan 1970 00:00:00 -0000 >+++ toolkit/components/systray/public/nsISystemTrayService.idl 26 May 2006 08:23:52 -0000 >@@ -0,0 +1,53 @@ > > [...] > >+[scriptable, uuid(2CD7469C-C8EC-48fe-9E9A-88DBECE777FC)] Common practice is lower-case. >+interface nsISystemTrayService : nsISupports >+{ >+ void showIcon(in AString iconId, in nsIURI imageURI, in nsIDOMWindow window); >+ void hideIcon(in AString iconId); >+ >+ void setTitle(in AString iconId, in AString title); >+}; Could you please add javadoc-like comments to all methods/attributes in IDLs ? That allows automated docs on xulplanet and friends. See for instance http://lxr.mozilla.org/mozilla/source/docshell/base/nsIGlobalHistory2.idl >Index: toolkit/components/systray/public/nsIWindowUtil.idl >=================================================================== >RCS file: toolkit/components/systray/public/nsIWindowUtil.idl >diff -N toolkit/components/systray/public/nsIWindowUtil.idl >--- /dev/null 1 Jan 1970 00:00:00 -0000 >+++ toolkit/components/systray/public/nsIWindowUtil.idl 5 Jun 2006 17:59:54 -0000 > > [...] >+ >+#include "nsISupports.idl" >+ >+interface nsIDocShell; >+interface nsIDOMWindow; >+interface nsIXULWindow; >+ >+[scriptable, uuid(A9C9EE3D-8A13-410a-A1E0-3A14B4734ABD)] >+interface nsIWindowUtil : nsISupports >+{ >+ void init(in nsIDOMWindow win); >+ nsIXULWindow getXULWindow(); >+ nsIDocShell getDocShell(); >+ >+ boolean isHidden(); Is it isHidden() or isMinimizedToTray() ? >+ >+ void watch(); >+ void unwatch(); >+}; >Index: toolkit/components/systray/src/Makefile.in >=================================================================== >RCS file: toolkit/components/systray/src/Makefile.in >diff -N toolkit/components/systray/src/Makefile.in This file is full of tabs.

(In reply to comment #28) > >Index: toolkit/components/systray/src/Makefile.in > >=================================================================== > >RCS file: toolkit/components/systray/src/Makefile.in > >diff -N toolkit/components/systray/src/Makefile.in > > This file is full of tabs. Umm, not my patch or business actually - but even though tabs are considered bad in many situations, Makefiles need them very much to work correctly...

> Umm, not my patch or business actually - but even though tabs are considered > bad in many situations, Makefiles need them very much to work correctly... Agreed, but you'll find tabs really everywhere in that one. Not only where it's needed. Display the file into an editor having the wrong wp per tab ratio and you'll understand why I complain about it.

This is a patch that can actually be applied to a normal Mozilla tree without the need for hand-tweaking (as was the case with the previous version). I also made a number of the changes Daniel asked for, mostly (but not exclusively) to do with formatting. I haven't changed the behavior (e.g. of minimizeAll) or added features (e.g. number of clicks to restore) since I still hold out the hope that someone who actually wants these will pick up the ball and implement them. A much bigger problem with the current patch is that it still crashes when you right-click a few times on the tray icon. I spent a considerable amount of time investigating this, but I don't know anything about the handling of events and frames inside Mozilla, and the code is too complex to fix this or find a workaround without a significant time investment. If someone knowledgeable in this area can help out (at least by giving me a nudge in the right direction) this would be greatly appreciated. Otherwise this patch is basically useless.

Here's the most common stacktrace that I get as a result of the crash: > gklayout.dll!nsDOMEvent::GetTargetFromFrame() Line 241 + 0x34 C++ gklayout.dll!nsDOMEvent::nsDOMEvent(nsPresContext * aPresContext=0x0516c208, nsEvent * aEvent=0x0843e7a8) Line 122 + 0xc C++ gklayout.dll!nsDOMUIEvent::nsDOMUIEvent(nsPresContext * aPresContext=0x0516c208, nsGUIEvent * aEvent=0x0843e7a8) Line 59 + 0x7b C++ gklayout.dll!nsDOMMouseEvent::nsDOMMouseEvent(nsPresContext * aPresContext=0x0516c208, nsInputEvent * aEvent=0x00000000) Line 52 + 0x67 C++ gklayout.dll!NS_NewDOMMouseEvent(nsIDOMEvent * * aInstancePtrResult=0x0012f6d0, nsPresContext * aPresContext=0x0516c208, nsInputEvent * aEvent=0x00000000) Line 288 + 0x23 C++ gklayout.dll!nsEventListenerManager::CreateEvent(nsPresContext * aPresContext=0x0516c208, nsEvent * aEvent=0x00000000, const nsAString_internal & aEventType={...}, nsIDOMEvent * * aDOMEvent=0x0012f6d0) Line 1868 + 0x11 C++ gklayout.dll!nsDocument::CreateEvent(const nsAString_internal & aEventType={...}, nsIDOMEvent * * aReturn=0x0012f6d0) Line 4232 + 0x25 C++ systray.dll!nsSystemTrayService::DispatchEvent(nsIDOMDocumentEvent * documentEvent=0x03af6e40, nsIDOMEventTarget * eventTarget=0x076971e0, const nsAString_internal & typeArg={...}, nsIDOMAbstractView * viewArg=0x03b7f4d8, int detailArg=1, int screenXArg=1044, int screenYArg=845, int clientXArg=1044, int clientYArg=845, int ctrlKeyArg=0, int altKeyArg=0, int shiftKeyArg=0, int metaKeyArg=0, unsigned short buttonArg=2, int * _retval=0x0012f850) Line 532 + 0x34 C++ systray.dll!nsSystemTrayService::WindowProc(HWND__ * hwnd=0x00130a78, unsigned int uMsg=7094, unsigned int wParam=1, long lParam=516) Line 498 + 0x67 C++

I'm one of the MinimizeToTray extension developers. (I'm not very tech savvy, I was more of a bug fixer/maintainer, though I did add a few features myself). Anyways, the right click crash is a common problem that we saw before. I may be able to offer some insight. We filed our own bug and commented on what we thought might be causing it here: http://bugzilla.mozdev.org/show_bug.cgi?id=10955 Some extra comments on this bug... Before the 1.5 release of TB and FF, this was a major bug for us. At 1.5, for some reason, those Mozilla apps would respond much quicker to a right mouse click than before. So nobody really runs into this problem like they used to. Now, the only time you see it crash, is if you right click on the icon quickly several times.

One extra comment. If this is a race condition (where a second tray menu is trying to load before the prior one has finished loading), then you may be able to fix it by simply checking if any previous tray menu has finished loading. If the previous menu hasn't finished running through it's stuff, then don't attempt to display it again. I'm at work, so I don't have the time right now to go through our extensions code to see if such a thing is possible.

Thanks for the additional info, Brad. It's interesting to know that this problem has been encountered before. Based on your feedback, I spent a few hours today implementing a check to make sure that any open popup has been closed before a new one is opened. I did this by hooking every popup and reacting to the "popupshowing" and "popuphidden" events. This way I know for sure that all popups are closed before I create any new DOM events (which is where the crash is occurring for me). Unfortunately this didn't help at all, and in fact the crash seems to occur much sooner and more consistently after my changes. So I'm putting this on ice. I'm very disappointed since this was quite a lot of work for several people, but I just can't see releasing code that is known to crash under fairly common circumstances.

That's no fun to hear. I was really looking forward to this going in, mainly because I've been overwhelmed with the work on my end. :) Hopefully someone else more knowledgable will be able to come by and determine the cause of this problem. In the mean time, if I can find some time, I'll start stripping the extension down to it's bare bones, and see if I can get lucky and stumble on a workaround solution.

https://bugzilla.mozilla.org/show_bug.cgi?id=288763 and https://bugzilla.mozilla.org/show_bug.cgi?id=279205 look to be somewhat related. I'm CCing Boris since he seems to have been active in investigating both of these. Probably someone knowledgeable should at least reproduce and take a closer look so there's a chance this can be fixed in a future release. Of course if anyone has an idea of how to workaround the crash that would be wonderful.

Is the crash reproducible on Linux? I don't have a Windows environment set up...

Unfortunately the patch is Windows-only for now since it uses a lot of Windows-specific stuff related to the notification tray. If someone is interested in porting to Linux, that would obviously be useful in any case since the longer term goal is for the notification icon to work on all platforms.

One more thought... First, in our current version of the extension, I implemented a very basic "-turbo" mode, to try to duplicate the behavior seen in Mozilla and Seamonkey. It loads it's own basic XUL window and minimizes that to the tray. The code this turbo XUL window uses is a stripped down version of the code used to minimize a normal Mozilla app window. With a turbo XUL window, I've noticed that it's much harder for me to get this to crash. This is when I realized there was a common trait to the crashes. I'll summarize all the crash scenarios I've seen or heard: * The turbo XUL window running on stripped down code rarely crashes. I had to click about 20 times as fast as I could before I got it to crash (and I can click pretty fast). After that, I haven't been able to crash it since. * Using our extension normally, it is not as tough to crash as the turbo window, but still takes a good amount of speedy right clicking. * Matthew Gertner said with his patch it when you "right-click a few times on the tray icon." * Then when more code was added to watch the tray menu events, and he said "the crash seems to occur much sooner and more consistently after my changes." * Also, back on Firefox 1.0, when using the extension, if we let a minimized window sit in the tray for an extended period of time, it would seem as if Windows would swap Firefox out of memory. In that scenario, a right click on the tray icon would take several seconds to process before a tray menu would show. If I did a second right click during this time before tray menu showed, it would crash. At version 1.5 and after, right clicking always responds instantly. The common trait here seem to be that the more time it takes to process a right click from start to finish, the more likely it is to crash. Either Mozilla is bogged down while processing a right click, or it has to process through more code. Using this, my second guess then is that a second right click gets into WindowProc() before a previous right click is fully processed, and that second right click is crashing it somewhere. (I don't have a clue though specifically where or why crashes). Anyways, maybe it would work if there was some kind of simple boolean flag to see if WindowProc() is currently processing a previous right click message, and if so, just don't process the current right click or hand it back to CallWindowProc().

Can you attach a testcase to the bug? Note also that the last patch won't build anything as no files have been changed outside the systray directory.

Sorry about that, oversight on my part. I'm attaching an additional patch that modifies the parent's makefile so the new component DLL gets built, to be used in conjunction with the previous patch (I can also make a consolidated patch if you prefer). I also spent a couple of hours trying to create a simpler test case, but I can't seem to reproduce the crash without actually having clicks happening on the tray icon. I tried dispatching events via timer from another XUL window, dispatching events from one window to another in response to a mouse click, sending eventing events from the tray icon in response to a timer, etc... none of these crashes. The one thing I haven't tried is dispatching events from another non-Mozilla window in response to a click, so I'm not sure if this is completely system tray-specific (though I doubt it). Actually what I think is happening is that with the menu open, the mouse click on the system tray causes the menu window to lose focus, which triggers the destruction process of the menu and associated frame. While this is happening, the system tray code tries to create a new DOM event, which causes the frame (to which there are still lingering references, although it has been destroyed) to be referenced... resulting in a crash. What I don't understand: - How does Mozilla prevent this from happening when the focus event that triggers destruction of the menu frame comes from one of its windows? - How can the click event get dispatched before the menu destruction has finished, considering this is all happening on one thread? I even tried posting a new message in response to the click and using that to dispatch the event to the <trayicon> element so that the kill focus event gets into the message queue first, but this doesn't help. - How come my attempted fix where I wait for the "popuphidden" event before creating/dispatching the new event didn't help? I can only guess that "popuphidden" is dispatched before the frames have been cleaned up. It shouldn't hard to reproduce the problem with the new patch. Otherwise, if anyone can provide answers to any of the above questions, this might help me to figure this out myself.

(In reply to comment #42) > I also spent a couple of hours trying to create a simpler test case, but I > can't seem to reproduce the crash without actually having clicks happening on > the tray icon. Just attach whatever testcase you have that is crashing. > Actually what I think is happening is that with the menu open, the mouse click > on the system tray causes the menu window to lose focus, which triggers the > destruction process of the menu and associated frame. While this is happening, > the system tray code tries to create a new DOM event, which causes the frame > (to which there are still lingering references, although it has been destroyed) > to be referenced... resulting in a crash. Does it still crash if you call nsIPopupBoxObject::enableRollup(false) ? If not, then the reason you describe would be correct. AWhat I don't understand: > > - How does Mozilla prevent this from happening when the focus event that > triggers destruction of the menu frame comes from one of its windows? The menu code makes sure to destroy the menu only after it has finished with it.

I managed to fix the context menu crash problem. The issue, apparently, was that window procedures can be reentered during Win32 API calls (some sort of idle processing I guess). This means that the system tray service's procedure was being called during frame destruction, and since it creates DOM events that use the frame, bad things happened. I solved this by turning off the default menu rollup (thanks Neil D!) and doing it myself in my own WindowProc (just calling popup.hidePopup() when the tray window loses focus). In itself this doesn't prevent the WindowProc from being reentered during frame destruction, but since now the same WindowProc is responsible for destroying the popup it is possible to protect it against reentrancy. This did lead to an additional problem since apparently a side effect of the automatic rollup is that the menu's parent doesn't get activated when you click on a menu item. Calling enableRollup(false) therefore causes the browser window to be displayed whenever you choose something from the menu (even if the window is hidden in the tray). I managed to get around this by catching the "popupshown" event, getting the native window and hooking it into my own WindowProc that prevents activation of the menu window when the WM_MOUSEACTIVATE message is received (i.e. basically what the automatic rollup does as well). Both of these are terrible hacks of course. I am posting this patch because I strongly suspect that a cleaner solution won't be possible before Mozilla 1.9 and I know some people (including us) want to use this functionality sooner than that. IMO what needs to happen before this gets checked in: - The popup crash has to be fixed properly. It's probable that this will happen (or be much easier to achieve) as a result of Neil D's work (https://bugzilla.mozilla.org/show_bug.cgi?id=279703). - There should be an option to determine whether activating a popup should activate its parent. This gives the implementor more flexibility (I'm pretty sure not many people would want to jump through the hoops that I did) and in any case the current behavior is inconsistent (unless there's a reason why enableRollup should have this side effect). - nsXULWindow::GetVisibility() should be fixed. - The window watcher code should be moved to the widgets module (which would involve creating some new DOM events). Unless anyone disagrees, I'll file bugs for the last 3 when I have time and set up the dependencies.

A couple of small improvements, including making double click on the icon bring the window to the foreground (requested by Daniel).

Please make sure the tray icon has a tooltip or the tray icon window has a SetText done on it so that screenreaders can read it aloud.

The tooltip is set by default to the name of the window associated with the tray icon. It can be overridden by setting the title attribute of the <trayicon> tag.

Someone reported that the context menu is displayed in the wrong place when using two monitors.

I made a couple of tweaks: 1) You can now change all attributes of the trayicon element on the fly (minimizeToTray, closeToTray, alwaysShow, etc.). 2) I changed it to use a custom Minimize() method when the window is hidden in the tray so that I can call SW_MINIMIZE instead of SW_SHOWMINIMIZE. This causes Windows to reduce the working set of Firefox (see https://bugzilla.mozilla.org/show_bug.cgi?id=76831). One could dispute whether this is the right approach, but several of our testers have complained that memory usage doesn't go down when hiding in the tray.

Thanks for the continued work Matthew. I'd jump into helping right now, but I have a new backyard and an obnoxious sprinkling system I have to fix first. :) As for the reduced memory fix, my only concern would be a similar situation bug 76831 describes. If a Mozilla window stays minimizes too long, and then a user right clicks on a tray icon to see the tray menu, it may take several seconds or longer for the menu to appear. In the past, this was a big problem with the extension. But then when the memory usage was changed a year ago or so, the problem went away. But if you see that the tray menu responds quickly no matter how long it sits minimized, then I'll be really happy.

guess this bug would fix bug 115348

I realized that the patch doesn't really work right if you don't have alwaysShow set. This new version should correct that.

http://developer.gnome.org/doc/API/2.0/gtk/GtkStatusIcon.html contains documentation on StatusIcon implementation in GTK (2.10+). Might be useful.

There's a strange regression in Firefox 1.5.0.7. The tray icon menu pops up, but it is an empty rectangle (i.e. the menu options are not visible and can't be selected). Everything works fine in FF 1.5.0.6. If anyone has any idea what might have caused this regression, please let me know.

Apparently there is a regression in FF 1.5.0.7. The tray popup menu appears as an empty rectangle instead of displaying the menu options. It seems like this is a result of me setting the popup to display=none and then back. The good news is that the problems I was having that led me to do this look like they have been fixed, so I went ahead and removed this code. Without it the menu works fine.

The latest regression in Firefox has motivated me to try to get this into the trunch and hence into some sort of future Firefox branch. As I said in https://bugzilla.mozilla.org/show_bug.cgi?id=325353#c44, it would take a considerable amount of work to do this in what I would consider to be the Right Way, but at this point, I think it makes sense to get something into CVS and target the 1.9 branch for some of the deeper changes. I've also seen some indications of momentum towards getting a Linux version of this working, which will be easier to coordinate if the current code is checked in. If anyone wants to volunteer to review, please let me know. Otherwise I'll poke around for someone.

- Fixed include guard in nsWindowUtil.h. - Removed MOZILLA_STRICT_API from src\Makefile.in. - Documented IDL files.

(In reply to comment #57) > Created an attachment (id=240160) [edit] > Fixes based on biesi's comments > > - Fixed include guard in nsWindowUtil.h. > - Removed MOZILLA_STRICT_API from src\Makefile.in. > - Documented IDL files. I've only skimmed this, but: * it does not compile on trunk (should be easily fixable) * imho, has some weird indentation styles * very Windows-centric, without even Makefile foo to make it compile on all platforms * not sure about the need for a "Win32 Bitmap" service. Especially not in the systray component. I'll look a bit more into it.

Did anyone ever create a wikipage/example for this?

Patch for attachment 240160 [details] [diff] [review] that makes it compile on trunk. no-brain attempt to set a tray icon did not work though, but the default icon shows up, minimize to tray works, and context menu likewise.

Comment on attachment 240160 [details] [diff] [review] Fixes based on biesi's comments General code comments * comment syntax is /** ... */ * indentation and code style is screwed many places... * break lines at 80 chars * use attributes in IDL files, f.x. nsIXULWindow getXULWindow(); should be readonly attribute xulWindow; (dunno about exact caps, the attribute is the main point :) ) Functionality: * only part of the title shows for me * click count should be configurable. If an application want to use single-click to restore f.x. I'm no Windows coder, so I do not have many comments on the Windows stuff. But a big issue is that we need to figure out how to make way for a cross-platform implementation of this. >Index: toolkit/components/systray/content/trayicon.xml >=================================================================== >+ <destructor><![CDATA[ >+ this.windowUtil = null; >+ >+ // remove event handlers for window events >+ window.removeEventListener("closing", this.handleClosingEvent, false); >+ window.removeEventListener("minimizing", this.handleMinimizingEvent, false); >+ window.removeEventListener("activating", this.handleActivatingEvent, false); >+ >+ if (this.alwaysShow) >+ this.hideIcon(); Should this not depend on there the icon is actually shown? >+ // get another window to display the icon >+ var e = this.windowMediator.getEnumerator(null); >+ var newTrayIcon = null; >+ var hiddenCount = 0; >+ while (e.hasMoreElements()) >+ { >+ var win = e.getNext(); >+ var trayIcon = ('trayIcon' in win) ? win.trayIcon[this.id] : null; >+ if (trayIcon && trayIcon.windowUtil.isHidden()) >+ hiddenCount++; >+ if (trayIcon && !newTrayIcon) >+ { >+ newTrayIcon = trayIcon; >+ if (this.alwaysShow) >+ newTrayIcon.showIcon(); >+ } >+ } >+ >+ if (newTrayIcon) >+ newTrayIcon.hiddenCount = hiddenCount; >+ >+ this.windowMediator = null; What's the reason behind all this "passing on an icon"? If a window shows an icon, is it not it's own responsibility for leaving it shown (if it has too), when it closes itself? >+ ]]> >+ </destructor> >+ >+ <method name="showIcon"> >+ <body><![CDATA[ >+ var imageSpec = this.getAttribute("image"); >+ var imageUri; >+ if (!imageSpec) >+ imageUri = null; please always use {} >+ else >+ { >+ var ioService = Components.classes["@mozilla.org/network/io-service;1"]. >+ getService(Components.interfaces.nsIIOService); >+ imageUri = ioService.newURI(imageSpec, null, null); >+ } >+ this.trayService.showIcon(this.id, imageUri, window); >+ if (this.title) >+ this.trayService.setTitle(this.id, this.title); >+ ]]> >+ </body> >+ </method> >+ <handlers> For consistency, put handler code inside CDATA too? >+ <handler event="click" phase="target" clickcount="2" button="0"> >+ this.restoreAll(); >+ event.preventDefault(); >+ </handler> >Index: toolkit/components/systray/public/nsISystemTrayService.idl >=================================================================== >+/* >+ * Manages the display of tray icons from Mozilla-based applications. >+ */ >+[scriptable, uuid(2CD7469C-C8EC-48fe-9E9A-88DBECE777FC)] >+interface nsISystemTrayService : nsISupports >+{ >+ /* >+ * Display the tray icon with the specified ID. So do I get the ID from the service or do I invent one myself? (I know the answer :), but it should be clear here) >+ * >+ * @param iconId Unique ID for the icon >+ * @param imageURI URI of the image to use or null to use the window's icon >+ * @param window Window associated with the tray icon >+ */ >+ void showIcon(in AString iconId, in nsIURI imageURI, in nsIDOMWindow window); >Index: toolkit/components/systray/public/nsIWin32Bitmap.idl >=================================================================== >+/* >+ * Converts arbitrary images into Windows-specific image formats >+ */ As I previously commented, I'm not sure we should have a dedicated service for this. At least not in systray. >Index: toolkit/components/systray/public/nsIWindowUtil.idl >=================================================================== >+/* >+ * Utility functions for manipulating DOM windows. This is an intermediate step as these >+ * methods would probably be better situated as part of various existing interfaces. >+ */ Oh yes please :) >Index: toolkit/components/systray/src/Makefile.in >=================================================================== >+REQUIRES = \ >+ xpcom_glue \ >+ xpcom \ >+ widget \ >+ appshell \ >+ docshell \ >+ dom \ >+ string \ >+ gfx \ >+ imglib2 \ >+ necko \ >+ content \ >+ layout \ >+ xuldoc \ >+ locale \ >+ $(NULL) See anything wrong in the above? :) >+CPPSRCS = \ >+ nsWin32Bitmap.cpp \ >+ nsSystemTrayService.cpp \ >+ nsWindowUtil.cpp \ >+ trayModule.cpp \ >+ trayToolkit.cpp \ >+ $(NULL) >+ >+include $(topsrcdir)/config/config.mk ? >+LIBS += \ >+ $(NULL) ? >+SHARED_LIBRARY_LIBS = \ >+ $(NULL) ? >+include $(topsrcdir)/config/rules.mk >+ >+EXTRA_DSO_LIBS = \ >+ $(XPCOM_GLUE_LDOPTS) \ >+ $(NULL) >+EXTRA_DSO_LDOPTS= $(EXTRA_DSO_LIBS) $(MOZ_COMPONENT_LIBS) Consistency please. >+install:: $(TARGETS) ? >Index: toolkit/components/systray/src/nsSystemTrayService.cpp >=================================================================== >RCS file: toolkit/components/systray/src/nsSystemTrayService.cpp >diff -N toolkit/components/systray/src/nsSystemTrayService.cpp This is a WIN32 only file. Either we need to have fully seperate implementations for WIN32, or a super-class if there is anything to share -- not much by the looks of it. I'm no Windows-guy, so only some assorted code comments. >+nsSystemTrayService::~nsSystemTrayService() >+{ >+ BOOL windowClassUnregistered = ::UnregisterClass( >+ (LPCTSTR)nsSystemTrayService::s_wndClass, >+ ::GetModuleHandle(NULL)); BOOL windowClassUnregistered = ::UnregisterClass((LPCTSTR)nsSystemTrayService::s_wndClass, ::GetModuleHandle(NULL)); >+ if (windowClassUnregistered) >+ nsSystemTrayService::s_wndClass = NULL; >+} {} around if clause >+ >+NS_IMETHODIMP >+nsSystemTrayService::ShowIcon( >+ const nsAString& aIconId, nsIURI* imageURI, nsIDOMWindow* aDOMWindow >+) nsSystemTrayService::ShowIcon(const nsAString &aIconId, nsIURI *aImageURI, nsIDOMWindow *aDOMWindow) For this, and everywhere else too. >+{ >+ nsresult rv; check aImageURI and aDOMWindow for null >+ NOTIFYICONDATAW notifyIconData; >+ if (mIconDataMap.Get(aIconId, &notifyIconData)) >+ // Already have an entry so just change the image >+ { Put { on same line as if. Here and everywhere else. >+ HICON hicon; >+ rv = GetIconForURI(imageURI, hicon); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ notifyIconData.hIcon = hicon; >+ MK_ENSURE_NATIVE(trayToolkit::pShell_NotifyIcon(NIM_MODIFY, &notifyIconData)); >+ >+ return NS_OK; >+ } >Index: toolkit/components/systray/src/nsSystemTrayService.h >=================================================================== >+// needed for QueueUserAPC >+#ifdef _WIN32_WINNT >+#undef _WIN32_WINNT >+#endif >+#define _WIN32_WINNT 0x0400 Smells bad. >+// {F53FCEFA-2F53-40cf-BBAF-2F5896A56E82} >+#define NS_SYSTEMTRAYSERVICE_CID \ >+ { 0xf53fcefa, 0x2f53, 0x40cf, { 0xbb, 0xaf, 0x2f, 0x58, 0x96, 0xa5, 0x6e, 0x82 } } >+ >+#define NS_SYSTEMTRAYSERVICE_CONTRACTID \ >+ "@mozilla.org/system-tray-service;1" I'm not sure it is considered "good behaviour", but I'm in favor of putting this in the IDL file. Then they'll be available when you include that. >+#define NS_SYSTEMTRAYSERVICE_CLASSNAME "System Tray Service" >+ >+class nsIDOMAbstractView; >+class nsIDOMDocumentEvent; >+class nsIDOMElement; >+class nsIDOMEvent; >+class nsIDOMEventTarget; >+class nsIURI; >+ >+class nsSystemTrayService : public nsISystemTrayService, public nsIDOMEventListener class nsSystemTrayService : public nsISystemTrayService, public nsIDOMEventListener >+protected: >+ nsresult AddTrayIcon(HWND hwnd, nsIURI* iconURI, const nsAString& iconId, >+ nsIDOMElement* targetElement); >+ nsresult GetIconForWnd(HWND hwnd, HICON& result); >+ nsresult GetIconForURI(nsIURI* iconURI, HICON& result); >+ nsresult CreateListenerWindow(HWND* listenerWindow, nsIDOMElement* targetElement); >+ nsresult GetDOMWindow(nsIXULWindow* xulWindow, nsIDOMWindow** _retval); >+ nsresult GetHwndForEvent(nsIDOMEvent* event, HWND& hwnd); >+ >+ static nsresult CreateEvent(nsIDOMDocumentEvent* documentEvent, >+ const nsAString& typeArg, nsIDOMEvent** _retval); >+ static nsresult CreateMouseEvent(nsIDOMDocumentEvent* documentEvent, >+ const nsAString& typeArg, nsIDOMAbstractView* viewArg, PRInt32 detailArg, >+ PRInt32 screenXArg, PRInt32 screenYArg, PRInt32 clientXArg, PRInt32 clientYArg, >+ PRBool ctrlKeyArg, PRBool altKeyArg, PRBool shiftKeyArg, PRBool metkeyArg, PRUint16 buttonArg, >+ nsIDOMEvent** _retval); >+ >+ static LRESULT CALLBACK WindowProc( >+ HWND hwnd, >+ UINT uMsg, >+ WPARAM wParam, >+ LPARAM lParam >+ ); >+ static LRESULT CALLBACK PopupWindowProc( >+ HWND hwnd, >+ UINT uMsg, >+ WPARAM wParam, >+ LPARAM lParam >+ ); Please fix identation of all functions. >Index: toolkit/components/systray/src/trayModule.cpp >=================================================================== >+static nsModuleComponentInfo components[] = >+{ >+ { >+ NS_WIN32BITMAP_CLASSNAME, >+ NS_WIN32BITMAP_CID, >+ NS_WIN32BITMAP_CONTRACTID, >+ nsWin32BitmapConstructor >+ }, >+ { >+ NS_SYSTEMTRAYSERVICE_CLASSNAME, >+ NS_SYSTEMTRAYSERVICE_CID, >+ NS_SYSTEMTRAYSERVICE_CONTRACTID, >+ nsSystemTrayServiceConstructor, >+ }, >+ { >+ NS_WINDOWUTIL_CLASSNAME, >+ NS_WINDOWUTIL_CID, >+ NS_WINDOWUTIL_CONTRACTID, >+ nsWindowUtilConstructor >+ }, kill the trailing ",". Anal compilers might not like it >+}; >+ >+NS_IMPL_NSGETMODULE(minimizeToTrayModule, components) it's called systray now, is it not? >Index: toolkit/components/systray/src/trayToolkit.cpp >=================================================================== This is purely a Windows util, is it not? Then the name should reflect that.

(In reply to comment #61) > (From update of attachment 240160 [details] [diff] [review] [edit]) > General code comments Argh, forgot to prepend the "warning": This is not the part of the code base I normally work in, so this might be off in the "local code syntax" etc.

(In reply to comment #61) > >+ <method name="showIcon"> > >+ <body><![CDATA[ > >+ var imageSpec = this.getAttribute("image"); > >+ var imageUri; > >+ if (!imageSpec) > >+ imageUri = null; > > please always use {} Common Toolkit style is to omit brackets where they aren't necessary (single line "then" clauses), unless it affects readability.

(In reply to comment #63) > (In reply to comment #61) > > >+ <method name="showIcon"> > > >+ <body><![CDATA[ > > >+ var imageSpec = this.getAttribute("image"); > > >+ var imageUri; > > >+ if (!imageSpec) > > >+ imageUri = null; > > > > please always use {} > > Common Toolkit style is to omit brackets where they aren't necessary (single > line "then" clauses), unless it affects readability. Mkay.

Comment on attachment 240160 [details] [diff] [review] Fixes based on biesi's comments Some comments on the binding as I skimmed through it: >Index: toolkit/components/systray/content/bindings.css >+trayicon { >+ -moz-binding: url("chrome://systray/content/trayicon.xml#trayicon"); >+} How does this get applied? >Index: toolkit/components/systray/content/trayicon.xml >+ <binding id="trayicon" extends="xul:spacer"> >+ <implementation> >+ <constructor><![CDATA[ >+ if (this.alwaysShow) >+ this.showIcon(); >+ this.windowUtil.init(window); >+ this.windowUtil.watch(); >+ What if someone uses this binding from non-chrome, or that not possible? >+ if (!('trayIcon' in window)) >+ window.trayIcon = new Array(); >+ window.trayIcon[this.id] = this; What if no id was specified? >+ if (this.alwaysShow) >+ this.hideIcon(); What if alwaysShow was changed after the element was created? Wouldn't the icon still need to be hidden? >+ >+ // get another window to display the icon >+ var e = this.windowMediator.getEnumerator(null); >+ var newTrayIcon = null; >+ var hiddenCount = 0; >+ while (e.hasMoreElements()) >+ { >+ var win = e.getNext(); >+ var trayIcon = ('trayIcon' in win) ? win.trayIcon[this.id] : null; >+ if (trayIcon && trayIcon.windowUtil.isHidden()) >+ hiddenCount++; >+ if (trayIcon && !newTrayIcon) >+ { >+ newTrayIcon = trayIcon; >+ if (this.alwaysShow) >+ newTrayIcon.showIcon(); >+ } >+ } I'm guessing that this code is trying to find a similar open window in order to keep the tray icon open? This should be looking at the windows by type. Also it shouldn't be assuming that something in that window with the same id is even a trayicon. Better would be to just use getElementsByTagNameNS(XUL_NS, "trayicon"). That would also eliminate the need to store a magical 'trayIcon' property on the window. >+ <field name="trayService" readonly="true"> >+ <field name="windowUtil" readonly="true"> >+ <field name="windowMediator" readonly="true"> >+ <field name="hiddenCount">0</field> >+ <field name="isMinimizing">false</field> >+ <field name="handleClosingEvent"> >+ <field name="handleMinimizingEvent"> >+ <field name="handleActivatingEvent"> Are all of the properties above meant to be called from outside the binding? They should be preceded by an underscore if not. For example, _trayService. >+ <property >+ name="shouldCloseToTray" >+ onget="return this.closeToTray &amp;&amp; this.getWindowCount() == 1" >+ /> The should be readonly as it doesn't have a setter. >+ >+ <property >+ name="title" >+ onset="this.setAttribute('title', val);" >+ onget="return this.getAttribute('title');" >+ /> >+ Is this title used as a tooltip for the icon? If so, that's OK. >+ <property >+ name="image" >+ onset="this.setAttribute('image', val); >+ if (this.alwaysShow || this.hiddenCount > 0) this.showIcon();" >+ onget="return this.getAttribute('image');" >+ /> Fix the indenting in onset so that 'if' lines up with 'this'. >+ >+ <property >+ name="alwaysShow" >+ onset="this.setAttribute('alwaysShow', val);" >+ onget="return this.getAttribute('alwaysShow') == 'true';" >+ /> XUL attributes should be all lowercase. Fix this as well as minimizetotray and closetotray. Also, the setters should return val, and should for all the other property setters earlier as well. >+ >+ <property >+ name="baseWindow" >+ onget="return this.windowUtil.getXULWindow().QueryInterface(Components.interfaces.nsIBaseWindow);" >+ /> _baseWindow, I would assume? >+ >+ <method name="showIcon"> >+ <body><![CDATA[ >+ var imageSpec = this.getAttribute("image"); Or, this.image >+ var imageUri; >+ if (!imageSpec) >+ imageUri = null; >+ else >+ { var imageUri = null; if (imageSpec) { would be more readable here. >+ var ioService = Components.classes["@mozilla.org/network/io-service;1"]. >+ getService(Components.interfaces.nsIIOService); >+ imageUri = ioService.newURI(imageSpec, null, null); >+ } Would be nice to support relative URIs as well, but no big deal. The documentation should mention if only absolute URIs are allowed. >+ >+ <method name="onClosing"> >+ <parameter name="aEvent" /> No space needed at the end here, or in the other methods. >+ >+ <method name="minimizeAll"> >+ <body><![CDATA[ >+ var ret = false; >+ >+ // the current window first so we're sure the systray will use its icon There is no verb in beginning of that sentence ;) >+ <method name="restoreAll"> >+ <body><![CDATA[ ... >+ var windowState = window.windowState; >+ setTimeout("window.focus()", 50); Is there a reason that this is inside quotes? >+ <method name="getWindowCount"> This could be a readonly property named windowCount. >+ <handler event="blur"> >+ var thePopup = document.getElementById(this.contextMenu); >+ thePopup.hidePopup(); What if such a popup doesn't exist? >+ <handler event="click" button="2" clickcount="1"> >+ var thePopup = document.getElementById(this.contextMenu); Again, what if such a popup doesn't exist? >+ var display = thePopup.style["display"]; >+ Doesn't seem to be used anywhere. >+ var onshown = function(event) { >+ var popup = event.target; >+ >+ // prevent infinite recursion (trips bug 174320) >+ popup.removeEventListener("popupshown", onshown, true); >+ >+ var box = popup.popupBoxObject; >+ // display on the right so the tooltip and/or edge of screen don't get in the way >+ var x = popup.targetX - box.width; >+ var y = popup.targetY; >+ >+ // show the popup in the right position >+ popup.hidePopup(); >+ document.popupNode = null; >+ >+ popup.showPopup( >+ document.documentElement, >+ x, y, >+ "context", >+ "", ""); Not sure what you're trying to do here. Show a popup and then hide it and then show it again in a different location? Why don't you show it in the right place to begin with? Also, most of the code above will only work in a top-level chrome window, yet a chrome URL loaded in a frame will have privileges to do all the operations anyway, quite likely messing with the tray icon in various ways. The code should make sure that tray icons only have any effect when loaded from a top level window (nsIDOMChromeWindow).

Comment on attachment 240160 [details] [diff] [review] Fixes based on biesi's comments >+NS_IMETHODIMP >+nsSystemTrayService::ShowIcon( >+ const nsAString& aIconId, nsIURI* imageURI, nsIDOMWindow* aDOMWindow >+) >+{ >+ nsresult rv; >+ >+ NOTIFYICONDATAW notifyIconData; >+ if (mIconDataMap.Get(aIconId, &notifyIconData)) >+ // Already have an entry so just change the image >+ { >+ HICON hicon; >+ rv = GetIconForURI(imageURI, hicon); >+ NS_ENSURE_SUCCESS(rv, rv); This does not work when a icon is being show and there is no imageURI. >+ notifyIconData.hIcon = hicon; >+ MK_ENSURE_NATIVE(trayToolkit::pShell_NotifyIcon(NIM_MODIFY, &notifyIconData)); >+ >+ return NS_OK; >+ }

This fixes using the application icon. It is not optimal. There is no need to reset the application icon all the time, but it fixes attachment 240160 [details] [diff] [review]

Comment on attachment 240160 [details] [diff] [review] Fixes based on biesi's comments >+NS_IMETHODIMP >+nsSystemTrayService::SetTitle(const nsAString& iconId, const nsAString& title) >+{ >+ NOTIFYICONDATAW notifyIconData; >+ if (!mIconDataMap.Get(iconId, &notifyIconData)) >+ return NS_ERROR_NOT_AVAILABLE; >+ >+ notifyIconData.uFlags = NIF_TIP; >+ PRUint32 length = iconId.Length(); Should be title.Length(); >+ if (length > 64) >+ length = 64; >+ >+ wcsncpy(notifyIconData.szTip, PromiseFlatString(title).get(), length); >+ >+ MK_ENSURE_NATIVE(trayToolkit::pShell_NotifyIcon(NIM_MODIFY, &notifyIconData)); >+ >+ return NS_OK; >+}

updated it a bit so that when used on platforms that don't have an implementation it doesn't cause js errors or warnings.

Reminder to plasticmillion from IRC: Remember to change MK_ENSURE_NATIVE to just return a simple error instead of the current macro goop that steals a giant chunk of nsresult possibilities :)

Matthew, Daniel, any chance of traction on this? Would be cool if it went in for Fx 3.

I should have some time in the next few days to deal with this. Thanks for the reminder.

Even more importantly (IMHO) would be useful in XULRunner. Now if only we had a graceful method for the Mac (Growl looks like the best case).

I've integrated almost all of the feedback that I received, and I'm pretty much good to go with a new patch. I'm also making a test extension. Unfortunately I'm stuck since my bitmap generation code doesn't work anymore on the trunk. The problem appears to be that the new Canvas-based image code doesn't know how to deal with alpha channel data. For example, nsThebesImage::GetAlphaBits() simply returns null. I'm not sure if this is work that is yet to be completed or if I have to use an entirely new approach to generate an Windows ICO from another image format. Any guidance would be appreciated.

Matthew, does your (not-yet-released) patch work in 1.8 branch? I'd like to try it.

I've now moved it to the trunk but the existing patches should still work on the the 1.8 branch.

1.8 branch support would be wonderful...

Yeah I'm planning to support both.

I really don't like the C++ portions of this patch. The attributes on nsIWin32Bitmap should all be private members of a win32 specific implementation of nsISystemTrayService.

(In reply to comment #79) > I really don't like the C++ portions of this patch. The attributes on > nsIWin32Bitmap should all be private members of a win32 specific implementation > of nsISystemTrayService. Why? I am working on moving this to widget, but I can't understand why you would want to remove attributes specifically relating to image-type conversion from an interface designed for that purpose and add them to an interface which is for another purpose entirely.

I'm almost done with the new patch. Here's a screenshot to whet your appetites.

Perhaps it will look better as a PNG...

I've made most of the changes suggested by various people in the comments, and ported to the latest trunk sources. I moved the Windows bitmap generation code to the nsImageToBitmap component in widget on biesi's suggestion, and I changed nsWindow::SetCursor to use this component (which basically contains the code that I moved out of that method). I've also separated out the Windows-specific code and added stubs for other platforms to facilitate porting. To answer Neil's question, I display the popup, hide it, move it and then show it because I was getting a lot of flicker when I displayed it directly. Perhaps this is fixed on the trunk now, but I'm leaving the existing code for the time being since I want to preserve compatibility with the 1.8 branch. I know that there are also issues when this code is used with multiple monitors. It's possible that the right solution might be to use a native menu, although I imagine that this would be a lot of work since we'd need to map our menu items into native format and then hook into the native callbacks and get them to call the right Mozilla event handlers for the various menu. I'd be interested in opinions on this. I would also like to move the nsIWindowUtil stuff into widget, ideally on top of existing interfaces for windows. This might require considerable discussion to get through, since it would touch critical Mozilla code, so I'd prefer to get this patch checked in first and then tackle that.

This is the same patch for the 1.8 branch. I didn't change anything in widget since it doesn't seem like it's worth it, so the system tray service (Windows-specific part) is responsible for generating the Windows bitmaps, as Daniel suggested.

Here's an extension that displays a system tray icon with a context menu.

Comment on attachment 256180 [details] [diff] [review] Consolidated trunk patch After just a very quick glance: >+ if (!('trayIcon' in window)) >+ window.trayIcon = new Array(); >+ window.trayIcon[this.id] = this; You're still using a magic property here. You should be retrieving the trayicon element in a window using getElementsByTagName if needed. But I don't think you need to do that either, see below. >+ <property >+ name="alwaysshow" >+ onset="this.setAttribute('alwaysShow', val); No, I meant the attributes should be all lowercase, the properties should be mixed case. >+ <property >+ readonly="true" >+ name="windowCount" >+ onget="var e = this._windowMediator.getEnumerator(window.windowtype); >+ var count = 0; >+ while (e.hasMoreElements()) { >+ var w = e.getNext(); >+ if (('trayIcon' in w) &amp;&amp; w.trayIcon[this.id]) >+ count++; >+ } >+ return count;" >+ /> For instance, here, why do you need a window trayIcon property? It seems you just want to do: if (w == document.defaultView) count++; Similar for the other places it is used. Or is the idea to look for all windows that contain a trayicon with that id? In that case, use getElementById and check to ensure the tag matches, or getElementsByTagName and scan for that id. Also, you should use CDATA if you need to escape characters.

Just pulled and applied this patch, besides awesome, here are some of my finding: - while the param based tray object is nice, it would be even better if it had a failsafe of default preferences with a pref observers for easier configuration - one of the parameters, which I have changed manually in the bindings, is to allow for single versus double-clicking to re-open the window - also the extension demo should show how to reference the .restoreWindow() or restoreAll() calls in the context menu - a nice to have would be if alwaysShow is enabled and window is visible, click tray icon would actually hide window(s)

I haven't looked at this patch closely. Is it possible to start minimized? Or is the closest possibility to auto-hide (meaning window appears then disappears)?

(In reply to comment #88) > I haven't looked at this patch closely. Is it possible to start minimized? Or > is the closest possibility to auto-hide (meaning window appears then > disappears)? There isn't a feature right now to start minimized, but I dare say this wouldn't be that hard to implement. Maybe by registering an nsIWindowMediatorListener and hiding the window before it is shown?

I could definitely see a good reason for having such functionality. Not so much for Thunderbird or Firefox but for the growing # of XULRunner apps out there. Hmm... I wonder if your suggestion would work.

(In reply to comment #90) > (In reply to comment #88) > > I haven't looked at this patch closely. Is it possible to start minimized? Or > > is the closest possibility to auto-hide (meaning window appears then > > disappears)? > > There isn't a feature right now to start minimized, but I dare say this > wouldn't be that hard to implement. Maybe by registering an > nsIWindowMediatorListener and hiding the window before it is shown? > In the MinimizeToTray extension, I tried doing my own implementation for the turbo mode that sounds very similar to what you suggested. In fact, the current version of the extension has code for the turbo mode right now. Just fire up the mozilla app with a -turbo as a command line argument. Unfortunately, it turned out to be very difficult to clean up. So difficult that I completely abandoned that particular implementation. The main problems I had were: 1) The window does show for a split second in the top left hand corner. It's enough that it catches your eye. I spent a few weeks thoroughly trying to find a solution to this. Eventually, I coded in a logging system to track what was going on, and discovered that messages are sent to paint the window before any event can be fired in the extension. Since the painting came before the events, I had no way to prevent the painting from occurring. I also tried to move the window offscreen, but Mozilla doesn't allow that either. 2) Occasionally I got timing issues where the some event doesn't fire quite right, and the window that should get minimized never does. 3) Every so often, the window maximizes itself out of the tray. This is rare, but enough that I could never nail down what was happening. Usually I noticed it happening when Windows became bogged down with a problem. 4) I've had it conflict with other extensions. For example, a Nagios FF extension works with the first open window, and no others. If the first window is a basic/generic window used only for tray purposes, then these other extensions don't work on subsequent windows. Eventually, because of these problems, it almost seemed like I shouldn't try to make a window start minimized. I started researching what it would take to simply do something along the lines of Mozilla's -turbo mode, where it only loads a tray icon not associated with a window. However, all of this was last year. If enough code changes have occurred that negate my 4 problems listed above, then starting out a window minimized will probably work great.

I am not sure how to use the tray icon service for multiple windows: - Install the example extension - Open two firefox windows - Close them one by one - Restore them Looking at the IDL, it seems that it takes a window and an ID of an element in that window, and then uses that ID (string) as the key for further referring to the systray icon - unfortunately, for multiple windows showing the same document, the IDs would necessarily overlap :( (Noting that this is a service...) (Random nit: in nsSystemTrayService::WindowProc, NS_ENSURE_SUCCESS is used, but that function shouldn't be returning nsresult)

(In reply to comment #93) > I am not sure how to use the tray icon service for multiple windows: > - Install the example extension > - Open two firefox windows > - Close them one by one > - Restore them > Looking at the IDL, it seems that it takes a window and an ID of an element in > that window, and then uses that ID (string) as the key for further referring to > the systray icon - unfortunately, for multiple windows showing the same > document, the IDs would necessarily overlap :( (Noting that this is a > service...) This is by design. My feeling was that multiple windows should all share the same tray icon. If e.g. multiple extensions want to have different tray icons, they will provide different IDs.

first stab at adding gtk2 support. Only has the tray part, not the hiding part (no WindowUtils) don't bother seriously reviewing this one. This is a WIP that's basically here just to have a snapshot of where I am. requires gtk2 2.10, I think. And this is my first stab at gtk, so I'm probably leaking all over the place, etc.

The code is really awful as it's one of my first attempt to make a XPCOM. The code compile and run with GTK 2.10 or higher and the icon is suppose to be cross desktop manager(at least KDE/Gnome/XFCE). http://www.freedesktop.org/wiki/Specifications/systemtray-spec?action=show&redirect=Standards%2Fsystemtray-spec

A firefox extension to quickly test the Icon on Linux. Full of bugs and memory Leak...

I've made the changes suggested by Neil. I'd really like to see this land in some form and make further changes and improvements after that.

It would be good if the singleton window code could be fixed up to restore from tray if necessary. A simple focus() call doesn't cut it: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/components/nsDefaultCLH.js&rev=1.10&mark=133#123

(In reply to comment #100) > It would be good if the singleton window code could be fixed up to restore from > tray if necessary. A simple focus() call doesn't cut it: > > http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/components/nsDefaultCLH.js&rev=1.10&mark=133#123 What else is needed besides focus()? Can you propose a patch?

Hi Matthew. I don't have time to look at the tray icon code right now so I can't propose an actual patch. What I meant, though, was that there needs to be some sort of restore_from_tray() call before that focus() call to ensure that if the application is minimized to tray it will be restored, else the focus() call doesn't work.

(In reply to comment #102) > Hi Matthew. I don't have time to look at the tray icon code right now so I > can't propose an actual patch. What I meant, though, was that there needs to be > some sort of restore_from_tray() call before that focus() call to ensure that > if the application is minimized to tray it will be restored, else the focus() > call doesn't work. Are you speculating or did you try this? I call window.restore() before calling focus(), and last time I tried it it worked fine.

Sorry Matthew, it turns out I've completely misunderstood one of our project's internal bug reports. I thought the report boiled down to "focus() is not enough", but in fact it was saying something different. The real issue is that we need to receive a notification to know specifically when the application is restored from the system tray. Apologies again.

(In reply to comment #99) > Created an attachment (id=275954) [details] > Updated extension for testing the system tray functionality > Doesn't seem to work with this build: Mozilla/5.0 (Windows; U; Windows NT 5.1; lt; rv:1.9a9pre) Gecko/2007100505 Minefield/3.0a9pre Plus, Minefield freezes when trying to access extension preferences..

Comment on attachment 275953 [details] [diff] [review] Updated trunk patch Apparently there are some issues building this with the current trunk, so let me do some testing, post a new patch and then do a major review-seeking charm offensive to try to get this checked in before it bitrots again.

> post a new patch and then do a major review-seeking charm > offensive to try to get this checked in before it bitrots again. Did you check with drivers if this will be able to go in before 1.9 in case it even gets reviews? We're well past the feature freeze and there are lots of 1.9 blockers that are prioritized by reviewers above the rest of bugs.

Yeah, I discussed that with Mike Connor yesterday. I'm leaning towards waiting til post 1.9 as being more realistic, considering the volume of higher priority 1.9 stuff that people have on their plates right now.

I've updated the code to build with the latest HEAD. I've also removed all conditional compilation for the 1.8 branch since it's probably not relevant anymore. On the other hand, I added conditional compilation for the whole enchilada with --enable-trayicon. I'm hoping this will facilitate getting this checked into the 1.9 branch since it would be turned off in Firefox anyway. There is probably an issue with naming. For "historical reasons" the project's directory and component DLLs are called "systray". The compilation options use "trayicon", which seems more descriptive. On the other hand, the official name of the place you put the icon is the "notification area", and of course on other platforms (which we want to support as well) it might be something else entirely. I'm leaning towards changing everything to "trayicon" derivatives but if people have better ideas, please let me know. Asking from review from Enn for XUL stuff, Neil or biesi for C++ and luser for makefile-related changes.

Comment on attachment 288857 [details] [diff] [review] Trunk patch Try not to diff configure next time, it makes your patch a lot longer than need be. Index: toolkit/components/systray/src/Makefile.in +LIBS += \ + $(NULL) + +SHARED_LIBRARY_LIBS = \ + $(NULL) Those seem to be useless, remove them? r=me on the configure.in and Makefile bits. Remember that you don't need to check in configure, just configure.in.

Comment on attachment 288857 [details] [diff] [review] Trunk patch > +<?xml version="1.0"?> > + <implementation> > + <constructor><![CDATA[ > + var _windowUtil = this._windowUtil; Just 'var windowUtil' should be used. No need for an underscore for local variables. > + var _hiddenCount = 0; Same here with hiddenCount. > + while (e.hasMoreElements()) > + { > + var win = e.getNext(); > + var trayIcon = this.getTrayIconElement(win); > + if (trayIcon && trayIcon._windowUtil.hidden) > + _hiddenCount++; The hidden count seems like something you could just compute again when needed rather than caching it. These kinds of things can easily get out of sync. > + <field name="_trayService" readonly="true"> > + "@mozilla.org/system-tray-service;1" in Components.classes &amp;&amp; > + Components.classes["@mozilla.org/system-tray-service;1"] > + .getService(Components.interfaces.nsISystemTrayService); Use CDATA blocks for these places instead of &amp; There are several blocks like this. > + <property > + readonly="true" > + name="windowCount" > + onget="var e = this._windowMediator.getEnumerator(window.windowtype); > + var count = 0; > + while (e.hasMoreElements()) { > + var w = e.getNext(); > + if (this.getTrayIconElement(w)) > + count++; > + } > + return count;" Use a <getter> here. > + <method name="getTrayIconElement"> > + <parameter name="aWindow" /> > + <body><![CDATA[ > + var element = aWindow.document.getElementById(this.id); > + if (element.tagName == "trayicon") Use element.localName not element.tagName and compare the namespace also. > + <method name="showIcon"> ... > + var ioService = > + Components.classes["@mozilla.org/network/io-service;1"]. > + getService(Components.interfaces.nsIIOService); > + imageUri = ioService.newURI(imageSpec, null, null); Maybe include the base uri (this.baseURI), so relative uris can be used? > + if (this.title) > + _trayService.setTitle(this.id, this.title); What if the title is empty? Is there some title from an earlier call that gets used? > + <method name="onClosing"> onClosing, onMinimizing and onActivating should be preceded by underscores. > + <method name="minimizeAll"> > + <body><![CDATA[ > + var ret = false; > + > + // minimize the current window first so we're sure the systray will > + // use its icon > + if (this.minimizeWindow(window)) > + ret = true; > + > + // then the others > + var e = this._windowMediator.getEnumerator(null); > + while (e.hasMoreElements()) { > + var w = e.getNext(); > + if (this.getTrayIconElement(w)) > + { > + if (this.minimizeWindow(w)) Skip this if w is the current window, which was already minimized above. > + <method name="restoreAll"> > + <body><![CDATA[ > + var e = this._windowMediator.getEnumerator(null); > + while (e.hasMoreElements()) { > + var win = > + e.getNext().QueryInterface(Components.interfaces.nsIDOMWindow); QueryInterface shouldn't be necessary. > + var trayIcon = null; > + if (trayIcon = this.getTrayIconElement(win)) No real reason to initialize to null here, as it just gets set again in the next line. > + var windowState = window.windowState; > + setTimeout("window.focus()", 50); setTimeout(window.focus, 50) doesn't work here? > + <handler event="blur"> > + var thePopup = document.getElementById(this.contextMenu); No one really uses the contextmenu attribute. You can check it too, but 'context' is more useful. > + <handler event="click" button="2" clickcount="1"> > + var thePopup = document.getElementById(this.contextMenu); Same here. > + var onshown = function(event) { > + var popup = event.target; > + > + // prevent infinite recursion (trips bug 174320) > + popup.removeEventListener("popupshown", onshown, true); > + > + var box = popup.popupBoxObject; > + // display on the right so the tooltip and/or edge of screen don't > + // get in the way > + var x = popup.targetX - box.width; > + var y = popup.targetY; > + > + // show the popup in the right position > + if (thePopup) > + popup.hidePopup(); > + document.popupNode = null; > + > + popup.showPopup( > + document.documentElement, > + x, y, > + "context", > + "", ""); > + } > + > + thePopup.addEventListener("popupshown", onshown, true); > + > + thePopup.targetX = event.screenX; > + thePopup.targetY = event.screenY; > + > + thePopup.hidePopup(); > + thePopup.showPopup( > + document.documentElement, // anchoring element > + -999999, // x > + -999999, // y > + "context", // type > + "", // popupanchor (ignored) > + ""); // popupalign (ignored) > + > + // need to handle rollup ourselves to prevent reentrancy into our > + // system tray WindowProc > + // during menu frame cleanup > + thePopup.enableRollup(false); > + I still don't understand all this hackery. At the very least, just use thePopup.openPopupAtScreen(x, y)

(In reply to comment #92) > > However, all of this was last year. If enough code changes have occurred that > negate my 4 problems listed above, then starting out a window minimized will > probably work great. > Did this ever work out for you? Last time I started tinkering with it, I wasn't able to get it to work.

Matthew, do you have an updated patch for this bug?

Gijs, Please see https://bugzilla.mozilla.org/show_bug.cgi?id=413517 for current status. I have vastly simplified the code, taking advantage of a lot of bug fixes and other improvements that have taken place since the original patch. I am planning to add menu support for both the Windows tray icon and Mac dock badges. I'll look into moving this back into toolkit once the dust has settled on the Firefox 3 release.

Hello all, - Is this patch to be functionnal with Xulrunner 1.9 just about to be released? - If yes, how can I build this using my Ubuntu station? - Anyone has knowledge of any roadmap of the Xulrunner project and the upcoming features? Is systray integration part of it? Should I wait a native functionnality? Thanks in advance

I'm glad to see some work on this finally getting done. Bug 413517 makes specific mention of Windows and Mac, but I'm curious as to the current state of Linux support. Also, now that Firefox3 has been released, is there any estimation of when the current work will make it in to trunk?

Matthew G: any update on this?

Well there is good tray support in Prism (see https://bugzilla.mozilla.org/show_bug.cgi?id=413517) that should be fairly straightforward to port to Firefox. There's been some talk of doing this, but I don't have any short-term plans to do so. I would definitely recommending dumping the code in this patch and using the Prism code instead (which is partially based on code from this bug).

Will this patch to be functionnal with Xulrunner? Any update on this specifically?

If someone ports the implementation from https://bugzilla.mozilla.org/show_bug.cgi?id=413517 then it should work in XULRunner. I have plans to do this in the next couple of months, but if someone wants to do it before then I certainly wouldn't object. If so, please let me know so we can discuss the approach.

In case people haven't seen this, there is a tray extension for Linux called FireTray http://code.google.com/p/firetray/ IIRC it seems to be functionally analogs to MinimizeToTray in Windows. -it provides a persistent tray icon -can hide individual or all windows -and can handle minimize and close actions -allows application to start-up hidden (which seems to be indefinitely broken in MinimizeToTray). Without too much work, it could also handle command line switch. It also looks to be a much simpler/easier to use in-chrome design with out losing any functionality. Regardless, it looks to provide a pretty good reference implementation for GTK tray code

This bug is now abandoned for almost a year and should be ASSIGNED to someone else.

Matthew Gertner or :nossralf, is anyone of you planning to work on this? If not, perhaps we should unmark this as ASSIGNED?

Last patch attached here was 2007, looks like unassigned is the right move per last two comments...

Just coming in here from the closed "WONTFIX" "turbo-mode" bugs.

XULRunner has been removed from the Mozilla tree: see https://groups.google.com/forum/#!topic/mozilla.dev.platform/_rFMunG2Bgw for context. I am closing all the bugs currently in the XULRunner bugzilla component, in preparation for moving this component to the graveyard. If this bug is still valid in a XULRunner-less world, it will need to be moved to a different bugzilla component to be reopened.