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

RESOLVED FIXED in mozilla17

Status

()

Toolkit
XUL Widgets
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: zpao, Assigned: Josh Aas)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs)

Trunk
mozilla17
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

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.

Updated

6 years ago
Blocks: 728438
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.

Comment 2

5 years ago
"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.
Created attachment 626175 [details] [diff] [review]
Patch v0.1 (WIP)

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.

Updated

5 years ago
Blocks: 728385

Updated

5 years ago
Blocks: 767083

Comment 15

5 years ago
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.

Comment 17

5 years ago
(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.

Comment 18

5 years ago
(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
Duplicate of this bug: 767083

Comment 21

5 years ago
(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

Comment 22

5 years ago
This is the Chromium implementation of the Notification Center:
http://codereview.chromium.org/10021026/

Comment 23

5 years ago
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.

Comment 24

5 years ago
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.)
(Assignee)

Comment 25

5 years ago
Paul - seems like you're not working on this any more, mind if I take it?
(Assignee)

Comment 26

5 years ago
Created attachment 650800 [details] [diff] [review]
fix v0.2

Updated to apply to current trunk, no other progress yet.
Assignee: paul → joshmoz
Attachment #626175 - Attachment is obsolete: true
(Assignee)

Comment 27

5 years ago
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
(Assignee)

Comment 28

5 years ago
Created attachment 650806 [details] [diff] [review]
fix v0.3
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?
(Assignee)

Comment 30

5 years ago
(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.
(Assignee)

Comment 31

5 years ago
Created attachment 650831 [details] [diff] [review]
fix v0.4

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

Comment 32

5 years ago
Created attachment 650955 [details] [diff] [review]
fix v0.8

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

Comment 33

5 years ago
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?
(Assignee)

Comment 34

5 years ago
(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?

Comment 35

5 years ago
(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.
(Assignee)

Comment 36

5 years ago
Created attachment 652514 [details] [diff] [review]
fix v1.0

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+

Comment 38

5 years ago
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...
(Assignee)

Comment 41

5 years ago
(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.
(Assignee)

Updated

5 years ago
Attachment #652514 - Flags: review?(doug.turner)

Comment 42

5 years ago
(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-
(Assignee)

Comment 44

5 years ago
(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.
(Assignee)

Comment 45

5 years ago
(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.
(Assignee)

Comment 46

5 years ago
Created attachment 654079 [details] [diff] [review]
fix v1.1
Attachment #652514 - Attachment is obsolete: true
Attachment #654079 - Flags: review?(doug.turner)

Updated

5 years ago
Attachment #654079 - Flags: review?(doug.turner) → review+
(Assignee)

Comment 47

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/67c5213dfe2b
https://hg.mozilla.org/mozilla-central/rev/67c5213dfe2b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
This bug added new NSPR types.  Please avoid doing that!  (See bug 579517 for more info).  Thanks!
(Assignee)

Comment 50

5 years ago
Apologies, I took some of them out before check-in but it looks like I missed some.

Updated

5 years ago
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.

Updated

5 years ago
Blocks: 793432
Depends on: 793519

Updated

5 years ago
Depends on: 805750
Depends on: 807079

Comment 53

4 years ago
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.

Comment 54

4 years ago
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).

Comment 55

4 years ago
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.