100846 - turbo: should get back on system tray after explorer.exe crash

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Jesse Ruderman]

Mozilla should re-add itself to the system tray after explorer.exe crashes.  I 
think there's a standard API for this, but I don't know what it is or where to 
find it.  (Apparently, most application developers also aren't familiar with 
this API, because out of 8 programs on my system tray, only AIM and the volume 
control survive an explorer.exe restart.)

Comment 1

[Warner Young]

You need to register a message with Windows, with:
  g_uTrayRestart = RegisterWindowMessage( TEXT( "TaskbarCreated" ));
Then watch for that message coming into your main tray app window.  If you get
this message, you should add the tray icon to the taskbar again.

Comment 2

[Jesse Ruderman]

Ok, now I know where I heard about the API: bug 18729.  (I was able to find it 
be searching for "taskbarcreated".)  Bug 18729 is about using a system tray 
icon for new mail notification.

Comment 3

[Warner Young]

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

Comment 4

[Brian 'netdragon' Bober]

I'll take this. Jesse, feel free to assign to me.

Comment 5

[Asa Dotzler [:asa]]

-> default assignee

Comment 6

[timeless]

netdemon volunteered

Comment 7

[Brian 'netdragon' Bober]

Adding URL
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/shellcc/platform/Shell/Shell_Int/Taskbar.asp

Info in the Taskbar Creation Notification section

Comment 8

[Brian 'netdragon' Bober]

Attachment: Patch diff -u against server

Changes:

1) Added response to WM_CREATE
2) Added response to the special message created in WM_CREATE (assigned by
explorer)
3) Cleaned up the code a little by putting into a switch statement

Comment 9

[Brian 'netdragon' Bober]

This file needs r and sr. Law, this is your area I think.

Comment 10

[Brian 'netdragon' Bober]

Attachment: Update to patch - prev. is obsolete

Fixed the tabbing a little.

Comment 11

[Brian 'netdragon' Bober]

Attachment: Update again - prev. 2 obsolete

I just noticed I was missing a break. Ugh.

Comment 12

[Bill Law]

Brian, that's the code we want, but it needs a bit of cleanup and re-packaging,
I think:

1. Do the RegisterWindowMessage when we create the system tray icon.  We only
need to do it when we're in turbo mode and that's the best place.

2. We need to stop looking for this message when we remove the systray icon.  An
easy way to accomplish that is to reset mTrayRestart to its unset/initial value.

3. I think nsNativeAppSupportWin::mTrayRestart needs an initializer (to the same
value).

4. Avoid changing the indentation level on all those lines.

Comment 13

[Brian 'netdragon' Bober]

I thought the messagewindow was only created when the icon was there. But I
tested with quicklaunch disabled and found the icon reappeared when explorer
restarted. :-)

I moved mTrayRestart to nsNativeAppSupportWin and made it only register when the
icon is added. I also added an mTurboEnabled flag to tell when it was enabled so
that it could be checked in the callback because otherwise there would be no way
to know from mTrayRestart. The reason for this is because the value 0 is used by
WM_NULL (whatever that is used for) and any other value is fair game for
Explorer if not already used.

Comment 14

[Brian 'netdragon' Bober]

Attachment: Diff -u from mozilla/.. dir (All prev obsolete)

I also fixed the tabbing on the lines that carried over since I think that was
what you were talking about.

Comment 15

[Bill Law]

Much better...but:

1. Your patch is about 500 lines long and there's maybe 20 new lines of code. 
I'd much, much prefer to have the patch just communicate what you're changing. 
I don't think adding a check for one more window message warrants switching from
if/else to a switch, especially since you're not even adding a case for the
restart message (because it's not a constant).  And you need to make your editor
stop putting in tab characters!  They are evil (when editing code).

2. There is no need for the mTurboEnabled flag.  It is identical to
!!mTrayRestart.  Just use "else if ( mTrayRestart && msg == mTrayRestart )...".

Comment 16

[Brian 'netdragon' Bober]

I realized that this morning when I woke up that I could have done a check for !
mTrayRestart. I could have added an extra check. Would it be a good idea to 
leave mTurboEnabled as a more intuitive way for people to check whether its 
enabled or not?

Also, I wasn't sure what you meant by tabbing. I had been told by a couple 
people in #mozilla a while back that tabs were good. I went back there today 
and now they are telling me tabs are bad and to use spaces instead. With tabs 
and spaces used equally throughout the code, I didn't know which to use. Ugh.

I can't fix it this weekend because I'm away, but I'll fix it when I get home 
on Monday.

Comment 17

[Nahor]

The url got screw up:
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/shellcc/platform/Shell/Shell_Int/Taskbar.asp

Comment 18

[Brian 'netdragon' Bober]

Attachment: Diff -u from mozilla/.. dir (All prev obsolete)

This fixes tabbing problems and was tested extensively.

Comment 19

[Brian 'netdragon' Bober]

Fixing URL

Comment 20

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

Comment on attachment 81341 [details] [diff] [review]
Diff -u from mozilla/.. dir (All prev obsolete)

+     return true;

use TRUE, not true.

And, on the next line: Why check if nsNativeAppSupportWin::mTrayRestart is not
0? Will such a message ever occur?
if not please remove this check

r=biesi with that

Comment 21

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

oh yeah, one more thing, please use the same indentation as the rest of the
file, and don't change the style of the DefWindowProc call

Comment 22

[Brian 'netdragon' Bober]

biesi: It can be 0 if turbo isn't active

Comment 23

[Brian 'netdragon' Bober]

Attachment: Final (I hope) patch

A little more elaboration on the mTrayRestart issue. It has been mentioned that
we should possibly use the mServerMode instead for checking if the icon is up,
but that would not be a good idea since there is a possibility in the future
the icon might not be up by a pref setting even if turbo is on. Its much safer
to use mTrayRestart == 0 because it only gets set when the icon is physically
added. If RegisterMessage ever fails, then the icon will not appear when
explorer restarts. Hopefully, that won't ever fail. If people start saying that
occasionally the icon doesn't reappear, we will know why. Then we can find a
workaround (maybe calling it multiple times or something). It _shouldn't_ fail
- and if it does there really isn't much we can do (maybe a low mem issue and
mozilla will probably crash anyway along with explorer and everything else). 

Thanks for noticing the true -> TRUE. That wouldn't have been a problem except
for performance. Every message loop would have to have that variable converted
which would have been a slight performance hit.

I realligned the code.

Comment 24

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

ok... also, you change:
-  return DefWindowProc( msgWindow, msg, wp, lp );
to
+  return DefWindowProc(msgWindow, msg, wp, lp);
leave that as it was

Comment 25

[Brian 'netdragon' Bober]

Attachment: The final patch (Yeah, I said that before :-) - all others obsolete

- patch.

Comment 26

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

Comment on attachment 81658 [details] [diff] [review]
The final patch (Yeah, I said that before :-) - all others obsolete

r=biesi

Comment 27

[jag (Peter Annema)]

Comment on attachment 81658 [details] [diff] [review]
The final patch (Yeah, I said that before :-) - all others obsolete

I just asked Bill to take another look at this patch since he reviewed (and had
objections to) the previous patch

Comment 28

[Bill Law]

+    Not many windows programmers realize this, but we
+    are special ;-) Bug 100846 */

The only thing I ask is that you please remove this bit of comment.  It will
make be cringe every time I edit this file for the rest of eternity.  Speaking
for myself at least, I don't think we're all that special, and I suspect *real*
Windows programmer will laugh in our faces.  It just isn't appropriate, IMHO.  

cvs will remember the bug number (as if anybody will ever care).

But the code is fine.

Comment 29

[Brian 'netdragon' Bober]

Attachment: diff -u (previous obsolete)

Removes text that says we are special ;-)

Comment 30

[Brian 'netdragon' Bober]

test - disregard this (I want to see the list of who it goes to)

Comment 31

[jag (Peter Annema)]

Comment on attachment 82180 [details] [diff] [review]
diff -u (previous obsolete)

sr=jag. Adding r=biesi from previous patch since the change was the mere
removal of a comment.

Comment 32

[Brian 'netdragon' Bober]

Thanks jag. I emailed [e-mail]

Comment 33

[Brian 'netdragon' Bober]

[e-mail] said this will probably be put in after 1.0 is out the door.

Comment 34

[Judson Valeski]

please checkin to the 1.0.1 branch ASAP, and replace the "mozilla1.0.1+" keyword
with "fixed1.0.1" once landed.

Comment 35

[John Morrison]

Whoa, whoa, whoa. I appreciate the code and functionality and all, so don't take
this the wrong way. But how can this be approved to land on 1.0.1 branch when it 
hasn't even landed on the trunk?

There is a patch on bug 98673 that changes the way that turbo works with code 
changes in nsNativeAppSupportWin.cpp [where this change is also implemented]. 
It has been baking on the trunk, has been well tested and is waiting for driver 
approval to land on the 1.0.1 branch (already has adt1.0.1+).

There is no way that this change should land directly on the 1.0.1 with landing
on the trunk first. I'm probably breaking protocol, but I'm removing the
mozilla1.0.1+. Request it again after this bug is verified/fixed on the trunk.

Comment 36

[Brian 'netdragon' Bober]

I looked at the code in bug 98673 and it doesn't appear to overlap.

Comment 37

[Brian 'netdragon' Bober]

The patch has been checked in by timeless on the trunk and there were no build
problems from this patch. Therefore, I am changing the keyword back to
mozilla1.0.1+. If in a day or two we don't see any problems caused or
regressions in bug 98673, then per Valeski's recommendations this should
probably be checked into the branch.

Comment 38

[Blake Ross]

I don't see the benefit of landing this on the branch.  Even though this looks
innocuous, I would rather we not touch branch turbo code unless it's for
critical reasons.

Comment 39

[Peter Trudelle]

I tend to agree, there is some unknown amount of risk here, too much for fixing
a low impact defect this late in the release cycle.

Comment 40

[Brian 'netdragon' Bober]

Trudelle, Blake:

Do you want it to go into the branch after the release of 1.0 (and start of
1.0.1), or not into the branch at all?

I think Valeski wanted it in after 1.0 was released.

Comment 41

[Stephen P. Morse]

I agree with Peter and Blake.  The probability of the user encountering this 
situation is low.  Furthermore, the consequences when this does occur are not 
that devastating.  So IMO, this is not worth the risk of a regression this late 
in the game.

Comment 42

[Peter Trudelle]

I think it should stay on the trunk until after 1.1/MachV.  

Comment 43

[Judson Valeski]

minusing for 1.0.1 as it's too late for this change on the branch.

Comment 44

[Brian 'netdragon' Bober]

What is the status on this? Is this bug fixed? I'm not sure to what extent its 
been checked in yet. We are past 1.1, is it still on the trunk?

Comment 45

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

fwiw, this has been fixed on trunk since may 30 2002, 21:31, when timeless
checked this in.

Comment 46

[Randell Jesup [:jesup] (needinfo me)]

a=[e-mail] for 1.0 branch; please change mozilla1.0.2+ to fixed1.0.2
when checked in

Comment 47

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

on 1.0 branch:
patching file `mozilla/xpfe/bootstrap/nsNativeAppSupportWin.cpp'
Hunk #1 succeeded at 372 (offset -6 lines).
Hunk #2 FAILED at 922.
Hunk #3 succeeded at 971 (offset 32 lines).
Hunk #4 succeeded at 2365 (offset 182 lines).
Hunk #5 succeeded at 2227 (offset 32 lines).

I manually applied hunk 2, hope I did it correctly.

anyway, I checked in, marking fixed

Comment 48

[Brian 'netdragon' Bober]

I hope so too, and until we hear otherwise, this bug is:

****** *****  *    *  ****** ****
*        *    *    *  *      *   *
*        *     *  *   *      *    *
***      *      **    ***    *    *
*        *     *  *   *      *    *
*        *    *    *  *      *   *
*      *****  *    *  *****  ****

AND

*   * ****** ****  ***** ****** ***** ****** ****
*   * *      *   *   *   *        *   *      *   *
*   * *      *   *   *   *        *   *      *    *
*   * ***    ****    *   ***      *   ***    *    *
*   * *      *   *   *   *        *   *      *    *
 * *  *      *   *   *   *        *   *      *   *
  *   *****  *   * ***** *      ***** ****** ****

Comment 49

[Gayatri Rimola]

Terri, should this bug be marked verified1.0.2 since it is verified in the branch?

Comment 50

[Terri Preston]

Verified win xp branch build 2002100208, adding verified1.0.2 keyword

Comment 51

[Brian 'netdragon' Bober]

A final update (hopefully). This appears fixed in the commercial product too. 
Netscape 7.0.1 is going to be released soon. I tested whether this is fixed on 
7.0.1 and it is. :-)