Closed Bug 728106 Opened 12 years ago Closed 12 years ago

[OS X] Use Notification Center instead of Growl if available

Categories

(Toolkit :: UI Widgets, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: zpao, Assigned: jaas)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 6 obsolete files)

OS X 10.8 (Mountain Lion) is introducing Notification Center, so in the spirit of platform integration, we should use it. Of course, this is under the assumption that it's even available for use.
Blocks: tb-mac
I spent a little bit making this work. There are a couple issues with the API we get.

1. No images - that's not so bad but breaks some current expectations. The image shown will be the application icon.

2. It's yet to be seen how this will play out with app store vs signed vs not.

3. By default notifications will be persisted if no action is taken. This isn't so great for disappearing contexts but we can work around this (clear them on delivery & force them not to persist or figure something out so we don't leak from those contexts)

4. If no action is taken on a notification, there's no easy way to know this. In growl we got notified if a notification timed out (so long as it had a clickContext attached), so we could always notify "alertfinished". Without knowing this, we will need to do something funky (either just notify "alertfinished" on delivery or maybe check on some timeout to see if it's visible) I can file a bug to see if we can get this, but I'm not sure how important it really is.
"The most significant point they raise is that the Mountain Lion notification functionality will only be available to applications that are sold through the Mac App Store."
http://arstechnica.com/apple/2012/02/growl-devs-respond-to-mountain-lions-notification-center/
I have seen this information (Notification Center only for Mac App Store App) in many places.

Doesn't it make it impossible for Firefox to add support of Notification Center?

Growl's developers stated that they are investigating options for integrating Growl with Notification Center
http://growl.posterous.com/growls-response-to-notification-center-welcom
(Current and future versions of Growl are distributed through the Mac App Store.)
I've heard that said too, but in practice I haven't seen that to be the case (eg, Chrome works, my build works).

Regardless though, we could probably make this fallback to Growl if Notification Center is not available due to whatever restrictions are in place (assuming 10.8)
I asked on the dev forums .. https://devforums.apple.com/message/659391

I think we can interpret that answer as: yes, local notifications work on signed apps.
Attached patch Patch v0.1 (WIP) (obsolete) — Splinter Review
I know there's still some progress to be made for sure (like, making it build on < 10.8 and deciding when to use growl conditionally) but this works. It's based *heavily* on the current growl code.

There are also some decisions to be made about our usage:
1. Do we use the default notification sound and leave it up to the user to turn that off in sys prefs? (my impl: yes)
2. Should we force alerts even when NC decides they shouldn't be shown? (my impl: yes)
3. What should we do about notification persistence? (my impl: disable, but that feels too naive)
4. Do we care enough about alertfinished to file a bug for Apple and try to get something similar or hack our way around? (my impl: no)
Assignee: nobody → paul
Status: NEW → ASSIGNED
Attachment #626175 - Flags: feedback?
Attachment #626175 - Flags: feedback? → feedback?(smichaud)
I'll go through this and test it tomorrow on 10.8.

Paul, did you test on 10.8 with a signed copy of FF?  And if so did you just sign it yourself?  If so, I can presumably just sign it myself (using my "Developer ID Application Certificate").

Is it necessary to sign FF to make this stuff work?
Nope, I've been running this in my own unsigned build and it works. I may have at some point had to approve the app but I don't remember. If there's a way to clear out some permission list (I think you may have mentioned something in the signing bug) then I could try that.
> If there's a way to clear out some permission list (I think you may
> have mentioned something in the signing bug) then I could try that.

Everything I currently know about this is in bug 752613 comment #20.

But rereading my own comment, I realize that people doing their own
custom builds probably don't need to bother with signing, even if the
code (in principal) requires it.

There's a gaping hole in Mountain Lion's signature checking -- you can
run an unsigned app, by double-clicking on it, if it doesn't have any
extended attributes (viewable using xattr -l).  Builds you do yourself
almost certainly won't.

Builds dragged from a dmg that's itself been downloaded in a web
browser *will* have the appropriate extended attributes.  So just to
be sure we should test with custom builds that have been "packaged"
and downloaded in this fashion.
I didn't have time yesterday to get to this.  Hopefully I'll have more time today.

Paul, did you find any documentation on the Mountain Lion Notification Center (other than NSUserNotification.h in the Foundation framework's Headers directory)?
(In reply to Steven Michaud from comment #9)
> Paul, did you find any documentation on the Mountain Lion Notification
> Center (other than NSUserNotification.h in the Foundation framework's
> Headers directory)?

No, that's all I've seen and I think it's the place that was suggested in several threads in the dev forums.

I also started a thread about getting notified when a notification disappears (for alertfinished) but they said to file a bug if we thought it was important enough which I never did. https://devforums.apple.com/message/655426
My time for this keeps bleeding to other things (like dealing with signing on OS X 10.8).  So I'll probably have to keep at it into next week.

I'm finally able to build on 10.8, and have a build with this patch going.

But now it occurs to me I don't know how to test it -- I don't know how to trigger the sending of a notification.  Please let me know!
Easy way is to download a file but there's no control there. I usually run the following in scratchpad (browser context):

> function observe(subject, topic, data) alert(topic);
> let as = Cc["@mozilla.org/alerts-service;1"].getService(Ci.nsIAlertsService);
> as.showAlertNotification(null, "title", "message", true, null, observe)
Comment on attachment 626175 [details] [diff] [review]
Patch v0.1 (WIP)

I've done a bunch of testing on this, and everything I tested looks
fine.

I'd hoped to try to find a workaround for the "disappearing"
notifications (ones that the user doesn't click on, and which
therefore cause us to persist ObserverPair objects until the window
gets closed).  But other things have intervened, and I won't have any
more time this week to work on this bug.

As you must be aware, the current code will only compile on OS X 10.8.
So someone needs to find out exactly how much info needs to be copied
from NSUserNotification.h into one of our header files, suitably
ifdefed so that it only gets included when compiling on earlier
versions of the OS (or with their corresponding SDKs).

We've done that many times.  So all over the tree you'll find code in
sections like:

if !defined(MAC_OS_X_VERSION_10_N) ||
   MAC_OS_X_MAX_ALLOWED < MAC_OS_X_VERSION_10_N

The header information from NSUserNotification.h would need to go into
such a section, where N == 8.

You'd also need to add code to prevent the Notification Center stuff
from being executed on any OS prior to 10.8.  There are various	ways
to do this -- for example using a call to Gestalt() or calling
respondsToSelector.

Good luck with this.  If you're not able to find time for it, I	now
know enough that I should be able to get to it before too long.
Attachment #626175 - Flags: feedback?(smichaud) → feedback+
(In reply to Steven Michaud from comment #13)
> I'd hoped to try to find a workaround for the "disappearing"
> notifications (ones that the user doesn't click on, and which
> therefore cause us to persist ObserverPair objects until the window
> gets closed).  But other things have intervened, and I won't have any
> more time this week to work on this bug.

So we may be able to just keep the notifications in NC instead of clearing them immediately like I'm doing. Then if the observer goes away (GCed weak observers?) then we remove the notification. I don't know that that's even possible...

I thought about trying to stuff the observer in the NSDictionary we can attach to the notification, but I figured that's not easily doable or we would have done that for Growl.
Blocks: 728385
Blocks: 767083
Normally in Notification Center on 10.8 I get this Growl-like notification and after that I find the notification also in the Notification Center itself (if I click the icon in the menu bar). And if I click on this notification, the application/file opens. With this patch FF/TB notifies me with the Growl-like message, but I don't see this message than in the Notification Center. Is this by purpose or is this feature not yet in the patch?
(In reply to Nomis101 from comment #15)
> Is this by purpose or is this feature not yet in the patch?

Right now that's intentional (see #3 in comment #5). It's sort of a limitation of how we've expected the previous incarnations of this to work. If a Growl notification goes away, it's gone. If a toaster alert (windows) goes away, there's no way to take action on that later. So I wrestled with that here. We probably want it to behave like other MoLo notifications, but I think we'd be at a good place integrating with Notification Center at all.
(In reply to Paul O'Shannessy [:zpao] (no longer moco, slower to respond) from comment #16)
> (In reply to Nomis101 from comment #15)
> > Is this by purpose or is this feature not yet in the patch?
> 
> Right now that's intentional (see #3 in comment #5). It's sort of a
> limitation of how we've expected the previous incarnations of this to work.
> If a Growl notification goes away, it's gone. If a toaster alert (windows)
> goes away, there's no way to take action on that later. So I wrestled with
> that here. We probably want it to behave like other MoLo notifications, but
> I think we'd be at a good place integrating with Notification Center at all.

Ah, thanks for explaning. I thought maybe for Thunderbird it would be nice if you could click on the message in the Notification Center and than this specific mail openes in TB.
(In reply to Steven Michaud from comment #13)
> Comment on attachment 626175 [details] [diff] [review]
> Patch v0.1 (WIP)
> You'd also need to add code to prevent the Notification Center stuff
> from being executed on any OS prior to 10.8.  There are various	ways
> to do this -- for example using a call to Gestalt() or calling
> respondsToSelector.

Note that Gestalt() is deprecated since 10.8!
According to the Growl blog[1], if we upgrade to the Growl 2 framework then it will fallback to notification center if Growl is not running (opposite of the bug summary). This would mean we wouldn't have to write notification center specific code at all which hopefully means less maintenance

As the blog post puts it:
"The benefit of this is that you do not need to rewrite your code, you'll simply drop in an updated Growl.framework… users who want Growl are happy, users who just want NC are happy, etc etc."

Something to consider.

[1] http://growl.posterous.com/going-forward-with-growl-and-notification-cen
(In reply to Paul O'Shannessy [:zpao] (no longer moco, slower to respond) from comment #16)
> (In reply to Nomis101 from comment #15)
> > Is this by purpose or is this feature not yet in the patch?
> 
> Right now that's intentional (see #3 in comment #5).
Aaah, now I get what you have meant. With removeDeliveredNotification:notification the notification will be removed from the notification center while it is created. If I remove this part, I see it in the notification center. :)


(In reply to Matthew N. [:MattN] from comment #19)
> According to the Growl blog[1], if we upgrade to the Growl 2 framework then
> it will fallback to notification center if Growl is not running (opposite of
> the bug summary).
But this would mean I need to uninstall or disable Growl if I want to use the Notification Center. But this would than be bad for the other Growl-using apps.


There is now also terminal notifier, which has interesting notification center code pieces:
https://github.com/alloy/terminal-notifier/blob/master/Terminal%20Notifier/AppDelegate.m
This is the Chromium implementation of the Notification Center:
http://codereview.chromium.org/10021026/
It would be better to provide an option to switch between Growl and Notification Center in the preferences. Put it on the Advanced tab as a combo list, and default to Notification Center on OS X 10.8, Growl for anything less.
Or make it a hidden pref. 

Actually, isn't it possible to disable Growl for specific applications in the settings? I have Growl 1.2.2 and there you can deactivate applications in the Growl system prefs panel. If there is a way to programmatically detect whether Fx is disabled in the prefs, you can fall back to NC. 

The question is, do we want to keep Growl around forever? E. g. do we drop Growl support if OS X 10.7 loses support (thus the last version without NC)?

If the answer is yes, it would be better to address NC directly NOW to avoid work overhead (thus Growl code can just be removed and everything works as expected). 

If the answer is no, I see no reason to not use the newest Growl Framework (as soon as it is available) IF the framework supports a "prefer NC over Growl" switch – though, it has to be discussed whether such a control is actually necessary. 

Alright, second big question: If Growl is available (so a user decided to prefer Growl over NC [1]), do we really need a switch to decide whether Growl or NC should be used? Or can we just go with whatever the Frameworks decides to do? [2]

[1] Please bear in mind that Growl SDK 1.3 does not need an App to display notifications, it has a built-in simple notification called "Mist". You only need the Growl app (as an end user) to change the appearance of the notification, and gain more control over what is displayed in what way. 
[2] As mentioned above, you can disable specific applications from showing Growl notifications. Hopefully, "the new Growl" allows to redirect messages to NC in that case thus Fx does not need a switch at all! 


(Fx actually means Platform, of course.)
Paul - seems like you're not working on this any more, mind if I take it?
Attached patch fix v0.2 (obsolete) — Splinter Review
Updated to apply to current trunk, no other progress yet.
Assignee: paul → joshmoz
Attachment #626175 - Attachment is obsolete: true
Comment on attachment 650800 [details] [diff] [review]
fix v0.2

Uploaded the wrong patch, this doesn't include new files.
Attachment #650800 - Attachment is obsolete: true
Attached patch fix v0.3 (obsolete) — Splinter Review
Josh, have you considered instead upgrading the Growl framework so that all Mac users get notifications via Mist and/or Notification Center even if they don't have Growl installed?
(In reply to Matthew N. [:MattN] from comment #29)
> Josh, have you considered instead upgrading the Growl framework so that all
> Mac users get notifications via Mist and/or Notification Center even if they
> don't have Growl installed?

I'm planning to remove all Growl support after notification center support is added, so that's not on my agenda. See bug 777409.
Attached patch fix v0.4 (obsolete) — Splinter Review
Progress. Still have some issues to clean up, need to add growl/nc detection support, and make it compile on < 10.8.
Attachment #650806 - Attachment is obsolete: true
Attached patch fix v0.8 (obsolete) — Splinter Review
This patch is about done, functionally, imo. The only thing left to do is get it compiling on 10.6 and 10.7. I can't do that right now, will get to it soon.

I made some behavioral changes from zpao's patch. Most notably:

1. Don't insist on showing our notifications against OS settings and user wishes.

2. Dropped support for further action as a result of clicking on a notification. The Notification Center APIs make this hard if not impossible to do correctly per how notifications work internally to Gecko right now. We can probably get this working but it'll likely require changes to how our internal notification system works. I think we should take care of it in a followup bug where we can separately examine things carefully.
Attachment #650831 - Attachment is obsolete: true
Hm, the old zpao's patch worked just nicely for me with Thunderbird. Your v0.8 patch doesn't give me any notification at all. Is this on purpose?
(In reply to Nomis101 from comment #33)
> Hm, the old zpao's patch worked just nicely for me with Thunderbird. Your
> v0.8 patch doesn't give me any notification at all. Is this on purpose?

My guess is that you're looking for a notification when the app is in the foreground. By default OS X generally chooses not to show notifications if your app is the currently active app. zpao's patch overrode that behaviour, erroneously imo, so that the notification would always show. Is this your issue?
(In reply to Josh Aas (Mozilla Corporation) from comment #34)
> (In reply to Nomis101 from comment #33)
> > Hm, the old zpao's patch worked just nicely for me with Thunderbird. Your
> > v0.8 patch doesn't give me any notification at all. Is this on purpose?
> 
> My guess is that you're looking for a notification when the app is in the
> foreground. By default OS X generally chooses not to show notifications if
> your app is the currently active app. zpao's patch overrode that behaviour,
> erroneously imo, so that the notification would always show. Is this your
> issue?
Could be, but I checked with Thunderbird in the background and I also don't get any notification. Maybe something in mailnews/base/src/nsMessengerOSXIntegration.mm needs to be changed, so TB can use the Notification Center with your new patch? This foreground behaviour wasn't with Growl, right? This is since NC? Because Growl worked nicely with Thunderbird in the foreground.
Attached patch fix v1.0 (obsolete) — Splinter Review
This includes compile-time backwards compatibility for 10.7 and 10.6. Not pretty, but it works.
Attachment #650955 - Attachment is obsolete: true
Attachment #652514 - Flags: review?(smichaud)
Comment on attachment 652514 [details] [diff] [review]
fix v1.0

Looks fine to me (though I haven't compiled or tested it).
Attachment #652514 - Flags: review?(smichaud) → review+
With patch v1.0 I also get a new mail notification center message for Thunderbird. But only if TB is in the background (as assumed in #34). Can this background behaviour be changed in MailnewsCore (so that TB also displays a message if it is in the foreground) or must this be done directly in Core?
Comment on attachment 652514 [details] [diff] [review]
fix v1.0

Review of attachment 652514 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for picking this up, Josh! I was going to respond sooner but saw you'd already started working on it so didn't feel the need to spam.

I do have a concern about this right now... I'm afraid we won't follow up on hooking response actions into the notifications. I agree that the situation is suboptimal, but I worry we won't put the work into figuring out how to make it better quickly enough and we'll end up with notifications that aren't as useful as they used to be.

::: toolkit/components/alerts/mac/nsGrowlAlertsService.mm
@@ +111,5 @@
> +  {
> +    NS_OBJC_BEGIN_TRY_ABORT_BLOCK;
> +
> +    mDelegate = [[mozNotificationCenterDelegate alloc] init];
> +    

Nit: whitespace that I may have put there.

@@ +185,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  // Only use Growl with OS version < 10.8.
> +  SInt32 osVersion = 0;
> +  OSErr err = ::Gestalt (gestaltSystemVersion, &osVersion);

Is it not possible to take advantage of the version checks we do in widget? We'd need to add 10.8 checks in nsCocoaFeatures, but if we can reuse that...
(In reply to Nomis101 from comment #38)
> With patch v1.0 I also get a new mail notification center message for
> Thunderbird. But only if TB is in the background (as assumed in #34). Can
> this background behaviour be changed in MailnewsCore (so that TB also
> displays a message if it is in the foreground) or must this be done directly
> in Core?

The way this patch is written it must be done in Core. I don't think it's completely crazy to put a pref in front of this so that if set, we would show it (and thus not respect OS setting). But I think it might be better to say that users should flip that in system settings. Though now that I'm looking, I don't see anything to specify that...
(In reply to Paul O'Shannessy [:zpao] (no longer moco, slower to respond) from comment #39)

> Is it not possible to take advantage of the version checks we do in widget?
> We'd need to add 10.8 checks in nsCocoaFeatures, but if we can reuse that...

I looked into it, didn't seem worth it. If we add any more version checking we can create a re-usable function.

(In reply to Paul O'Shannessy [:zpao] (no longer moco, slower to respond) from comment #40)

> The way this patch is written it must be done in Core. I don't think it's
> completely crazy to put a pref in front of this so that if set, we would
> show it (and thus not respect OS setting). But I think it might be better to
> say that users should flip that in system settings. Though now that I'm
> looking, I don't see anything to specify that...

I don't see why you'd want a notification for an app you're looking right at. The point of this system is not to replace your UI obligations in the app, but to notify you when you're not looking at your app.

I'm don't like adding hidden prefs for minor things like this, we can't make prefs for everything. I think we need a stronger case for a pref, though I'd prefer to discuss that in another bug.
Attachment #652514 - Flags: review?(doug.turner)
(In reply to Josh Aas (Mozilla Corporation) from comment #41)
> I don't see why you'd want a notification for an app you're looking right
> at. The point of this system is not to replace your UI obligations in the
> app, but to notify you when you're not looking at your app.
For FF I absolutely agree with you. I don't know if you use Thunderbird. Because currently if you start Thunderbird and you get new mails, than you see a Growl notification about the new mails (with Growl installed) with the sender and so on. With your patch this behaviour (which exists now since TB 3) is now changed. Getting notifications with TB in the foreground is also very usefull when you get a new mail and you are currently composing another mail (and the compose window hides the main window). So you can easily see everything important in the Growl notification of this new mail while composing. This also doesn't work anymore with this patch.
Comment on attachment 652514 [details] [diff] [review]
fix v1.0

Review of attachment 652514 [details] [diff] [review]:
-----------------------------------------------------------------

what is the plan to hook up onclicks callbacks?  This is pretty important imo.

::: toolkit/components/alerts/mac/ObserverPair.h
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#import <Cocoa/Cocoa.h>

This code is just a move, right?

::: toolkit/components/alerts/mac/ObserverPair.mm
@@ +14,5 @@
> +
> +  if ((self = [super init])) {
> +    NS_ADDREF(observer = aObserver);
> +    NS_IF_ADDREF(window = aWindow);
> +  }

You're missing the code that safeguarded against calling NS_RELEASE on uninitialized memory.

::: toolkit/components/alerts/mac/nsGrowlAlertsService.mm
@@ +187,5 @@
> +  // Only use Growl with OS version < 10.8.
> +  SInt32 osVersion = 0;
> +  OSErr err = ::Gestalt (gestaltSystemVersion, &osVersion);
> +  osVersion &= 0xFFFF; // The system version is in the low order word
> +  mUsingGrowl = !!((err != noErr) || (osVersion < 0x00001080));

Do you really need mUsingGrowl?  Can't you just use mGrowlDelegate everywhere else but here?

@@ +226,2 @@
>  
>    if (notifications)

The style is mixed in this file.  I think the style should be:

if (test) {
  stmt;
}

@@ +286,1 @@
>      if (NS_SUCCEEDED(rv)) {

I tend to like returning early.  I think it will make this code cleaner.
Attachment #652514 - Flags: review?(doug.turner) → review-
(In reply to Doug Turner (:dougt) from comment #43)

> what is the plan to hook up onclicks callbacks?  This is pretty important
> imo.

I agree this is important but it is hard to do with Notification Center. See my earlier comments about it. We may be able to pull it off by changing how notifications work internally, and I think we should handle that in another bug.

> ::: toolkit/components/alerts/mac/ObserverPair.h
...
> This code is just a move, right?

Right.

> ::: toolkit/components/alerts/mac/ObserverPair.mm
> @@ +14,5 @@
> > +
> > +  if ((self = [super init])) {
> > +    NS_ADDREF(observer = aObserver);
> > +    NS_IF_ADDREF(window = aWindow);
> > +  }
> 
> You're missing the code that safeguarded against calling NS_RELEASE on
> uninitialized memory.

That code isn't necessary. I rewrote the function to follow the normal Cocoa/Obj-C pattern for init. We now always return "self", and if self == nil then we return nil. That will, in practice, never happen -- if NSObject initialization is returning nil there is something seriously wrong and our code probably won't matter. But if it did return nil for some reason, we still wouldn't crash because of the uninitialized pointers - we'd call "release" on nil, which doesn't actually execute anything (nil calls are safe in Cocoa though), so we aren't touching the bad memory. Hope this makes sense.
(In reply to Doug Turner (:dougt) from comment #43)
> Comment on attachment 652514 [details] [diff] [review]
> fix v1.0

> ::: toolkit/components/alerts/mac/nsGrowlAlertsService.mm
> @@ +187,5 @@
> > +  // Only use Growl with OS version < 10.8.
> > +  SInt32 osVersion = 0;
> > +  OSErr err = ::Gestalt (gestaltSystemVersion, &osVersion);
> > +  osVersion &= 0xFFFF; // The system version is in the low order word
> > +  mUsingGrowl = !!((err != noErr) || (osVersion < 0x00001080));
> 
> Do you really need mUsingGrowl?  Can't you just use mGrowlDelegate
> everywhere else but here?

Good catch, fixed in the upcoming patch.

> @@ +226,2 @@
> The style is mixed in this file.  I think the style should be:
> 
> if (test) {
>   stmt;
> }

Fixed in the upcoming patch.

> @@ +286,1 @@
> >      if (NS_SUCCEEDED(rv)) {
> 
> I tend to like returning early.  I think it will make this code cleaner.

Hard to do an early return here actually, but I did find a bad indentation mistake in this code that might have been confusing.
Attached patch fix v1.1Splinter Review
Attachment #652514 - Attachment is obsolete: true
Attachment #654079 - Flags: review?(doug.turner)
Attachment #654079 - Flags: review?(doug.turner) → review+
https://hg.mozilla.org/mozilla-central/rev/67c5213dfe2b
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
This bug added new NSPR types.  Please avoid doing that!  (See bug 579517 for more info).  Thanks!
Apologies, I took some of them out before check-in but it looks like I missed some.
Depends on: 785599
Depends on: 785894
Depends on: 785897
Depends on: 785904
(In reply to Josh Aas (Mozilla Corporation) from comment #44)
> (In reply to Doug Turner (:dougt) from comment #43)
> 
> > what is the plan to hook up onclicks callbacks?  This is pretty important
> > imo.
> 
> I agree this is important but it is hard to do with Notification Center. See
> my earlier comments about it. We may be able to pull it off by changing how
> notifications work internally, and I think we should handle that in another
> bug.

As suggested in bug 785904 comment 4, can we set hasActionButton/actionButtonTitle on notifications that set textClickable and pass an observer to showAlertNotification? Would sending alertfinished before alertclickcallback be a problem?
(In reply to Reuben Morais [:reuben] from comment #51)
> Would sending alertfinished before alertclickcallback be a problem?

Yes, that's exactly what the problem is, I should have read the entire patch before posting, sorry for the bugspam.
Blocks: 793432
Depends on: 805750
Depends on: 807079
I am not certain if I need to file a new bug, please advice.

Running the example shown in http://jsbin.com/notification/1/edit to generate notifications, the banner for Firefox looks like the image below:

  http://i49.tinypic.com/35b8e3c.png

Chrome/Safari look like the one shown below for Chrome:

  http://i47.tinypic.com/ogw8xj.png

All three browsers have the same settings as the example below:

  http://i46.tinypic.com/6jdk7p.png

And with this test, Chrome and Safari show the notification in the Notification Center pane as shown below, while Firefox shows nothing.

  http://i46.tinypic.com/fwucrk.png

Should this bug be reopened or another one created to cover for the integration into the notification center? The latter might be something currently broken, as I cannot reproduce an entry in the Notification Center pane following the details in bug 805750.
This Bug was never about NC for WebNotifications. For now, WN intentionally don't use NC but XUL Alerts. I think there was a Bug about supporting NC for WebNotifications, please search for it before opening a new Bug (you might want to check the dependencies of the WN Bugs).
Thanks, I found bug 852648 already exists for NC support of Web Notifications.
You need to log in before you can comment on or make changes to this bug.