53345 - Mac OS notification for Mozilla alerts

Status: RESOLVED FIXED

(Core :: XUL, defect, P3)

Description

[Adam]

Mozilla M18, Mac OS 9.0.4 - If you hide Mozilla/make another app active, there
is no way to tell if an error occured unless you make Mozilla active again. On
the Mac, apps will usually inform users of errors by flashing their icon in the
application menu.

Comment 1

[sairuh (rarely reading bugmail)]

simon/pink, d'you know which engr should get this?

Comment 2

[Simon Fraser [no longer active]]

Toolkit bug -> trudelle

Comment 3

[Peter Trudelle]

Pink recalls AIM doing some work on notification mgr for their needs, cc syd.
This should be in toolkit, and general purpose for all platforms.

Comment 4

[Mike Pinkerton (not reading bugmail)]

We need to mimic the behavior of the ::Alert()/::ModalDialog() toolbox calls, 
which use the notification manager if the app is in the bg. Since we're rolling 
our own modal stuff, not using the system, we don't get this behavior for free.

if we're looking for an owner, it should probably be danm.

Comment 5

[Syd Logan]

Yes, there is a "GetAttention" interface function on nsIDOMWindow for that. 

Comment 6

[Peter Trudelle]

mass-acceptance

Comment 7

[Matthew T (active 1999-2002)]

Is there an equivalent bug for flashing the taskbar button on Windows, or is this 
bug covering both OSes?

Comment 8

[Matthew T (active 1999-2002)]

To answer my own question, the equivalent Windows bug is bug 42212. Resummarizing.

Comment 9

[Adam]

Does anyone else agree that this deserves some kind of nomination? Too many
times have I switched to my email client (or some other app), thinking that a
page is gracefully loading in the background (over my 56K-only-connects-at-28.8K
piece-of-crap Apple internal modem), only to find that an error has occured. A
real waste of time that can be resolved by flashing the application menu when
Mozilla is in the background.

Comment 10

[Peter Trudelle]

->danm, moz1.0

Comment 11

[Greg K.]

I believe the applicable Inside Macintosh documentation may be found at [http://
developer.apple.com/techpubs/mac/Processes/Processes-105.html#HEADING105-0].

Comment 12

[Henri Sivonen (:hsivonen) (away from Bugzilla until 2026-08-31)]

Should I file another bug about making the Dock icon jump on Mac OS X 10.1 or
does this bug cover that case, too?

Comment 13

[Simon Fraser [no longer active]]

No. Icon-jumping happens as a side-effect of calling the Notification manager.

Comment 14

[Dan M]

The basic code to fix this has already been written; it just needs to be hooked 
up. Attaching a patch to do so. Note it's Windows and Mac, not unix -- unix 
already does this. (Also note it doesn't work terribly well on Windows, probably 
because of the recent deprioritization of timer events. But it does work.)

Comment 15

[Dan M]

Attachment: hook up notification manager to window::SetFocus, Mac & Windows

Comment 16

[Simon Fraser [no longer active]]

Erm, do we really want to post notifications any time somone calls SetFocus() on 
a window? It seems like we should only ever post notifications if we're in the 
background, and even then only when there is an error condition (or something 
useful, like new mail).

Comment 17

[Dan M]

Point 1: GetAttention() itself does nothing if the window is already in the 
foreground. Point 2: you're right, this patch only complains if someone tries to 
raise a window to the foreground and is denied because Mozilla isn't the 
foreground app. It wants more GetAttention()s sprinkled around in the 
"interesting event just happened" arena.

Comment 18

[(not reading, please use [e-mail] instead)]

ooh, we'd love this for new mail.  (especially on the mac, where there's even IC
prefs for this behaviour).

Comment 19

[Simon Fraser [no longer active]]

Note that 4.x has some pretty subtle new mail notification behaviour w.r.t this. 
If Communicator is in the background, and you get new mail, then it (optionally) 
beeps, and starts flashing its icon the applcation menu (via the Notification 
manager). When you bring 4.x to the foreground, it cancels the notification (and 
thus stops flashing the application menu), but swaps to flash the apple menu icon 
until the user goes to the mail window. If mail arrives while Communicator is in 
the foreground, but the mail window is not frontmost, then it flashes the apple 
menu until the user goes to read mail.

Comment 20

[Dan M]

Alright. Been thinking about this a little more. I propose that my patch above is 
actually pretty close to what we want. Lacking an explicit error-posing API, we 
have to rely on more general principles. The patch will fire a notification any 
time Mozilla attempts to bring a window to the foreground and fails. Any error or 
condition that involves a dialog or alert of some kind, when the app is in the 
background, will trigger the patch. Unfortunately so will any new window 
creation, or any attempt to focus the window.

Simon, what do you think of something like restricting the GetAttention call to 
only, say, modal windows, or perhaps commonDialog.xul windows?

By the way, arguably there are many other conditions where notifications would be 
appropriate. The Macintosh new mail notification for instance is covered by bug 
18730 (which is not to say I couldn't help out with those). I read this bug as 
being about the general case.

Comment 21

[Simon Fraser [no longer active]]

I think we want to be very restrictive in when we post notifications. I'd rather 
not do it at all than do it on every window creation when in the background. 
Can't we get code that really does care to call GetAttention() itself?

Comment 22

[Dan M]

Well then, commonDialog.xul. They're all alerts or dialogs or something that
wants your immediate attention. And email as in bug 18730.

Comment 23

[Mike Pinkerton (not reading bugmail)]

is there a way that we can call it only for certain window types? 

I agree with simon, we don't want this going off all the time. On OS9, it's not
a big deal if the menu icon is flashing a little too often, but shooooweee, on
OSX the bouncing dock icon will just piss off users. danm, if you haven't seen
the bouncing dock icon on OSX, i suggest you run, not walk, to the nearest OSX
box and have a look. Then you'll understand ;)

Comment 24

[Dan M]

Alright, sure. I agree with the generalization. Anybody want to comment on my 
specific suggestion, then? To be more specific, the patch is:

Index: mozilla/xpfe/global/resources/content/commonDialog.js
===================================================================
RCS file: /cvsroot/mozilla/xpfe/global/resources/content/commonDialog.js,v
retrieving revision 1.36
diff -u -2 -r1.36 commonDialog.js
--- commonDialog.js	2001/09/28 20:14:04	1.36
+++ commonDialog.js	2001/11/02 22:31:18
@@ -46,4 +46,6 @@
   gCommonDialogParam = 
window.arguments[0].QueryInterface(Components.interfaces.nsIDialogParamBlock);
 
+  GetAttention();
+
   // display the main text
   var messageText = gCommonDialogParam.GetString(0);

(for the Mac -- Windows needs some widget love before this could be checked in.) 
This should only bounce your dock when Mozilla tries to open an alert or password 
dialog or some such and it's not the foremost app. Joy for all. Why not run with 
it for a while and see what you think?

Comment 25

[Dan M]

...and the wind whistled. Anybody tried the patch? If you think it'll be OK -- 
and I encourage you to run with it, maybe use this for testing:

<html><head><script>
  function showalert() {
    alert("hi");
    window.setTimeout("showalert()", 5000);
  }
</script></head><body onload="showalert()">
this window throws an alert every five seconds
</body></html>

then by all means say so. If you hate it on principal, by all means say so and 
I'll just close this bug "won't fix" (with, as always, the reminder that this bug 
is about general notifications; there is a different bug specifically for mail 
notifications.)

Comment 26

[Greg K.]

Whoa, whoa, whoa, whoa, whoa, whoa, whoa, whoa, whoa, whoa, whoa.

Please, Dan, don't assume that because nobody commented that you should just
close this bug. I'd be happy to test it, but I can't/don't build. Is it checked
in for nightlies?

Comment 27

[Dan M]

No, the patch isn't checked in, so it's hard to test-run without building. Well
actually it's just chrome, so it's not too difficult to unzip the jar file from
a standard distribution, change the text file and rebuild the jar. It's
chrome/toolkit.jar, and the file is content/global/commonDialog.js within.
Stuffit/dropstuff should suffice.

I reiterate my threat to close the bug. I wish one of the objectors would just
try the (&*#$ patch. I think it's fine and I could find reviewers enough to
ramrod it through. But I'm hearing that People Who Care don't want a general
notification mechanism. If I'm hearing correctly, this bug has run up against
resistance enough to warrant its replacement by a series of smaller bugs of
lesser scope, like bug 18730. I just want to know if I'm hearing correctly.

Comment 28

[Simon Fraser [no longer active]]

Check it in, sr=sfraser. We'll whine later if we find it objectionable.

Comment 29

[Mike Pinkerton (not reading bugmail)]

i'll try the patch today. i'm having a bitch of a time getting a build that
actually works.

Comment 30

[Dan M]

Well, cool, then. I'll check it in soon. That'll spread out the task of whining 
to everyone. Moving to 0.9.7; after the tree closed for 0.9.6 seems like a bad 
time to experiment.

Comment 31

[Dan M]

New patch comin'. It doesn't affect the Macintosh or gtk behaviour, but does
Windows. Clearing review status and fishing for more.

Comment 32

[Dan M]

Attachment: notify on common dialogs if they're not foreground; fix Windows notification mechanism

Comment 33

[Dan M]

Attachment: notify on common dialogs if they're not foreground; fix Windows notification mechanism

Sorry. New version of the above patch. I was concerned about the slight
change it did make; it was trying to KillTimer for every nsWindow that was ever
destroyed. Probably this would do no damage, but it irked me. This version is
just the same, but only calls the OS's ::KillTimer when it knows a timer
actually exists.
  I'm doing it with a homespun timer object maintenance class. As explained in
source comments, a hashtable seemed like too much overhead for a list that'll
probably never have more than one element. And to keep the necessary status as
nsWindow member variables, I'd have to make nsWindow::GetNSWindowPtr public,
which seems like a bad idea.

Comment 34

[Syd Logan]

+  void AppendTimer(TimerInfo *info) {
+    if (!mHeadTimer)
+      mHeadTimer = info;
+    else {
+      TimerInfo *scan, *last;
+      for (scan = mHeadTimer; scan; scan = scan->next)
+        last = scan;
+      last->next = info;
+    }
+  }

What if info is NULL && !mHeadTimer? Then we don't go through the loop, last is
NULL, and last->next is a segfault. Is that possible, or need we check the info
arg for NULL and deal with it? if ( last ) last->next = info; else XXX; Fix or
respond, and I'll give an r=

Comment 35

[Dan M]

I think it's OK.
  info mHeadTimer
    y      n      mHeadTimer=info
    y      y      >=1 pass through loop guarantees last is not null
    n      n      mHeadTimer=info=0
    n      y      like (y,y) above, but last->next=info=0, which it already was

right? If we don't go through the loop, and last is (actually undefined, not
merely null) but that's OK; it has to go through the loop at least once if it
reaches the loop at all.

Comment 36

[Dan M]

The part of the above patch that makes the notification mechanism healthier on 
Windows builds is checked in. The commonDialog.js part that actually sends 
notifications lived on the trunk for one day before it was backed out because it 
caused bug 110494.

Comment 37

[Dan M]

Attachment: notification on alerts, all platforms (mac 'n windows, really)

This version is a modification of the previous patch's attempt to hook up
notifications on alerts. I've tested it on three bloody platform's browsers and
four bleeding embedding test apps, especially TestGtkEmbed. It's the exact same
patch but the additional lines have been moved to the end of the initialization
function. Shrug. Figure the same reviews apply?

Comment 38

[Dan M]

The patch has been in since Wednesday evening, and no objections yet. I claim
this is fixed, though I remain ready for repercussions.